When the hotfix is downloaded we should verify that it is signed by an expected signing cert.

Can the scope of this be expanded to support signing any extension, or is there a reason that only the hotfix should be signed?

(In reply to Jeff D from comment #1) > Can the scope of this be expanded to support signing any extension, or is > there a reason that only the hotfix should be signed? I don't understand. Any extension can be signed if you buy yourself an XPI signing key. Here we just want to ensure that the automatically installed hotfix can only ever be something created by Mozilla (or whoever the developer of the application is)

Does this require adding a capability to Firefox, or could the title simply read "Sign the hotfix"? If the former, should the capability be restricted to the hotfix or available to any add-on? (or available to any and all add-ons created by Mozilla?)

(In reply to Jeff D from comment #3) > Does this require adding a capability to Firefox, or could the title simply > read "Sign the hotfix"? It requires Firefox to verify the signature on the XPI we download for the hotfix add-on. > If the former, should the capability be restricted to the hotfix or > available to any add-on? (or available to any and all add-ons created by > Mozilla?) Again, any add-on can already be signed by its developer, we display the signing information in the install UI. We have no way of knowing who a particular add-on should be signed for so there is no way to apply this to other add-ons.

This splits out the cert checking code added in bug 544442 so that we can use the same code to validate a bare cert. It also shares the code used to retrieve the cert attributes from prefs to save us having to write it a second time for this.

This uses the cert checking code to verify the hotfix was signed by an expected certificate. For now just stuck a dummy fingerprint in there so it will always fail.

Comment on attachment 578768 [details] [diff] [review] shared patch rev 1 >diff --git a/toolkit/mozapps/shared/CertUtils.jsm b/toolkit/mozapps/shared/CertUtils.jsm >--- a/toolkit/mozapps/shared/CertUtils.jsm >+++ b/toolkit/mozapps/shared/CertUtils.jsm >@@ -34,23 +34,116 @@ > * use your version of this file under the terms of the MPL, indicate your > * decision by deleting the provisions above and replace them with the notice > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > #endif >-EXPORTED_SYMBOLS = [ "BadCertHandler", "checkCert" ]; >+EXPORTED_SYMBOLS = [ "BadCertHandler", "checkCert", "readCertPrefs", "validateCert" ]; > > const Ce = Components.Exception; > const Ci = Components.interfaces; > const Cr = Components.results; > const Cu = Components.utils; > >+Components.utils.import("resource://gre/modules/Services.jsm"); >+ >+/** >+ * Reads a set of expected certificate attributes from preferences. The returned >+ * array can be passed to calidateCert or checkCert to validate that a s/calidateCert/validateCert/ Also mention that the format of the preferences (e.g. start at *.1., etc.) >+ * certificate matches the expected attributes. >+ * >+ * @param aPrefBranch >+ * The nsIX509Cert to compare to the expected attributes. This isn't right >+ * @return aCerts >+ * An array of JS objects with names / values corresponding to the >+ * expected certificate's attribute names / values. Don't specify aCerts See he following for more info http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html >+ */ >+function readCertPrefs(aPrefBranch) { >+ if (Services.prefs.getBranch(aPrefBranch).getChildList("").length == 0) >+ return null; >+ >+ let certs = []; >+ let counter = 1; >+ while (true) { >+ let prefBranchCert = Services.prefs.getBranch(aPrefBranch + counter + "."); >+ let prefCertAttrs = prefBranchCert.getChildList(""); >+ if (prefCertAttrs.length == 0) >+ break; >+ >+ let certAttrs = {}; >+ for each (let prefCertAttr in prefCertAttrs) >+ certAttrs[prefCertAttr] = prefBranchCert.getCharPref(prefCertAttr); >+ >+ certs.push(certAttrs); >+ counter++; >+ } >+ >+ return certs; >+} Would you mind adding an xpcshell specifically test for this? I know it is tested by code that performs all of the steps but having specific tests often helps with identifying the specific part that fails and it should be a pretty simple test. >+/** >+ * Verifies that an nsIX509Cert matches the expected certificate attribute >+ * values. >+ * >+ * @param aCertificate >+ * The nsIX509Cert to compare to the expected attributes. >+ * @param aCerts >+ * An array of JS objects with names / values corresponding to the >+ * expected certificate's attribute names / values. If this is null or >+ * an empty array then no checks are performed. >+ * @throws NS_ERROR_ILLEGAL_VALUE if a certificate attribute name from the >+ * aCerts param does not exist or the value for a certificate attribute >+ * from the aCerts param is different than the expected value or there >+ * was no certificate and aCerts contained requirements. or aCertificate wasn't specified and aCerts is not null or an empty array. >+ */ >+function validateCert(aCertificate, aCerts) { >+ // If there are no certificate requirements then just exit >+ if (!aCerts || aCerts.length == 0) >+ return; >+ >+ if (!aCertificate) { >+ const missingCertErr = "A required certificate was not present."; >+ Cu.reportError(missingCertErr); >+ throw new Ce(missingCertErr, Cr.NS_ERROR_ILLEGAL_VALUE); >+ } >+ >+ for (var i = 0; i < aCerts.length; ++i) { >+ var error = false; >+ var certAttrs = aCerts[i]; >+ for (var name in certAttrs) { >+ if (!(name in aCertificate)) { >+ error = true; >+ Cu.reportError("Expected attribute '" + name + "' not present in " + >+ "certificate."); >+ break; >+ } >+ if (aCertificate[name] != certAttrs[name]) { >+ error = true; >+ Cu.reportError("Expected certificate attribute '" + name + "' " + >+ "value incorrect, expected: '" + certAttrs[name] + >+ "', got: '" + aCertificate[name] + "'."); If you wouldn't mind could you just add the errors here and above to an array and only report them when there is an error below? I've been meaning to do this but haven't found the time. >+ break; >+ } >+ } >+ >+ if (!error) >+ break; >+ } >+ >+ if (error) { >+ const certCheckErr = "Certificate checks failed. See previous errors " + >+ "for details."; >+ Cu.reportError(certCheckErr); >+ throw new Ce(certCheckErr, Cr.NS_ERROR_ILLEGAL_VALUE); >+ } >+} >+

Comment on attachment 578769 [details] [diff] [review] patch rev 1 Review of attachment 578769 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +55,5 @@ > const PREF_APP_UPDATE_AUTO = "app.update.auto"; > const PREF_EM_HOTFIX_ID = "extensions.hotfix.id"; > const PREF_EM_HOTFIX_LASTVERSION = "extensions.hotfix.lastVersion"; > const PREF_EM_HOTFIX_URL = "extensions.hotfix.url"; > +const PREF_EM_CERT_CHECKATTRIBUTES = "extensions.hotfix.cert.checkAttributes"; Feels a bit odd that hotfixes have it's own .checkAttributes, but share .requireBuiltinCerts with normal addons; but it seems difficult to change that. Something to bring up in security review. @@ +71,5 @@ > > const VALID_TYPES_REGEXP = /^[\w\-]+$/; > > Components.utils.import("resource://gre/modules/Services.jsm"); > +Components.utils.import("resource://gre/modules/CertUtils.jsm"); Nit: Would be nice to avoid global namespace pollution here (given CertUtils.jsm exports a bunch of functions, rather than a object/namespace). Import it into a CertUtils object? @@ +835,5 @@ > + if (!Services.prefs.getBoolPref(PREF_EM_CERT_CHECKATTRIBUTES)) > + return; > + } > + catch (e) { > + // By default don't to certificate checks Nit: Typo s/to/do/ Also, full-stop.

Updated from comments and added the additional test.

Comment on attachment 582178 [details] [diff] [review] shared patch rev 2 >diff --git a/toolkit/mozapps/shared/CertUtils.jsm b/toolkit/mozapps/shared/CertUtils.jsm >--- a/toolkit/mozapps/shared/CertUtils.jsm >+++ b/toolkit/mozapps/shared/CertUtils.jsm >@@ -34,23 +34,126 @@ >... >+ * @param aPrefBranch >+ * The prefix for all preferences, should end with a ".".. nit: extra period at the end of the line Really nice and thanks!

On inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5d8d2fb987d

Comment on attachment 582178 [details] [diff] [review] shared patch rev 2 [Triage Comment] We'll want this on beta as well to test bug 694068