net: retain leading zero bytes in X.509 serial numbers.

net: retain leading zero bytes in X.509 serial numbers.

X.509 serial numbers should be a positive numbers according to the spec.
However, certificates have been issued with negative serial numbers. Negative
serial numbers are indicated with a most-significant bit of one. Positive
numbers which would have a MSB of 1 have a zero byte prepended to avoid the
ambiguity.

Previously we removing leading zero bytes because we were only matching against
a blacklist of serial numbers, none of which were negative.

This change moves the handling of serial numbers to the place where they are
used, rather than where they are parsed.

BUG=none
TEST=none


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

[agl, 2011-10-24 20:03:09]

This is the result of a comment on http://codereview.chromium.org/8342054 (CRL
sets) which ended up needing its own CL.

I've also removed a large number of DigiNotar serial numbers because we have
blocked their public keys now.

[wtc, 2011-10-25 01:26:16]

agl: is this CL ready for review?

[agl, 2011-10-25 01:43:16]

Yes, but I appear to have forgotten to set you as a reviewer! I'll correct
that tomorrow. Sorry about that.
On Oct 24, 2011 9:26 PM, <wtc@chromium.org> wrote:

> agl: is this CL ready for review?
>
>
http://codereview.chromium.**org/8381017/<http://codereview.chromium.org/8381...
>

[wtc, 2011-10-25 21:14:44]

Patch Set 2 LGTM.

Are the serial numbers in CRL sets the original serial
numbers, or the leading-0-byte-stripped serial numbers?

http://codereview.chromium.org/8381017/diff/1001/net/base/crl_set.cc
File net/base/crl_set.cc (right):

http://codereview.chromium.org/8381017/diff/1001/net/base/crl_set.cc#newcode420
net/base/crl_set.cc:420: while (!serial.empty() && serial[0] == 0x00)

BUG(?): !serial.empty() => serial.size() > 1
to avoid ending up with an empty string.

http://codereview.chromium.org/8381017/diff/1001/net/base/crl_set.cc#newcode431
net/base/crl_set.cc:431: if (base::StringPiece(*i) == serial_number)

BUG: serial_number should be changed to |serial| here, right?
Otherwise |serial| is not used.

NOTE: I prefer that CRL sets contain the original serial
numbers that may contain leading 0 bytes.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (left):

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:947: // Special case for DigiNotar: this serial
number had a leading 0x00 byte

Just wanted to confirm that you meant to delete this blacklisted
cert.

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

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:695: return false;

Should we reject a negative serial number by returning true
(meaning it is blacklisted) instead?  That seems too harsh.

If we return false here, saying "we reject this now" is a
little confusing because returning false means allowing it.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:700: while (!serial.empty() && serial[0] == 0) {

BUG(?): !serial.empty() => serial.size() > 1
to avoid ending up with an empty string.

Unless you are doing this intentionally, because an empty
|serial| is harmless here.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:702: }

Nit: omit the curly braces.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:421: // The serial number, DER encoded, possibly
including leading 00 bytes.

Nit: "a leading 00 byte" might be better because one leading
zero byte is enough to make the INTEGER positive.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:422: // NOTE: keep this method private, used by
IsBlacklisted only.

We can remove this NOTE now (i.e., this method can become
public if necessary) because it preserves the leading 0 bytes
required for DER encoding of ASN.1 INTEGER.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:429: static const uint8
paypal_null_serial[3] = {0x00, 0xf0, 0x9b};

I was going to suggest that we add a unit test for this.
Good!

[agl, 2011-10-28 20:29:06]

Thanks. Not submitting yet because it's late on a Friday.

http://codereview.chromium.org/8381017/diff/1001/net/base/crl_set.cc
File net/base/crl_set.cc (right):

http://codereview.chromium.org/8381017/diff/1001/net/base/crl_set.cc#newcode420
net/base/crl_set.cc:420: while (!serial.empty() && serial[0] == 0x00)
On 2011/10/25 21:14:44, wtc wrote:
> 
> BUG(?): !serial.empty() => serial.size() > 1
> to avoid ending up with an empty string.

Done.

http://codereview.chromium.org/8381017/diff/1001/net/base/crl_set.cc#newcode431
net/base/crl_set.cc:431: if (base::StringPiece(*i) == serial_number)
On 2011/10/25 21:14:44, wtc wrote:
> BUG: serial_number should be changed to |serial| here, right?
> Otherwise |serial| is not used.

Yes, thank you.

> NOTE: I prefer that CRL sets contain the original serial
> numbers that may contain leading 0 bytes.

They don't for size reasons. Negative serial numbers are rejected at creation
time.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (left):

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:947: // Special case for DigiNotar: this serial
number had a leading 0x00 byte
On 2011/10/25 21:14:44, wtc wrote:
> 
> Just wanted to confirm that you meant to delete this blacklisted
> cert.

Yep.

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

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:695: return false;
On 2011/10/25 21:14:44, wtc wrote:
> Should we reject a negative serial number by returning true
> (meaning it is blacklisted) instead?  That seems too harsh.

I would like to, but I think we have to pick our battles and I'm not sure that
one is worth fighting over.

> If we return false here, saying "we reject this now" is a
> little confusing because returning false means allowing it.

Yes, fair point. Reworded.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:700: while (!serial.empty() && serial[0] == 0) {
On 2011/10/25 21:14:44, wtc wrote:
> 
> BUG(?): !serial.empty() => serial.size() > 1
> to avoid ending up with an empty string.
> 
> Unless you are doing this intentionally, because an empty
> |serial| is harmless here.

Done.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:702: }
On 2011/10/25 21:14:44, wtc wrote:
> 
> Nit: omit the curly braces.

Done.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:421: // The serial number, DER encoded, possibly
including leading 00 bytes.
On 2011/10/25 21:14:44, wtc wrote:
> 
> Nit: "a leading 00 byte" might be better because one leading
> zero byte is enough to make the INTEGER positive.

Done.

http://codereview.chromium.org/8381017/diff/1001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:422: // NOTE: keep this method private, used by
IsBlacklisted only.
On 2011/10/25 21:14:44, wtc wrote:
> 
> We can remove this NOTE now (i.e., this method can become
> public if necessary) because it preserves the leading 0 bytes
> required for DER encoding of ASN.1 INTEGER.

Done.

[wtc, 2011-10-28 20:53:06]

Patch Set 3 LGTM.

[commit-bot: I haz the power, 2011-10-31 14:11:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/8381017/4003

[commit-bot: I haz the power, 2011-10-31 15:16:57]

Change committed as 107956