Closed Bug 917510 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: briansmith, Assigned: Cykesiopka)

References

Details

(Whiteboard: [good next bug])

Attachments

(1 file, 3 obsolete files)

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.
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.
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [good first bug]
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) 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).
Whiteboard: [good first bug] → [good next bug]
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?
Attached patch bug917510_v0.patch (obsolete) — Splinter Review
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.
Attachment #8428439 - Flags: feedback?(brian)
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 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?
Attachment #8428439 - Flags: review?(kwilson)
Attachment #8428439 - Flags: feedback?(brian)
Attachment #8428439 - Flags: feedback+
(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.
(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?
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 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.
Attachment #8428439 - Flags: review?(kwilson) → review+
(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!
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Attached patch bug917510_v1.patch (obsolete) — Splinter Review
+ Update the relevant parts of the HOWTO comment section as well
+ Wrap long memcmp line

Try: https://tbpl.mozilla.org/?tree=Try&rev=f3bf401812a2
Attachment #8428439 - Attachment is obsolete: true
Attachment #8430459 - Flags: review?(brian)
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...
Attachment #8430459 - Attachment is obsolete: true
Attachment #8430459 - Flags: review?(brian)
Attached patch bug917510_v2.patch (obsolete) — Splinter Review
- Use
    ...[32]; // SHA256_LENGTH
  instead of
    [SHA256_LENGTH];

All green try push: https://tbpl.mozilla.org/?tree=Try&rev=9a05634a2d53
Attachment #8431361 - Flags: review?(brian)
(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.
Flags: needinfo?(cykesiopka.bmo)
(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.
Flags: needinfo?(cykesiopka.bmo)
Same as v1, except with the addition of:
  #include "hasht.h"

Try: https://tbpl.mozilla.org/?tree=Try&rev=e6381e95c123
Attachment #8431361 - Attachment is obsolete: true
Attachment #8431361 - Flags: review?(brian)
Attachment #8431387 - Flags: review?(brian)
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.
Attachment #8431387 - Flags: review?(brian) → review+
(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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e2558aae0cf7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.