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


Review URL: http://codereview.chromium.org/8381017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@107956 0039d316-1c4b-4281-b951-d872f2087c98

net/base/crl_set.cc

@@ -408,6 +408,18 @@ bool CRLSet::ApplyDelta(base::StringPiece data,
 CRLSet::Result CRLSet::CheckCertificate(
     const base::StringPiece& serial_number,
     const base::StringPiece& parent_spki) const {
+  base::StringPiece serial(serial_number);
+
+  if (!serial.empty() && serial[0] >= 0x80) {
+    // This serial number is negative but the process which generates CRL sets
+    // will reject any certificates with negative serial numbers as invalid.
+    return UNKNOWN;
+  }
+
+  // Remove any leading zero bytes.
+  while (serial.size() > 1 && serial[0] == 0x00)
+    serial.remove_prefix(1);
+
   std::map<std::string, size_t>::const_iterator i =
       crls_index_by_issuer_.find(parent_spki.as_string());
   if (i == crls_index_by_issuer_.end())
@@ -416,7 +428,7 @@ CRLSet::Result CRLSet::CheckCertificate(
 
   for (std::vector<std::string>::const_iterator i = serials.begin();
        i != serials.end(); ++i) {
-    if (base::StringPiece(*i) == serial_number)
+    if (base::StringPiece(*i) == serial)
       return REVOKED;
   }
 

net/base/x509_certificate.h

@@ -433,12 +433,7 @@ class NET_EXPORT X509Certificate
                      CRLSet* crl_set,
                      CertVerifyResult* verify_result) const;
 
-  // The serial number, DER encoded.
-  // NOTE: keep this method private, used by IsBlacklisted only.  To simplify
-  // IsBlacklisted, we strip the leading 0 byte of a serial number, used to
-  // encode a positive DER INTEGER (a signed type) with a most significant bit
-  // of 1.  Other code must not use this method for general purpose until this
-  // is fixed.
+  // The serial number, DER encoded, possibly including a leading 00 byte.
   const std::string& serial_number() const { return serial_number_; }
 
   // IsBlacklisted returns true if this certificate is explicitly blacklisted.

net/base/x509_certificate_mac.cc

@@ -203,10 +203,6 @@ std::string GetCertSerialNumber(X509Certificate::OSCertHandle cert_handle) {
     break;
   }
 
-  // Remove leading zeros.
-  while (ret.size() > 1 && ret[0] == 0)
-    ret = ret.substr(1, ret.size() - 1);
-
   return ret;
 }
 

net/base/x509_certificate_nss.cc

@@ -682,9 +682,6 @@ void X509Certificate::Initialize() {
   serial_number_ = std::string(
       reinterpret_cast<char*>(cert_handle_->serialNumber.data),
       cert_handle_->serialNumber.len);
-  // Remove leading zeros.
-  while (serial_number_.size() > 1 && serial_number_[0] == 0)
-    serial_number_ = serial_number_.substr(1, serial_number_.size() - 1);
 }
 
 // static

net/base/x509_certificate_openssl.cc

@@ -332,9 +332,6 @@ void X509Certificate::Initialize() {
     serial_number_ = std::string(
         reinterpret_cast<char*>(num->data),
         num->length);
-    // Remove leading zeros.
-    while (serial_number_.size() > 1 && serial_number_[0] == 0)
-      serial_number_ = serial_number_.substr(1, serial_number_.size() - 1);
   }
 
   ParsePrincipal(cert_handle_, X509_get_subject_name(cert_handle_), &subject_);

net/base/x509_certificate_unittest.cc

@@ -429,7 +429,7 @@ TEST(X509CertificateTest, SerialNumbers) {
           reinterpret_cast<const char*>(paypal_null_der),
           sizeof(paypal_null_der)));
 
-  static const uint8 paypal_null_serial[2] = {0xf0, 0x9b};
+  static const uint8 paypal_null_serial[3] = {0x00, 0xf0, 0x9b};
   ASSERT_EQ(sizeof(paypal_null_serial),
             paypal_null_cert->serial_number().size());
   EXPECT_TRUE(memcmp(paypal_null_cert->serial_number().data(),

net/base/x509_certificate_win.cc

@@ -552,9 +552,6 @@ void X509Certificate::Initialize() {
     serial_bytes[i] = serial->pbData[serial->cbData - i - 1];
   serial_number_ = std::string(
       reinterpret_cast<char*>(serial_bytes.get()), serial->cbData);
-  // Remove leading zeros.
-  while (serial_number_.size() > 1 && serial_number_[0] == 0)
-    serial_number_ = serial_number_.substr(1, serial_number_.size() - 1);
 }
 
 // IsIssuedByKnownRoot returns true if the given chain is rooted at a root CA