Add support for signed AUS update snippets

RESOLVED FIXED in mozilla2.0b3

Status

()

Toolkit
Application Update
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: dveditz, Assigned: rstrong)

Tracking

(Blocks: 1 bug)

Trunk
mozilla2.0b3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 beta3+, status1.9.2 wontfix)

Details

(Whiteboard: [sg:want p1])

Attachments

(2 attachments, 5 obsolete attachments)

The only thing currently securing client updates is a SSL connection to the AUS server. Should there be a flaw anywhere in the PKI infrastructure we no longer have secure updates. In the past year or so we've had bogus "mozilla" certs issued from two different CA's as PoC attacks, a CA that fell to a MD5 collision attack yielding a bogus intermediate cert, and an SSL implementation flaw that let people use embeded nulls to obtain certs for any site they wanted (one of which was again for mozilla as a PoC -- it's fun to be popular).

We need to sign the update information and check it against a key shipped with the client, in addition to the SSL connection.
Component: Application Update → Release Engineering
OS: Mac OS X → All
Product: Toolkit → mozilla.org
QA Contact: application.update → release
Hardware: x86 → All
Version: unspecified → other
Reed: Please file your own damn bug if you want one as a placeholder for releng work that cannot possibly happen until we design a signing system in the client.
Component: Release Engineering → Application Update
Product: mozilla.org → Toolkit
QA Contact: release → application.update
Version: other → Trunk
(In reply to comment #1)
> Reed: Please file your own damn bug if you want one as a placeholder for releng
> work that cannot possibly happen until we design a signing system in the
> client.

Well, the summary as filed is clearly releng-specific, so let me update it to something better. In the end, will need a releng bug to handle the creation of the signed snippets themselves and an AUS bug to provide the signing information somehow.
Summary: AUS update snippets should be signed → Add support for signed AUS update snippets

Comment 3

8 years ago
dveditz: Great bug!

> sign the update information and check it against a key shipped with the client

How exactly do you want the signing to work?
PGP/GPG signatures of the files checked by the client?

Comment 4

8 years ago
(In reply to comment #0)
> In the past year or so we've had bogus "mozilla" certs
> issued from two different CA's

Huuu? Which were those two?

Comment 5

8 years ago
Eddy, one was bug 470897 and you're the cert holder.

Comment 6

8 years ago
I know, I wondered which was the second.
I do not think that we should invent our own PKI here, just as we should not invent our own cipher or HMAC for this process.  For one thing, we need to understand how we would revoke that key without stranding all our users, should it be compromised.  We're talking about doing this because of successful attacks against key management systems, and even _I_ have insufficient hubris to believe that we will be immune to similar attacks if we effectively become our own CA.

It seems to me that we would be better off adding (in the client) additional constraints on the required SSL connection for updates, such as:

- requiring an EV cert with a given organization name, raising the bar for purely algorithmic attacks substantially and providing OCSP

- requiring a specific root/issuer, to limit the scope of viable attacks (or just using a custom trust bit for this)

- requiring a certain set of ciphers in various parts of the chain
(In reply to comment #6)
> I know, I wondered which was the second.

I thought Zusman had gotten one but apparently I misremembered the domains.
http://schmoil.blogspot.com/2009/01/nobody-is-perfect.html
Point still stands though: two separate CA's were demonstrated to have different issuing flaws resulting in fraudulent certs, however short-lived.

Comment 9

8 years ago
Our own root would be a hundred times more secure that requiring a specific CA with specific cert owner name, because that CA also has to secure their servers and have a much tougher job, given that they need to sell these things to everyone.

Mike, there's a huge difference between a public PKI system which sells certs to everyone with a credit card, and your own root cert (stored away securely offline) which issues a cert, which is used only on your own release build machines.
And the release build machines better be secure, otherwise no singing can help you anymore *hint hint*. (Note the existence of root kits and how hard it would be to detect a hidden modification during build.) So, the "we can't secure that" argument holds absolutely no water.
Doing our own private root presents issues with 3rd party authentication.  No OS would recognize it.

Comment 11

8 years ago
Lucas, we're talking about our update servers. Only our clients access them.

Comment 12

8 years ago
Here's one way how it could work:
1. We already sign releases using our PGP key. Sign the partial update files, too.
2. Ship GPG binary with Firefox (ignoring license and download footprint
   for now, it's just a proposal) and our public PGP (root) key.
3. The updater downloads update binary file and its .sig.
4. Before using the binary, the updater runs gpg --verify file.sig and
   only proceeds when the verification and key passes.


Before anyone says "revocation", that's simple: Make a root key, store it securely on 2 USB flash sticks somewhere, and let that sign the key which is on the release build machines and actually signs the releases. If that ever gets broken (how would it? by the release machines being hacked?) or lost, revoke it and use the root key to sign a new key. The clients are configured to trust the root key and any keys it signed.
Taking and I have a patch in progress.
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Created attachment 446077 [details] [diff] [review]
patch for CertUtils.jsm

Still finishing up the fallback implementation for app update
Attachment #446077 - Flags: review?(dtownsend)
Comment on attachment 446077 [details] [diff] [review]
patch for CertUtils.jsm

Also want dveditz to take a look
Attachment #446077 - Flags: review?(dveditz)
Comment on attachment 446077 [details] [diff] [review]
patch for CertUtils.jsm

So this only verifies the attributes on the final server? That seems ok to me but just wanted to make sure it was called out.

>diff --git a/toolkit/mozapps/shared/test/chrome/test_bug544442_checkCert.xul b/toolkit/mozapps/shared/test/chrome/test_bug544442_checkCert.xul

>+  var cert = channel.securityInfo.QueryInterface(Ci.nsISSLStatusProvider).
>+             SSLStatus.QueryInterface(Ci.nsISSLStatus).serverCert;
>+
>+  certAttrs = { issuerName: cert.issuerName,
>+                commonName: cert.commonName };

Can we verify the actual values here? They should be fixed for the test server.
Attachment #446077 - Flags: review?(dtownsend) → review+
blocking2.0: ? → beta1+
(In reply to comment #16)
> (From update of attachment 446077 [details] [diff] [review])
> So this only verifies the attributes on the final server? That seems ok to me
> but just wanted to make sure it was called out.
> 
> >diff --git a/toolkit/mozapps/shared/test/chrome/test_bug544442_checkCert.xul b/toolkit/mozapps/shared/test/chrome/test_bug544442_checkCert.xul
> 
> >+  var cert = channel.securityInfo.QueryInterface(Ci.nsISSLStatusProvider).
> >+             SSLStatus.QueryInterface(Ci.nsISSLStatus).serverCert;
> >+
> >+  certAttrs = { issuerName: cert.issuerName,
> >+                commonName: cert.commonName };
> 
> Can we verify the actual values here? They should be fixed for the test server.
Since the test also checks failure of an incorrect cert attribute name / value I prefer to not hardcode the values and instead use the values from the cert which future proofs the test.
(In reply to comment #16)
> So this only verifies the attributes on the final server? That seems ok to me
> but just wanted to make sure it was called out.

That should be OK: however we got there we started out with an SSL URL that NSS said was OK, and then in the end if it directed us off-course this is one last double-check.
Comment on attachment 446077 [details] [diff] [review]
patch for CertUtils.jsm

Looks good as additional support in CertUtils, r=dveditz, but not a fix for this bug until the callers are patched (is that in a different bug, or just a future patch here?).

This approach, though, is only a minor bit of progress or risks leaving users behind. If we only check the common name, for example, then it's not solving the original demonstrated scenario of CAs doing a bad job. If we check the issuer (I'm assuming we must) then we're locked into a particular CA, and heaven help us if they issue our next cert from a different intermediate (Verisign's CA business bought by Symantec?). Plus if someone could get an bogus intermediate CA cert (e.g. the Sotirov, Applebaum et al MD5-hash attack) they could create a sub-intermediate that matches our issuer and issue the end-cert from that. [A chain length-check could prevent that particular attack.]

If we go all the way and check the specific fingerprint (sha1 hash) we may defeat all these attacks but then we are guaranteed updates break the next time our server cert expires. We can't afford to make updates any harder for people.

Checking for EV would help (as suggested in comment 7) but that's not a simple nsIX509Cert property that could be checked with this patch. Is that coming in the callsite patch?

If we required EV we'd have to actually get one for aus2.mozilla.org before we could turn this on.

I know Shaver didn't like the original idea of signing the actual snippets in some static fashion (comment 7 again), but it would have the advantage that users of old builds would forever be able to update against the previously generated snippets (might take multiple updates to get current, but doable).
Attachment #446077 - Flags: review?(dveditz) → review+
(In reply to comment #19)
> (From update of attachment 446077 [details] [diff] [review])
> Looks good as additional support in CertUtils, r=dveditz, but not a fix for
> this bug until the callers are patched (is that in a different bug, or just a
> future patch here?).
Future patch in this bug... I wanted to make sure you were ok with the approach before finishing / submitting the app update patch.

> This approach, though, is only a minor bit of progress or risks leaving users
> behind. If we only check the common name, for example, then it's not solving
> the original demonstrated scenario of CAs doing a bad job. If we check the
> issuer (I'm assuming we must) then we're locked into a particular CA, and
> heaven help us if they issue our next cert from a different intermediate
> (Verisign's CA business bought by Symantec?). Plus if someone could get an
> bogus intermediate CA cert (e.g. the Sotirov, Applebaum et al MD5-hash attack)
> they could create a sub-intermediate that matches our issuer and issue the
> end-cert from that. [A chain length-check could prevent that particular
> attack.]
Per our discussions this will check the cert's issuerName and commonName. Do you want to change this? It is very simple to add other checks if you want to add more.

> If we go all the way and check the specific fingerprint (sha1 hash) we may
> defeat all these attacks but then we are guaranteed updates break the next time
> our server cert expires. We can't afford to make updates any harder for people.
We could purchase a cert for a different host (aus3.mozilla.org) and make the client changes (e.g. url, cert attributes to check) well before expiration. Doesn't prevent human error where we forget to do this in a reasonable amount of time of course.

> Checking for EV would help (as suggested in comment 7) but that's not a simple
> nsIX509Cert property that could be checked with this patch. Is that coming in
> the callsite patch?
I wasn't planning on it since it wasn't requested in any of the conversation regarding the fix for this bug.

> If we required EV we'd have to actually get one for aus2.mozilla.org before we
> could turn this on.
> 
> I know Shaver didn't like the original idea of signing the actual snippets in
> some static fashion (comment 7 again), but it would have the advantage that
> users of old builds would forever be able to update against the previously
> generated snippets (might take multiple updates to get current, but doable).
Agreed.

To prevent rework, could you verify again that the approach we discussed where we check the issuerName and commonName of the cert is sufficient for this bug? If we were to get an EV cert for AUS would it be enough to just have a flag that requires the cert is an EV cert along with the verification that this patch currently performs or would you want to do something else?

Comment 21

8 years ago
You sometimes have "throw new Ce(" and sometimes "throw Ce(". I guess the former is correct?

Comment 22

8 years ago
> To prevent rework, could you verify again that the approach we discussed where
> we check the issuerName and commonName of the cert is sufficient for this bug?

The original description stated "Should there be a flaw anywhere in the PKI infrastructure...". I don't think anything relying on any third party (EV CA or not) is sufficient, nor needed here. We're in control of the client, the server and the bits shipped by the server, no need to rely on third parties. We don't need to invent a new system, just remove the third party CAs.
blocking2.0: beta1+ → beta2+
Ben, thanks for the drive by review... changed to throw new locally. Regarding the implementation you have proposed, the vast majority of that work would need to be done outside of the app update component and discussion of the proposal would likely be better in the newsgroups. Not saying that the proposal doesn't have a merits... just that it would need to be discussed / decided upon elsewhere.

Dan, I've pinged you a couple of times regarding our face to face discussion where you asked me to hold off on going any further with this for now... any updates?
Okay, prodded Dan about this in IRC, here's where I got to - stop me if I'm being crazy.

THE FACTS AS I UNDERSTAND THEM:

- Our AUS snippets are served over SSL (good) but are vulnerable to an attacker that can obtain a fraudulent mozilla cert (bad) which has been known to happen (also bad).

- This approach proposes to incrementally improve that situation by giving us the ability to bind updates to certain cert attributes (CN + Issuer for instance) thereby reducing the available attack surface, while not actually eliminating it

THE CATCHES:

- This binds us to a particular CA, making operational decisions around which CA to use into potentially-update-breaking decisions
- This is not resistant to attacks that undermine whatever CA we've chosen

THE PROPOSALS:

1) One thing we could do is bless a few CAs. Gives us a migration path if we need to kill of our primary cert, but increases our attack surface (albeit to 2, which is a lot less than $CAs_in_NSS).

2) Another thing we could do is bind to the certificate's *key* instead of its human readable attributes. A key is portable to other certs, so if we needed to switch CAs, we could do so (file a new CSR with the new CA, but supply the same public key). If we ever lose this key we are in a heap of trouble (no more ability to update). If the key is ever compromised, we'll be in a race with the attacker to get our users updated to a new key, although they will be slowed by needed to steal our key AND get a fraudulent mozilla cert issued using it (we wouldn't be getting rid of our existing "valid SSL" requirements, after all).

3) We could build our own CA/signing key/go-kart.

#3 is crypto-purity-appealing, but I agree with Shaver that inventing crypto systems is heebie-jeebie inspiring. #1 is not great, but would represent an incremental improvement without completely tying our hands - I could get behind that. #2 feels like a better version of #1, except for the risk of key loss. We could build in a second key that we never bring online except in emergencies, but HEY LOOK, INVENTING CRYPTO AGAIN.

That's as far as we got. I want to get this unstuck, because incremental improvements are within reach. I asked Dan to get together with Rob and Chris Lyon and Shaver-if-findable tomorrow to work this through and figure out what comes next. I understand the concerns around the current approach making us over-dependent on a particular issuer, but it feels like we're close.

Have I misstated facts, or missed obviousness? If not, I look forward to the comments in this bug tomorrow!
Requiring an EV cert for the SSL might be sufficient to reduce the fake-cert risk to acceptable levels?  That would, I guess, be binding to multiple cert attributes, as you describe in #2.
Yeah - binding to EV is something I've considered too (which is good, since it's mentioned in this very bug!), and significantly increases the threshold for obtaining a fraudulent cert. It still constrains us operationally to some extent, since EV certs take time for legitimate applicants to get as well - we were burned last year, you'll recall, when an EV cert we obtained couldn't handle the OCSP load, and we had to revert to the DV cert it replaced.

The IT/operations risk here is a big part of why I suggested that Dan talk it through with clyon and mrz/justin. I don't know what I don't know about the risks/flexibility-level there.
blocking2.0: beta2+ → beta3+
What's keeping this from landing? It's a b3 blocker, not sure that's appropriate, but also not sure what decisions/work needs to be done to get it closed out.
beltzner, I was waiting on input from Dan (see comment #23) which has now been resolved but required changing the approach. Then I had another blocker to fix along with other work. I changed the approach in the CertUtils.jsm and I am now working on the client side. I hope to it ready for review by tomorrow.
Created attachment 461333 [details] [diff] [review]
Updated patch for CertUtils.jsm

After discussing this with johnath and dveditz we decided to add the ability to have multiple certs checked so it is easy to specify a fallback if the current cert is revoked.
Attachment #446077 - Attachment is obsolete: true
Attachment #461333 - Flags: review?(dtownsend)
Comment on attachment 461333 [details] [diff] [review]
Updated patch for CertUtils.jsm

>-  var errorstring = "cert issuer is not built-in";
>+  var issuerCert = cert;
>+  var issuer = issuerCert.issuer;
>+  while (issuer && !issuerCert.equals(issuer)) {
>+    issuerCert = issuer;
>+    issuer = issuerCert.issuer;
>+  }

I'm not sure but this reads slightly better to me:

var issuerCert = cert;
while (issuerCert.issuer && !issuerCert.issuer.equals(issuerCert))
  issuerCert = issuerCert.issuer;

..
if (!issuerCert.issuer)

Also moving the issuer check above the attribute check would mean you could return at the first one without error rather than having to check the error twice. I guess that makes testing harder though right?
Attachment #461333 - Flags: review?(dtownsend) → review+
(In reply to comment #31)
> Comment on attachment 461333 [details] [diff] [review]
> Updated patch for CertUtils.jsm
> 
> >-  var errorstring = "cert issuer is not built-in";
> >+  var issuerCert = cert;
> >+  var issuer = issuerCert.issuer;
> >+  while (issuer && !issuerCert.equals(issuer)) {
> >+    issuerCert = issuer;
> >+    issuer = issuerCert.issuer;
> >+  }
> 
> I'm not sure but this reads slightly better to me:
> 
> var issuerCert = cert;
> while (issuerCert.issuer && !issuerCert.issuer.equals(issuerCert))
>   issuerCert = issuerCert.issuer;
Thanks

> ..
> if (!issuerCert.issuer)
> 
> Also moving the issuer check above the attribute check would mean you could
> return at the first one without error rather than having to check the error
> twice. I guess that makes testing harder though right?
I need the check after the cert attributes to be able to test this code since our tests never have a builtin cert.
Created attachment 461482 [details] [diff] [review]
CertUtils.jsm patch with comments addressed

Carrying forward r+
Attachment #461333 - Attachment is obsolete: true
Attachment #461482 - Flags: review+
Before we land this, we're sure that it works with AUS? :) If so, bombs away!
Created attachment 461717 [details] [diff] [review]
App update patch with test rev1

Talked with beltzner and we are going to go with no notification for the time being. Bug 583408 is for figuring the notification out.
Attachment #461717 - Flags: review?(dolske)
(In reply to comment #34)
> Before we land this, we're sure that it works with AUS? :) If so, bombs away!
I've been testing with AUS and it works fine.
Comment on attachment 461717 [details] [diff] [review]
App update patch with test rev1

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js

>+    var errCount = getPref("getIntPref", PREF_APP_UPDATE_CERT_ERROR_COUNT, 0);
>+    var status;
>     try {
>-      gCertUtils.checkCert(this._request.channel);
>-      // Analyze the resulting DOM and determine the set of updates to install
>-      var updates = this._updates;
>+      try {
>+        gCertUtils.checkCert(this._request.channel, certs);
>+         // Clear error certificate error count if there were previous errors
>+         // and the certificate attribute check was successful.
>+         if (errCount > 0)
>+          gPref.clearUserPref(PREF_APP_UPDATE_CERT_ERROR_COUNT);

Fix the indentation here and you also need to set errCount to 0 or the update found immediately after a set of failures will get ignored. Can you add a test for that?
Attachment #461717 - Flags: review?(dolske) → review+
Created attachment 461852 [details] [diff] [review]
App update patch with tests rev2 - updated to comments
Attachment #461717 - Attachment is obsolete: true
Comment on attachment 461852 [details] [diff] [review]
App update patch with tests rev2 - updated to comments

Forgot to refresh
Attachment #461852 - Attachment is obsolete: true
Created attachment 461853 [details] [diff] [review]
App update patch with tests rev2 - updated to comments
Attachment #461853 - Flags: review+
Comment on attachment 461852 [details] [diff] [review]
App update patch with tests rev2 - updated to comments

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js

>+    var certAttrCheckFailed = false;
>+    var status;
>     try {
>-      gCertUtils.checkCert(this._request.channel);
>-      // Analyze the resulting DOM and determine the set of updates to install
>-      var updates = this._updates;
>+      try {
>+        gCertUtils.checkCert(this._request.channel, certs);
>+      }
>+      catch (e) {
>+        Components.utils.reportError(e);
>+        if (e.result != Cr.NS_ERROR_ILLEGAL_VALUE)
>+          throw e;
>+
>+        certAttrCheckFailed = true;
>+      }
>+
>+      // Analyze the resulting DOM and determine the set of updates. If the
>+      // certificate attribute check failed treat it as no updates found until
>+      // Bug 583408 is fixed.
>+      var updates = certAttrCheckFailed ? [] : this._updates;

An alternative here would be to just set updates = this._updates at the start and set it to [] in the failed case and do away with certAttrCheckFailed altogether. But I'm fine with this as-is if it makes more sense for when the notification stuff happens.
Attachment #461852 - Flags: review+
I considered that but wanted to avoid evaluating the snippet unless the cert checks passed
Created attachment 461855 [details] [diff] [review]
CertUtils.jsm patch updated to use aAllowNonBuiltInCerts from Bug 581176 in the test

carrying forward r+
Attachment #461482 - Attachment is obsolete: true
Attachment #461855 - Flags: review+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/14bbdcaf695f
http://hg.mozilla.org/mozilla-central/rev/c262b545756f
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla2.0b3

Updated

8 years ago
Blocks: 593135
Duplicate of this bug: 414668
Blocks: 593571
Blocks: 643461

Comment 46

7 years ago
Two questions:

1) when taking extra efforts to check for the correct issuer, isn't it somewhat risky to retrieve the issuer DN from a pref? That makes it relatively simple for a rogue extension to "disable" this check, effectively.

2) neither aus2.mozilla.org nor aus3.mozilla.org currently use a cert with an OCSP URI - which basically means that revocation checks never take place. Given the recent disaster with Comodo (and the AMO cert, in particular), shouldn't the bar be raised in this case?

Comment 47

7 years ago
> relatively simple for a rogue extension to "disable" this check

A rogue extension can do everything, including overwrite our code. We cannot protect us from them, same as rogue EXEs.

> 2) neither aus2.mozilla.org nor aus3.mozilla.org currently use a cert
> with an OCSP URI

This is already on the radar of the moz-sec group, but thanks for noticing.

Comment 48

7 years ago
(In reply to comment #47)
> A rogue extension can do everything, including overwrite our code. We cannot
> protect us from them, same as rogue EXEs.

I don't think the world is just binary in this case ("extensions have all privileges, so all is lost when a rogue extension is in"). It's about making it less obvious/straightforward for an attacker to work around guards like these.

Also, why does it only check for the commonName attribute in the subject, and not the organizationName, too? This would help ruling out MITMs with DV certs from the same issuer.
status1.9.2: wanted → wontfix

Updated

6 years ago
Blocks: 691083
You need to log in before you can comment on or make changes to this bug.