544442 - Add support for signed AUS update snippets

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Daniel Veditz [:dveditz]]

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.

Comment 1

[Daniel Veditz [:dveditz]]

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.

Comment 2

[Reed Loden [:reed]]

(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.

Comment 3

[Ben Bucksch (:BenB)]

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

[Eddy Nigg (StartCom)]

(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

[Ben Bucksch (:BenB)]

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

Comment 6

[Eddy Nigg (StartCom)]

I know, I wondered which was the second.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

Comment 8

[Daniel Veditz [:dveditz]]

(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

[Ben Bucksch (:BenB)]

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.

Comment 10

[Lucas Adamski [:ladamski]]

Doing our own private root presents issues with 3rd party authentication.  No OS would recognize it.

Comment 11

[Ben Bucksch (:BenB)]

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

Comment 12

[Ben Bucksch (:BenB)]

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.

Comment 13

[Robert Strong (they/them - no direct email)]

Taking and I have a patch in progress.

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch for CertUtils.jsm

Still finishing up the fallback implementation for app update

Comment 15

[Robert Strong (they/them - no direct email)]

Comment on attachment 446077 [details] [diff] [review]
patch for CertUtils.jsm

Also want dveditz to take a look

Comment 16

[Dave Townsend [:mossop]]

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.

Comment 17

[Robert Strong (they/them - no direct email)]

(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.

Comment 18

[Daniel Veditz [:dveditz]]

(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 19

[Daniel Veditz [:dveditz]]

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).

Comment 20

[Robert Strong (they/them - no direct email)]

(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

[Ben Bucksch (:BenB)]

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

Comment 22

[Ben Bucksch (:BenB)]

> 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.

Comment 23

[Robert Strong (they/them - no direct email)]

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?

Comment 24

[Johnathan Nightingale [:johnath]]

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!

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.

Comment 26

[Johnathan Nightingale [:johnath]]

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.

Comment 27

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, but what about EV?

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 29

[Robert Strong (they/them - no direct email)]

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.

Comment 30

[Robert Strong (they/them - no direct email)]

Attachment: 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.

Comment 31

[Dave Townsend [:mossop]]

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?

Comment 32

[Robert Strong (they/them - no direct email)]

(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.

Comment 33

[Robert Strong (they/them - no direct email)]

Attachment: CertUtils.jsm patch with comments addressed

Carrying forward r+

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Before we land this, we're sure that it works with AUS? :) If so, bombs away!

Comment 35

[Robert Strong (they/them - no direct email)]

Attachment: 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.

Comment 36

[Robert Strong (they/them - no direct email)]

(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 37

[Dave Townsend [:mossop]]

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?

Comment 38

[Robert Strong (they/them - no direct email)]

Attachment: App update patch with tests rev2 - updated to comments

Comment 39

[Robert Strong (they/them - no direct email)]

Comment on attachment 461852 [details] [diff] [review]
App update patch with tests rev2 - updated to comments

Forgot to refresh

Comment 40

[Robert Strong (they/them - no direct email)]

Attachment: App update patch with tests rev2 - updated to comments

Comment 41

[Dave Townsend [:mossop]]

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.

Comment 42

[Robert Strong (they/them - no direct email)]

I considered that but wanted to avoid evaluating the snippet unless the cert checks passed

Comment 43

[Robert Strong (they/them - no direct email)]

Attachment: CertUtils.jsm patch updated to use aAllowNonBuiltInCerts from Bug 581176 in the test

carrying forward r+

Comment 44

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/14bbdcaf695f
http://hg.mozilla.org/mozilla-central/rev/c262b545756f

Comment 46

[Kaspar]

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

[Ben Bucksch (:BenB)]

> 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

[Kaspar]

(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.