1014344 - use Google's root certs for pinning in addition to their intermediate ones

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1004352#c2

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Use Google's root certs for pinning in addition to their intermediate ones (

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Use Google's root pems in addition to their intermediate certs (

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8426717 [details] [diff] [review]
Use Google's root pems in addition to their intermediate certs (

Review of attachment 8426717 [details] [diff] [review]:
-----------------------------------------------------------------

This list was obtained by grepping "Label" in http://pki.google.com/roots.pem. We have matching builtins for all of them except "Equifax Secure eBusiness CA 2". I think this is OK to omit, because since it's not builtin to Firefox it won't verify anyway (unless the user has installed it for some reason).

Comment 4

[Camilo Viecco (:cviecco)]

Comment on attachment 8426717 [details] [diff] [review]
Use Google's root pems in addition to their intermediate certs (

Review of attachment 8426717 [details] [diff] [review]:
-----------------------------------------------------------------

The process of converting the google pem file into the statis list should be part of genHPKPStaticPins.js. Further is this list comes from a stable URL it should download and process it, if not then the originator file should be part of the mozilla source.

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Camilo,

I didn't download the file in the generator on purpose. The other Chrome files we use are version controlled, and leave an audit trail back to the Chrome bug tracker that we can examine in case anything goes wrong. This one, who knows what the origin or change history is. 

The other reason for downloading the other Chrome files is that they let us easily pick up changes in the pinset that Chrome has already vetted. For this particular case, it's seems pretty clear that they don't want that, so I thought it best to keep a human in the loop. It would be bad to parse their root pems, pin to the hashes, and just assume everything works even if the hash doesn't match one of our own builtins -- better to audit these manually, I think.

Thanks,
Monica

Comment 6

[Camilo Viecco (:cviecco)]

Hello Monica

I understand you want to keep a human in the loop, but if that is the case we
should then add the chrome file to the mozilla source tree (so as you mention
we can have an audit trail). 
If the file contains a cert not in our root store, we should a. not import it
and b. log it similar to the way hsts logs its errors. That way we can track
changes and we aware of discrepancies.

Also, who is going to ensure that this imported data gets updated? (if its human
curated and not automated?) I had imagined that we could automate it and make it
alert and fail if there is a large* discrepancy between the current set and a new
calculation.

Camilo

* by large I mean more than 2 certs delta

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Camilo,

I don't think this can be automated. I think the way that we update it is the same way that Chrome updates, say, the Twitter pinset. Someone sends a request and we add it (see http://code.google.com/p/chromium/issues/detail?id=285472 for an example). This is something that we have to work out with Google prior to productionizing any of their domains.

I can add a copy of the root pems to the repo, but I don't think that the generator should be changing the pinset for Google without human intervention based on the current setup.

Thanks,
Monica

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

http://code.google.com/p/chromium/issues/detail?id=329961 is a better example.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8426717 [details] [diff] [review]
Use Google's root pems in addition to their intermediate certs (

Review of attachment 8426717 [details] [diff] [review]:
-----------------------------------------------------------------

To me it doesn't seem necessary to include Google's root certificates file since we don't even do something similar for our own roots. It would be good to document the process by which that list is arrived at, though.
Also, maybe I'm misunderstanding, but why is the old google pinset being written out in addition to the new one? It isn't used anymore, right? r=me with that fixed and/or investigated.

::: security/manager/boot/src/StaticHPKPins.h
@@ +365,5 @@
>    const StaticFingerprints* sha1;
>    const StaticFingerprints* sha256;
>  };
>  
>  /* Mozilla static pinsets */

Hmmm - this comment doesn't make much sense anymore (either that or the google_root_pems pinset should move).

@@ +445,5 @@
> +  kAddTrust_Qualified_Certificates_RootFingerprint,
> +};
> +static const StaticFingerprints kPinset_google_root_pems_sha256 = { 69, kPinset_google_root_pems_sha256_Data };
> +
> +static const StaticPinset kPinset_google_root_pems = {

Looks like the original google pinset is still written out - is it still needed? We're not using it anymore, right?

::: security/manager/tools/PreloadedHPKPins.json
@@ -27,5 @@
>  {
>    "chromium_data" : {
>      "cert_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.certs",
>      "json_file_url": "https://src.chromium.org/chrome/trunk/src/net/http/transport_security_state_static.json",
> -    "excluded_pinsets": [

I'm assuming you're intentionally removing excluded_pinsets since we're not actually using it yet?

@@ +75,5 @@
>          "End Entity Test Cert"
>        ]
> +    },
> +    // Google's root PEMs and intermediate certs. Chrome pins only to their
> +    // intermediate certs, but they'd like us to be more liberal.

Since right now this is human-curated, it would be good to document where this came from.

Comment 10

[[:mmc] Monica Chew (no longer reading bugmail)]

> Also, maybe I'm misunderstanding, but why is the old google pinset being
> written out in addition to the new one? It isn't used anymore, right? r=me
> with that fixed and/or investigated.

Chatted with keeler over irc, decided to remove the original intermediate certs.
 
> >  /* Mozilla static pinsets */
> Hmmm - this comment doesn't make much sense anymore (either that or the
> google_root_pems pinset should move).

Changed to PreloadedHPKPins.json pinsets
 
> I'm assuming you're intentionally removing excluded_pinsets since we're not
> actually using it yet?

Yeah, it just got swapped for the substituted_pinsets -- not sure if we actually will need excluded_pinsets.

> > +    // Google's root PEMs and intermediate certs. Chrome pins only to their
> > +    // intermediate certs, but they'd like us to be more liberal.
> 
> Since right now this is human-curated, it would be good to document where
> this came from.

Fixed comment, updated to include source.

Comment 11

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Use Google's root pems in addition to their intermediate certs (

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f59d15fc610

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f59d15fc610