Reduce Certificate Parsing Strictness

net/cert/internal/parse_certificate.cc

Index: net/cert/internal/parse_certificate.cc
diff --git a/net/cert/internal/parse_certificate.cc b/net/cert/internal/parse_certificate.cc
index 74222e8fde0a2cc5647ede754858044a4bfb1eb8..fbbee49349125e29c611a9d73d9fac64d523780d 100644
--- a/net/cert/internal/parse_certificate.cc
+++ b/net/cert/internal/parse_certificate.cc
@@ -68,43 +68,6 @@ WARN_UNUSED_RESULT bool ParseVersion(const der::Input& in,
   return !parser.HasMore();
 }
 
-// Returns true if the given serial number (CertificateSerialNumber in RFC 5280)
-// is valid:
-//
-//    CertificateSerialNumber  ::=  INTEGER
-//
-// The input to this function is the (unverified) value octets of the INTEGER.
-// This function will verify that:
-//
-//   * The octets are a valid DER-encoding of an INTEGER (for instance, minimal
-//     encoding length).
-//
-//   * No more than 20 octets are used.
-//
-// Note that it DOES NOT reject non-positive values (zero or negative).
-//
-// For reference, here is what RFC 5280 section 4.1.2.2 says:
-//
-//     Given the uniqueness requirements above, serial numbers can be
-//     expected to contain long integers.  Certificate users MUST be able to
-//     handle serialNumber values up to 20 octets.  Conforming CAs MUST NOT
-//     use serialNumber values longer than 20 octets.
-//
-//     Note: Non-conforming CAs may issue certificates with serial numbers
-//     that are negative or zero.  Certificate users SHOULD be prepared to
-//     gracefully handle such certificates.
-WARN_UNUSED_RESULT bool VerifySerialNumber(const der::Input& value) {
-  bool unused_negative;
-  if (!der::IsValidInteger(value, &unused_negative))
-    return false;
-
-  // Check if the serial number is too long per RFC 5280.
-  if (value.Length() > 20)
-    return false;
-
-  return true;
-}
-
 // Consumes a "Time" value (as defined by RFC 5280) from |parser|. On success
 // writes the result to |*out| and returns true. On failure no guarantees are
 // made about the state of |parser|.
@@ -191,6 +154,24 @@ ParsedTbsCertificate::ParsedTbsCertificate() {}
 
 ParsedTbsCertificate::~ParsedTbsCertificate() {}
 
+bool VerifySerialNumber(const der::Input& value) {
+  bool unused_negative;
+  if (!der::IsValidInteger(value, &unused_negative))
+    return false;
+
+  // Allow an extra octet if the first octet is 0, since the serial number may

[eroman, 2016/02/23 22:09:52]

This contradicts the comment in header.

[svaldez, 2016/02/24 16:36:57]

Acknowledged.

+  // not be negative and may require a prefixed 0 octet.

[eroman, 2016/02/23 22:09:52]

The comment should explain why. Maybe more like:
 
RFC 5280 says that serial numbers must be no more than 20 octets long. However a
number of certificates have been issued with 21 octets where the leading octet
is 0.

[svaldez, 2016/02/24 16:36:57]

Done.

+  // TODO: Add warning about non-strict parsing.

[eroman, 2016/02/23 22:09:52]

TODO(svaldez):

[svaldez, 2016/02/24 16:36:57]

Done.

+  if (value.Length() == 21 && value.UnsafeData()[0] == 0)
+    return true;
+
+  // Check if the serial number is too long per RFC 5280.
+  if (value.Length() > 20)
+    return false;
+
+  return true;
+}
+
 bool ParseCertificate(const der::Input& certificate_tlv,
                       ParsedCertificate* out) {
   der::Parser parser(certificate_tlv);