Detect SHA-1 when it appears in certificate chains

Detect SHA-1 when it appears in certificate chains

BUG=401365
Committed: https://crrev.com/b92e6f5c2a5271850a39afa1254d41ccc34ec5e6
Cr-Commit-Position: refs/heads/master@{#297307}

[Ryan Sleevi, 2014-08-28 04:17:50]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2014-08-28 04:17:50]

David: Would you take a look at this? Please pay particular attention to the
Android side.

I debated moving the algorithm detection to JNI, but that seems pointless and
heavyweight. I also debated whether it was OK to be hardcoding to Boring, but
then realized, hey, it's Android.

The slight concern (and perhaps you have more state) is ensuring that we don't
count the trust anchor (which may not be the root), but I don't think Android's
cert verification supports this.

[davidben, 2014-08-28 19:42:08]

lgtm with comments:

> The slight concern (and perhaps you have more state) is ensuring that we don't
> count the trust anchor (which may not be the root), but I don't think
Android's
> cert verification supports this.

Do you mean problems with intermediates? From what I recall, Android's verifier,
when it gives back a chain, should always return the root. I was worried at some
point that it would stop at an intermediate once it's cached the intermediate,
but I believe it gets this right; it stitches together the chain with that of
the cached intermediate.

If it didn't get this right, the is_known_root logic would fail and have false
negatives. (Which are safer than false positives in terms of not breaking
things, but unfortunate in terms of enforcing our various checks.)

https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_an...
File net/cert/cert_verify_proc_android.cc (right):

https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_an...
net/cert/cert_verify_proc_android.cc:85: // but unverified, chain.
Is this comment accurate? If we were unable to build a chain,
correction_for_root will be 0 and we would check that everything not have SHA-1.
So we're assuming that it's a partial chain with the root missing.

https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_ns...
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_ns...
net/cert/cert_verify_proc_nss.cc:208: case
SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST:
The other cases all list out three DSA OIDs and this only lists one. Poking
around MXR, I also see SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST and
SEC_OID_SDN702_DSA_SIGNATURE.

Looks like...
SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST is NID_dsaWithSHA1
SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST is NID_dsaWithSHA1_2
SEC_OID_SDN702_DSA_SIGNATURE is NID_dsaWithSHA

And then there's SEC_OID_ANSIX9_DSA_SIGNATURE (NID_dsa_2), but that doesn't seem
to specify a hash?

https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_op...
File net/cert/cert_verify_proc_openssl.cc (right):

https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_op...
net/cert/cert_verify_proc_openssl.cc:125: sig_alg == NID_dsaWithSHA1_2 ||
sig_alg == NID_sha1WithRSA ||
Aside: BoringSSL actually can't verify DSA signatures at all right now, and the
DSA-based cipher suites have been removed. If we're using some external
certificate validator, I'm not sure whether or not it would fail the handshake
if the leaf were RSA, but something up the chain was DSA.

Strictly speaking, an SSL implementation shouldn't care in that case, but I
could believe that it would fail trying to parse into an X509* or something.

[Ryan Sleevi, 2014-09-26 20:18:43]

https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_ns...
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_ns...
net/cert/cert_verify_proc_nss.cc:208: case
SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST:
On 2014/08/28 19:42:08, David Benjamin wrote:
> The other cases all list out three DSA OIDs and this only lists one. Poking
> around MXR, I also see SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST and
> SEC_OID_SDN702_DSA_SIGNATURE.
> 
> Looks like...
> SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST is NID_dsaWithSHA1
> SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST is NID_dsaWithSHA1_2
> SEC_OID_SDN702_DSA_SIGNATURE is NID_dsaWithSHA
> 
> And then there's SEC_OID_ANSIX9_DSA_SIGNATURE (NID_dsa_2), but that doesn't
seem
> to specify a hash?

None of these come up within NSS. Plus, DSS...

https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_op...
File net/cert/cert_verify_proc_openssl.cc (right):

https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_op...
net/cert/cert_verify_proc_openssl.cc:125: sig_alg == NID_dsaWithSHA1_2 ||
sig_alg == NID_sha1WithRSA ||
On 2014/08/28 19:42:08, David Benjamin wrote:
> Aside: BoringSSL actually can't verify DSA signatures at all right now, and
the
> DSA-based cipher suites have been removed. If we're using some external
> certificate validator, I'm not sure whether or not it would fail the handshake
> if the leaf were RSA, but something up the chain was DSA.
> 
> Strictly speaking, an SSL implementation shouldn't care in that case, but I
> could believe that it would fail trying to parse into an X509* or something.

Right, I'm fairly certain it will be an error beforehand.

[bannercrown_gmail.com, 2014-09-28 22:05:05]

>
> Enter code here...X509*
>
 



On Friday, September 26, 2014 1:18:44 PM UTC-7, rsl...@chromium.org wrote:
>
> Reviewers: David Benjamin, 
>
>
>
>
https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_ns...

> File net/cert/cert_verify_proc_nss.cc (right): 
>
>
https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_ns...

>
> net/cert/cert_verify_proc_nss.cc:208 
>
<https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_ns...>:

> case 
> SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST: 
> On 2014/08/28 19:42:08, David Benjamin wrote: 
> > The other cases all list out three DSA OIDs and this only lists one. 
> Poking 
> > around MXR, I also see SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST 
> and 
> > SEC_OID_SDN702_DSA_SIGNATURE. 
>
> > Looks like... 
> > SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST is NID_dsaWithSHA1 
> > SEC_OID_BOGUS_DSA_SIGNATURE_WITH_SHA1_DIGEST is NID_dsaWithSHA1_2 
> > SEC_OID_SDN702_DSA_SIGNATURE is NID_dsaWithSHA 
>
> > And then there's SEC_OID_ANSIX9_DSA_SIGNATURE (NID_dsa_2), but that 
> doesn't seem 
> > to specify a hash? 
>
> None of these come up within NSS. Plus, DSS... 
>
>
>
https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_op...

> File net/cert/cert_verify_proc_openssl.cc (right): 
>
>
https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_op...

>
> net/cert/cert_verify_proc_openssl.cc:125 
>
<https://codereview.chromium.org/509273002/diff/1/net/cert/cert_verify_proc_op...>:

> sig_alg == NID_dsaWithSHA1_2 
> || sig_alg == NID_sha1WithRSA || 
> On 2014/08/28 19:42:08, David Benjamin wrote: 
> > Aside: BoringSSL actually can't verify DSA signatures at all right 
> now, and the 
> > DSA-based cipher suites have been removed. If we're using some 
> external 
> > certificate validator, I'm not sure whether or not it would fail the 
> handshake 
> > if the leaf were RSA, but something up the chain was DSA. 
>
> > Strictly speaking, an SSL implementation shouldn't care in that case, 
> but I 
> > could believe that it would fail trying to parse into an X509* or 
> something. 
>
> Right, I'm fairly certain it will be an error beforehand. 
>
> Description: 
> Detect SHA-1 when it appears in certificate chains 
>
> BUG=401365 
>
> Please review this at https://codereview.chromium.org/509273002/ 
>
> SVN Base:   
> https://chromium.googlesource.com/chromium/src.git@cert_status_extended 
>
> Affected files (+98, -31 lines): 
>    M net/cert/cert_verify_proc_android.cc 
>    M net/cert/cert_verify_proc_mac.cc 
>    M net/cert/cert_verify_proc_nss.cc 
>    M net/cert/cert_verify_proc_openssl.cc 
>    M net/cert/cert_verify_proc_unittest.cc 
>    M net/cert/cert_verify_proc_win.cc 
>    M net/cert/cert_verify_result.h 
>    M net/cert/cert_verify_result.cc 
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Sleevi, 2014-09-29 22:44:56]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2014-09-29 22:46:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/509273002/40001

[commit-bot: I haz the power, 2014-09-29 23:48:11]

Committed patchset #3 (id:40001) as 581cc71be3c77845167f687eed5cbbdeb841cde5

[commit-bot: I haz the power, 2014-09-29 23:49:04]

Patchset 3 (id:??) landed as
https://crrev.com/b92e6f5c2a5271850a39afa1254d41ccc34ec5e6
Cr-Commit-Position: refs/heads/master@{#297307}

[Luke Faraone, 2014-11-03 21:28:21]

On 2014/08/28 04:17:50, Ryan Sleevi (ooo until 11-4) wrote:
> The slight concern (and perhaps you have more state) is ensuring that we don't
> count the trust anchor (which may not be the root), but I don't think
Android's
> cert verification supports this.

I'm pretty sure this code doesn't DTRT; On Ubuntu 14.10 running 39.0.2171.42
beta (64-bit), https://zulip.com/ is marked as using "outdated security
settings", yet SSLLabs indicates[1] we're sending only RSA-2048 certificates
signed with SHA256withRSA.

[1]: https://www.ssllabs.com/ssltest/analyze.html?d=zulip.com&hideResults=on

[Ryan Sleevi, 2014-11-03 22:04:26]

On 2014/11/03 21:28:21, Luke Faraone wrote:
> On 2014/08/28 04:17:50, Ryan Sleevi (ooo until 11-4) wrote:
> > The slight concern (and perhaps you have more state) is ensuring that we
don't
> > count the trust anchor (which may not be the root), but I don't think
> Android's
> > cert verification supports this.
> 
> I'm pretty sure this code doesn't DTRT; On Ubuntu 14.10 running 39.0.2171.42
> beta (64-bit), https://zulip.com/ is marked as using "outdated security
> settings", yet SSLLabs indicates[1] we're sending only RSA-2048 certificates
> signed with SHA256withRSA.
> 
> [1]: https://www.ssllabs.com/ssltest/analyze.html?d=zulip.com&hideResults=on

Please don't use code reviews for bug reports. Please file a new bug.