Last Comment Bug 704988 - Check the hotfix add-on is signed by a specific certificate
: Check the hotfix add-on is signed by a specific certificate
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Dave Townsend [:mossop]
:
Mentors:
Depends on: 707207
Blocks: 691083 694068
  Show dependency treegraph
 
Reported: 2011-11-23 14:30 PST by Dave Townsend [:mossop]
Modified: 2012-01-10 16:02 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
shared patch rev 1 (8.43 KB, patch)
2011-12-02 16:39 PST, Dave Townsend [:mossop]
robert.strong.bugs: review-
Details | Diff | Splinter Review
patch rev 1 (20.92 KB, patch)
2011-12-02 16:41 PST, Dave Townsend [:mossop]
bmcbride: review+
Details | Diff | Splinter Review
shared patch rev 2 (12.24 KB, patch)
2011-12-15 21:48 PST, Dave Townsend [:mossop]
robert.strong.bugs: review+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Dave Townsend [:mossop] 2011-11-23 14:30:48 PST
When the hotfix is downloaded we should verify that it is signed by an expected signing cert.
Comment 1 Jeff D 2011-11-27 01:54:33 PST
Can the scope of this be expanded to support signing any extension, or is there a reason that only the hotfix should be signed?
Comment 2 Dave Townsend [:mossop] 2011-11-27 09:42:48 PST
(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)
Comment 3 Jeff D 2011-11-27 11:26:03 PST
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?)
Comment 4 Dave Townsend [:mossop] 2011-11-27 11:37:54 PST
(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.
Comment 5 Dave Townsend [:mossop] 2011-12-02 16:39:04 PST
Created attachment 578768 [details] [diff] [review]
shared patch rev 1

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.
Comment 6 Dave Townsend [:mossop] 2011-12-02 16:41:14 PST
Created attachment 578769 [details] [diff] [review]
patch rev 1

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 7 Robert Strong [:rstrong] (use needinfo to contact me) 2011-12-05 22:01:48 PST
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 8 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-07 06:33:13 PST
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.
Comment 9 Dave Townsend [:mossop] 2011-12-15 21:48:35 PST
Created attachment 582178 [details] [diff] [review]
shared patch rev 2

Updated from comments and added the additional test.
Comment 10 Robert Strong [:rstrong] (use needinfo to contact me) 2011-12-16 03:26:53 PST
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!
Comment 11 Dave Townsend [:mossop] 2011-12-16 12:08:45 PST
On inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5d8d2fb987d
Comment 12 Matt Brubeck (:mbrubeck) 2011-12-17 09:20:45 PST
https://hg.mozilla.org/mozilla-central/rev/e5d8d2fb987d
Comment 13 christian 2012-01-10 15:33:17 PST
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
Comment 14 Dave Townsend [:mossop] 2012-01-10 16:02:19 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/749ae51cf382

Note You need to log in before you can comment on or make changes to this bug.