net: block bad DigiNotar serial numbers and several intermediates.

net: block bad DigiNotar serial numbers and several intermediates.

BUG=94673
TEST=None. No sites using these certs are known.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98980

[agl, 2011-08-30 17:34:17]

cevans: wtc is ill today and may not be able to do the review so, if you get to
it first, would you mind?

[wtc, 2011-08-30 17:47:20]

agl: I'll let cevans do the primary review.  I think this
CL will do what you intend to do.  I asked for more comments
below.

http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:684: // Bad DigiNotar leaf certificates for
non-Google sites.
Can you document how this list is generated?
Expiration info such as line 650 is also useful to let
future maintainers know when this list can be safely removed.

http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:959: 0x24, 0x80, 0x50, 0xeb, 0xef, 0xc6, 0x27,
0xbe, 0x4c, 0xa6},
I assume these two are intermediate CA certificates.  It would
be nice to document what their issuers are (i.e., use the
issuer name + serial number to uniquely identify them).

The reason is that our public_key_hashes are calculated in a
"non-standard" way (covering the entire SubjectPublicKeyInfo
structure rather than just the public key), so it is not easy
to know which certs these are given these hashes.

[Chris Evans, 2011-08-30 17:48:44]

LGTM modulo wtc's comments.
I checked the numbers add up and we discussed the performance implications over
chat (which are OK to land as-is for now).

On 2011/08/30 17:47:20, wtc wrote:
> agl: I'll let cevans do the primary review.  I think this
> CL will do what you intend to do.  I asked for more comments
> below.
> 
> http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc
> File net/base/x509_certificate.cc (right):
> 
>
http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc...
> net/base/x509_certificate.cc:684: // Bad DigiNotar leaf certificates for
> non-Google sites.
> Can you document how this list is generated?
> Expiration info such as line 650 is also useful to let
> future maintainers know when this list can be safely removed.
> 
>
http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc...
> net/base/x509_certificate.cc:959: 0x24, 0x80, 0x50, 0xeb, 0xef, 0xc6, 0x27,
> 0xbe, 0x4c, 0xa6},
> I assume these two are intermediate CA certificates.  It would
> be nice to document what their issuers are (i.e., use the
> issuer name + serial number to uniquely identify them).
> 
> The reason is that our public_key_hashes are calculated in a
> "non-standard" way (covering the entire SubjectPublicKeyInfo
> structure rather than just the public key), so it is not easy
> to know which certs these are given these hashes.

[wtc, 2011-08-30 18:13:49]

http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:838:
{0x00,0x17,0x7f,0xb6,0x53,0x6b,0x98,0xce,0x40,0xd5,0x4b,0x8b,0x24,0xe3,0x16,0x05},
This serial number starts with 0x00.

Our serial number parsing code uses a while loop to skip all
leading 0x00 bytes.

It may be easier to handle this serial number as a special
case.

[phshah, 2011-09-01 20:05:48]

http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:644: static const unsigned kNumSerials = 257;
This should be 256, since DigiNotar is being special cased.

[wtc, 2011-09-01 21:16:05]

http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/7791032/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:644: static const unsigned kNumSerials = 257;
On 2011/09/01 20:05:48, phshah wrote:
> This should be 256, since DigiNotar is being special cased.

You are right.  I wrote a new CL to fix this:
http://codereview.chromium.org/7792077/

Thanks.