917510 - Replace SHA-1 fingerprints of EV certs in ExtendedValidation.cpp with SHA-2 fingerprints

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Note that I am working on a patch that renames security/manager/ssl/src/nsIdentityChecking.cpp to security/certverifier/ExtendedValidation.cpp.

The EV code that registers OIDs looks up the root certs from NSS and compares their SHA-1 fingerprint to an expected SHA-1 fingerprint (ev_root_sha1_fingerprint). We should replace this with a comparison of SHA-2 fingerprints, as part of the effort to reduce reliance on SHA-1.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Basically, to fix this bug, we need to recalculate all the fingerprints of all the built-in roots that are EV using SHA-384 instead of SHA-1, replace the code that calculates the fingerprints to use SHA-384 instead of SHA-1, and replace the hard-coded SHA-1 hashes with the SHA-384 hashes. All of this is (will be) in security/certverifier/ExtendedValidation.cpp.

Comment 2

[Kaspar]

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #1)
> replace the hard-coded SHA-1 hashes with the SHA-384 hashes.

Is there a particular reason to prefer SHA-384 over SHA-256)? Microsoft's trust list (http://ctldl.windowsupdate.com/msdownload/update/v3/static/trustedr/en/authroot.stl) sticks to SHA-2, FYI (cf. their property with OID 1.3.6.1.4.1.311.10.11.98 OID).

Comment 3

[Mike Hoye [:mhoye]]

Changing the whiteboard to good-next. If there's uncertainty about what the right move is, this is not a good first bug.

Would one of you be willing to mentor a contributor on this?

Comment 4

[:Cykesiopka]

Attachment: bug917510_v0.patch

Something like this?

I haven't pushed this to the Try server yet, but I'll do so if this patch is in the right direction.

Comment 5

[:Cykesiopka]

Comment on attachment 8428439 [details] [diff] [review]
bug917510_v0.patch

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

::: security/certverifier/ExtendedValidation.cpp
@@ +1006,5 @@
>                        entry.cert->derCert.data,
>                        static_cast<int32_t>(entry.cert->derCert.len));
>      PR_ASSERT(rv == SECSuccess);
>      if (rv == SECSuccess) {
> +      bool same = !memcmp(certFingerprint, entry.ev_root_sha256_fingerprint, sizeof(certFingerprint));

Sorry, just noticed the length of this line, I'll wrap it in the next patch.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8428439 [details] [diff] [review]
bug917510_v0.patch

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

Cykesiopka, could you give us a hint as to how you calculated the SHA-256 fingerprints? That might help with the review.

Kathleen, could you verify that the SHA-2 hashes are correct?

Comment 7

[:Cykesiopka]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #6)
> Cykesiopka, could you give us a hint as to how you calculated the SHA-256
> fingerprints? That might help with the review.

Sure.

I made a (sloppily written) script/extension that enumerates over the certs in nsIX509CertDB2 and prints out some information about each cert, including the two fingerprints in the { 0x.., 0x.. } format (so really I had the browser/NSS do it for me).

I also made the script print out a bunch of sed commands to do search and replace on ExtendedValidation.cpp (I assumed there would be some automated test that would cover this, so I didn't really pay attention to the accuracy of the hashes, apologies if they aren't accurate).

For the xpcshell cert, I ran a modified pp that outputs the SHA-256 fingerprint as well, and gave it security/manager/ssl/tests/unit/test_ev_certs/evroot.der as input.

Comment 8

[Kathleen Wilson]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #6)
> 
> Kathleen, could you verify that the SHA-2 hashes are correct?

I'd be happy to, but...

To get the SHA1 Fingerprint I use the Certificate Manager or 
~/nssbin/pp -t certificate -i certFile -a

As far as I can tell, the SHA256 Fingerprint isn't available in either of these tools. 
Can one or both of these tools be updated to provide the SHA256 Fingerprint too?

Comment 9

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

Hi Kathleen,
Since bug 622332 landed, the sha256 fingerprint is shown in the certificate manager. If you have a recent version of Nightly, it should be there.
I'll file a bug on updating the NSS tools to print modern fingerprints (as opposed to MD5).

Comment 10

[Kathleen Wilson]

Comment on attachment 8428439 [details] [diff] [review]
bug917510_v0.patch

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

I confirm that all of the SHA2 hashes are correct.
Note: I could not find the "CN=XPCShell EV Testing (untrustworthy) CA" root via the Certificate Manager, so I did not check the SHA2 for that one.

Comment 11

[:Cykesiopka]

(In reply to Kathleen Wilson from comment #10)
> I confirm that all of the SHA2 hashes are correct.
> Note: I could not find the "CN=XPCShell EV Testing (untrustworthy) CA" root
> via the Certificate Manager, so I did not check the SHA2 for that one.

Thanks for checking!

Comment 12

[:Cykesiopka]

Attachment: bug917510_v1.patch

+ Update the relevant parts of the HOWTO comment section as well
+ Wrap long memcmp line

Try: https://tbpl.mozilla.org/?tree=Try&rev=f3bf401812a2

Comment 13

[:Cykesiopka]

Comment on attachment 8430459 [details] [diff] [review]
bug917510_v1.patch

(In reply to Cykesiopka from comment #12)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=f3bf401812a2

Nevermind... B2G doesn't even build...

Comment 14

[:Cykesiopka]

Attachment: bug917510_v2.patch

- Use
    ...[32]; // SHA256_LENGTH
  instead of
    [SHA256_LENGTH];

All green try push: https://tbpl.mozilla.org/?tree=Try&rev=9a05634a2d53

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Cykesiopka from comment #14)
> Created attachment 8431361 [details] [diff] [review]
> bug917510_v2.patch
> 
> - Use
>     ...[32]; // SHA256_LENGTH
>   instead of
>     [SHA256_LENGTH];
> 
> All green try push: https://tbpl.mozilla.org/?tree=Try&rev=9a05634a2d53

Please try to #include "hasht.h" so that your original patch works.

Comment 16

[:Cykesiopka]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #15)
> Please try to #include "hasht.h" so that your original patch works.

Sure.

Comment 17

[:Cykesiopka]

Attachment: bug917510_v3.patch

Same as v1, except with the addition of:
  #include "hasht.h"

Try: https://tbpl.mozilla.org/?tree=Try&rev=e6381e95c123

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8431387 [details] [diff] [review]
bug917510_v3.patch

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

Good work!

I reviewed the interdiff between v0, which kwilson and I already reviewed, and this version. In particular, I verified that none of the SHA2 hashes were changed in this patch since kwilson's review.

Comment 19

[:Cykesiopka]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #18)
> Good work!
> 
> I reviewed the interdiff between v0, which kwilson and I already reviewed,
> and this version. In particular, I verified that none of the SHA2 hashes
> were changed in this patch since kwilson's review.

Thanks!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2558aae0cf7

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2558aae0cf7