X509CertificateBytes: Allow invalid serial numbers for now.

net/cert/internal/parsed_certificate_unittest.cc

Index: net/cert/internal/parsed_certificate_unittest.cc
diff --git a/net/cert/internal/parsed_certificate_unittest.cc b/net/cert/internal/parsed_certificate_unittest.cc
index db1b0ad845bdce2d76825ca3ef8e2c4913cc673a..bd9cb2cc3489452fa263ddae333ec16b7f7df579 100644
--- a/net/cert/internal/parsed_certificate_unittest.cc
+++ b/net/cert/internal/parsed_certificate_unittest.cc
@@ -24,7 +24,8 @@ std::string GetFilePath(const std::string& file_name) {
 // Returns nullptr if the certificate parsing failed, and verifies that any
 // errors match the ERRORS block in the .pem file.
 scoped_refptr<ParsedCertificate> ParseCertificateFromFile(
-    const std::string& file_name) {
+    const std::string& file_name,
+    const ParseCertificateOptions& options) {
   std::string data;
   std::string expected_errors;
 
@@ -39,7 +40,7 @@ scoped_refptr<ParsedCertificate> ParseCertificateFromFile(
   scoped_refptr<ParsedCertificate> cert = ParsedCertificate::Create(
       bssl::UniquePtr<CRYPTO_BUFFER>(CRYPTO_BUFFER_new(
           reinterpret_cast<const uint8_t*>(data.data()), data.size(), nullptr)),
-      {}, &errors);
+      options, &errors);
 
   EXPECT_EQ(expected_errors, errors.ToDebugString()) << "Test file: "
                                                      << test_file_path;
@@ -63,7 +64,7 @@ der::Input DavidBenOid() {
 // Parses an Extension whose critical field is true (255).
 TEST(ParsedCertificateTest, ExtensionCritical) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("extension_critical.pem");
+      ParseCertificateFromFile("extension_critical.pem", {});
   ASSERT_TRUE(cert);
 
   const uint8_t kExpectedValue[] = {0x30, 0x00};
@@ -79,7 +80,7 @@ TEST(ParsedCertificateTest, ExtensionCritical) {
 // Parses an Extension whose critical field is false (omitted).
 TEST(ParsedCertificateTest, ExtensionNotCritical) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("extension_not_critical.pem");
+      ParseCertificateFromFile("extension_not_critical.pem", {});
   ASSERT_TRUE(cert);
 
   const uint8_t kExpectedValue[] = {0x30, 0x00};
@@ -96,40 +97,42 @@ TEST(ParsedCertificateTest, ExtensionNotCritical) {
 // however because critical has DEFAULT of false this is in fact invalid
 // DER-encoding.
 TEST(ParsedCertificateTest, ExtensionCritical0) {
-  ASSERT_FALSE(ParseCertificateFromFile("extension_critical_0.pem"));
+  ASSERT_FALSE(ParseCertificateFromFile("extension_critical_0.pem", {}));
 }
 
 // Parses an Extension whose critical field is 3. Under DER-encoding BOOLEAN
 // values must an octet of either all zero bits, or all 1 bits, so this is not
 // valid.
 TEST(ParsedCertificateTest, ExtensionCritical3) {
-  ASSERT_FALSE(ParseCertificateFromFile("extension_critical_3.pem"));
+  ASSERT_FALSE(ParseCertificateFromFile("extension_critical_3.pem", {}));
 }
 
 // Parses an Extensions that is an empty sequence.
 TEST(ParsedCertificateTest, ExtensionsEmptySequence) {
-  ASSERT_FALSE(ParseCertificateFromFile("extensions_empty_sequence.pem"));
+  ASSERT_FALSE(ParseCertificateFromFile("extensions_empty_sequence.pem", {}));
 }
 
 // Parses an Extensions that is not a sequence.
 TEST(ParsedCertificateTest, ExtensionsNotSequence) {
-  ASSERT_FALSE(ParseCertificateFromFile("extensions_not_sequence.pem"));
+  ASSERT_FALSE(ParseCertificateFromFile("extensions_not_sequence.pem", {}));
 }
 
 // Parses an Extensions that has data after the sequence.
 TEST(ParsedCertificateTest, ExtensionsDataAfterSequence) {
-  ASSERT_FALSE(ParseCertificateFromFile("extensions_data_after_sequence.pem"));
+  ASSERT_FALSE(
+      ParseCertificateFromFile("extensions_data_after_sequence.pem", {}));
 }
 
 // Parses an Extensions that contains duplicated key usages.
 TEST(ParsedCertificateTest, ExtensionsDuplicateKeyUsage) {
-  ASSERT_FALSE(ParseCertificateFromFile("extensions_duplicate_key_usage.pem"));
+  ASSERT_FALSE(
+      ParseCertificateFromFile("extensions_duplicate_key_usage.pem", {}));
 }
 
 // Parses an Extensions that contains an extended key usages.
 TEST(ParsedCertificateTest, ExtendedKeyUsage) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("extended_key_usage.pem");
+      ParseCertificateFromFile("extended_key_usage.pem", {});
   ASSERT_TRUE(cert);
 
   ASSERT_EQ(4u, cert->extensions().size());
@@ -147,7 +150,7 @@ TEST(ParsedCertificateTest, ExtendedKeyUsage) {
 // Parses an Extensions that contains a key usage.
 TEST(ParsedCertificateTest, KeyUsage) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("key_usage.pem");
+      ParseCertificateFromFile("key_usage.pem", {});
   ASSERT_TRUE(cert);
 
   ASSERT_TRUE(cert->has_key_usage());
@@ -164,7 +167,7 @@ TEST(ParsedCertificateTest, KeyUsage) {
 // Parses an Extensions that contains a policies extension.
 TEST(ParsedCertificateTest, Policies) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("policies.pem");
+      ParseCertificateFromFile("policies.pem", {});
   ASSERT_TRUE(cert);
 
   ASSERT_EQ(4u, cert->extensions().size());
@@ -182,7 +185,7 @@ TEST(ParsedCertificateTest, Policies) {
 // Parses an Extensions that contains a subjectaltname extension.
 TEST(ParsedCertificateTest, SubjectAltName) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("subject_alt_name.pem");
+      ParseCertificateFromFile("subject_alt_name.pem", {});
   ASSERT_TRUE(cert);
 
   ASSERT_TRUE(cert->has_subject_alt_names());
@@ -192,7 +195,7 @@ TEST(ParsedCertificateTest, SubjectAltName) {
 // real-world certificate.
 TEST(ParsedCertificateTest, ExtensionsReal) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("extensions_real.pem");
+      ParseCertificateFromFile("extensions_real.pem", {});
   ASSERT_TRUE(cert);
 
   ASSERT_EQ(7u, cert->extensions().size());
@@ -213,7 +216,7 @@ TEST(ParsedCertificateTest, ExtensionsReal) {
 // Parses a BasicConstraints with no CA or pathlen.
 TEST(ParsedCertificateTest, BasicConstraintsNotCa) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("basic_constraints_not_ca.pem");
+      ParseCertificateFromFile("basic_constraints_not_ca.pem", {});
   ASSERT_TRUE(cert);
 
   EXPECT_TRUE(cert->has_basic_constraints());
@@ -224,7 +227,7 @@ TEST(ParsedCertificateTest, BasicConstraintsNotCa) {
 // Parses a BasicConstraints with CA but no pathlen.
 TEST(ParsedCertificateTest, BasicConstraintsCaNoPath) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("basic_constraints_ca_no_path.pem");
+      ParseCertificateFromFile("basic_constraints_ca_no_path.pem", {});
   ASSERT_TRUE(cert);
 
   EXPECT_TRUE(cert->has_basic_constraints());
@@ -235,7 +238,7 @@ TEST(ParsedCertificateTest, BasicConstraintsCaNoPath) {
 // Parses a BasicConstraints with CA and pathlen of 9.
 TEST(ParsedCertificateTest, BasicConstraintsCaPath9) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("basic_constraints_ca_path_9.pem");
+      ParseCertificateFromFile("basic_constraints_ca_path_9.pem", {});
   ASSERT_TRUE(cert);
 
   EXPECT_TRUE(cert->has_basic_constraints());
@@ -247,7 +250,7 @@ TEST(ParsedCertificateTest, BasicConstraintsCaPath9) {
 // Parses a BasicConstraints with CA and pathlen of 255 (largest allowed size).
 TEST(ParsedCertificateTest, BasicConstraintsPathlen255) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("basic_constraints_pathlen_255.pem");
+      ParseCertificateFromFile("basic_constraints_pathlen_255.pem", {});
   ASSERT_TRUE(cert);
 
   EXPECT_TRUE(cert->has_basic_constraints());
@@ -258,26 +261,28 @@ TEST(ParsedCertificateTest, BasicConstraintsPathlen255) {
 
 // Parses a BasicConstraints with CA and pathlen of 256 (too large).
 TEST(ParsedCertificateTest, BasicConstraintsPathlen256) {
-  ASSERT_FALSE(ParseCertificateFromFile("basic_constraints_pathlen_256.pem"));
+  ASSERT_FALSE(
+      ParseCertificateFromFile("basic_constraints_pathlen_256.pem", {}));
 }
 
 // Parses a BasicConstraints with CA and a negative pathlen.
 TEST(ParsedCertificateTest, BasicConstraintsNegativePath) {
-  ASSERT_FALSE(ParseCertificateFromFile("basic_constraints_negative_path.pem"));
+  ASSERT_FALSE(
+      ParseCertificateFromFile("basic_constraints_negative_path.pem", {}));
 }
 
 // Parses a BasicConstraints with CA and pathlen that is very large (and
 // couldn't fit in a 64-bit integer).
 TEST(ParsedCertificateTest, BasicConstraintsPathTooLarge) {
   ASSERT_FALSE(
-      ParseCertificateFromFile("basic_constraints_path_too_large.pem"));
+      ParseCertificateFromFile("basic_constraints_path_too_large.pem", {}));
 }
 
 // Parses a BasicConstraints with CA explicitly set to false. This violates
 // DER-encoding rules, however is commonly used, so it is accepted.
 TEST(ParsedCertificateTest, BasicConstraintsCaFalse) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("basic_constraints_ca_false.pem");
+      ParseCertificateFromFile("basic_constraints_ca_false.pem", {});
   ASSERT_TRUE(cert);
 
   EXPECT_TRUE(cert->has_basic_constraints());
@@ -289,7 +294,7 @@ TEST(ParsedCertificateTest, BasicConstraintsCaFalse) {
 // the end.
 TEST(ParsedCertificateTest, BasicConstraintsUnconsumedData) {
   ASSERT_FALSE(
-      ParseCertificateFromFile("basic_constraints_unconsumed_data.pem"));
+      ParseCertificateFromFile("basic_constraints_unconsumed_data.pem", {}));
 }
 
 // Parses a BasicConstraints with CA omitted (false), but with a pathlen of 1.
@@ -297,7 +302,7 @@ TEST(ParsedCertificateTest, BasicConstraintsUnconsumedData) {
 // BasicConstraints at a higher level.
 TEST(ParsedCertificateTest, BasicConstraintsPathLenButNotCa) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("basic_constraints_pathlen_not_ca.pem");
+      ParseCertificateFromFile("basic_constraints_pathlen_not_ca.pem", {});
   ASSERT_TRUE(cert);
 
   EXPECT_TRUE(cert->has_basic_constraints());
@@ -310,7 +315,7 @@ TEST(ParsedCertificateTest, BasicConstraintsPathLenButNotCa) {
 // extension having requireExplicitPolicy:3.
 TEST(ParsedCertificateTest, PolicyConstraintsRequire) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("policy_constraints_require.pem");
+      ParseCertificateFromFile("policy_constraints_require.pem", {});
   ASSERT_TRUE(cert);
 
   EXPECT_TRUE(cert->has_policy_constraints());
@@ -324,7 +329,7 @@ TEST(ParsedCertificateTest, PolicyConstraintsRequire) {
 // extension having inhibitPolicyMapping:1.
 TEST(ParsedCertificateTest, PolicyConstraintsInhibit) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("policy_constraints_inhibit.pem");
+      ParseCertificateFromFile("policy_constraints_inhibit.pem", {});
   ASSERT_TRUE(cert);
 
   EXPECT_TRUE(cert->has_policy_constraints());
@@ -338,7 +343,7 @@ TEST(ParsedCertificateTest, PolicyConstraintsInhibit) {
 // extension having requireExplicitPolicy:5,inhibitPolicyMapping:2.
 TEST(ParsedCertificateTest, PolicyConstraintsInhibitRequire) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("policy_constraints_inhibit_require.pem");
+      ParseCertificateFromFile("policy_constraints_inhibit_require.pem", {});
   ASSERT_TRUE(cert);
 
   EXPECT_TRUE(cert->has_policy_constraints());
@@ -352,8 +357,72 @@ TEST(ParsedCertificateTest, PolicyConstraintsInhibitRequire) {
 // extension with an empty sequence.
 TEST(ParsedCertificateTest, PolicyConstraintsEmpty) {
   scoped_refptr<ParsedCertificate> cert =
-      ParseCertificateFromFile("policy_constraints_empty.pem");
+      ParseCertificateFromFile("policy_constraints_empty.pem", {});
+  ASSERT_FALSE(cert);
+}
+
+// Tests a certificate with a serial number with a leading 0 padding byte in
+// the encoding since it is not negative.
+TEST(ParsedCertificateTest, SerialNumberZeroPadded) {
+  scoped_refptr<ParsedCertificate> cert =
+      ParseCertificateFromFile("serial_zero_padded.pem", {});
+  ASSERT_TRUE(cert);
+
+  static const uint8_t expected_serial[3] = {0x00, 0x80, 0x01};
+  EXPECT_EQ(der::Input(expected_serial), cert->tbs().serial_number);
+}
+
+// Tests a serial number where the MSB is >= 0x80, causing the encoded
+// length to be 21 bytes long. This is an error, as RFC 5280 specifies a
+// maximum of 20 bytes.
+TEST(ParsedCertificateTest, SerialNumberZeroPadded21BytesLong) {
+  scoped_refptr<ParsedCertificate> cert =
+      ParseCertificateFromFile("serial_zero_padded_21_bytes.pem", {});
+  ASSERT_FALSE(cert);
+
+  // Try again with allow_invalid_serial_numbers=true. Parsing should succeed.
+  ParseCertificateOptions options;
+  options.allow_invalid_serial_numbers = true;
+  cert = ParseCertificateFromFile("serial_zero_padded_21_bytes.pem", options);
+  ASSERT_TRUE(cert);
+
+  static const uint8_t expected_serial[21] = {
+      0x00, 0x80, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09,
+      0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13};
+  EXPECT_EQ(der::Input(expected_serial), cert->tbs().serial_number);
+}
+
+// Tests a serial number which is negative.  CAs are not supposed to include
+// negative serial numbers, however RFC 5280 expects consumers to deal with it
+// anyway.
+TEST(ParsedCertificateTest, SerialNumberNegative) {
+  scoped_refptr<ParsedCertificate> cert =
+      ParseCertificateFromFile("serial_negative.pem", {});
+  ASSERT_TRUE(cert);
+
+  static const uint8_t expected_serial[2] = {0x80, 0x01};
+  EXPECT_EQ(der::Input(expected_serial), cert->tbs().serial_number);
+}
+
+// Tests a serial number which is very long. RFC 5280 specifies a maximum of 20
+// bytes.
+TEST(ParsedCertificateTest, SerialNumber37BytesLong) {
+  scoped_refptr<ParsedCertificate> cert =
+      ParseCertificateFromFile("serial_37_bytes.pem", {});
   ASSERT_FALSE(cert);
+
+  // Try again with allow_invalid_serial_numbers=true. Parsing should succeed.
+  ParseCertificateOptions options;
+  options.allow_invalid_serial_numbers = true;
+  cert = ParseCertificateFromFile("serial_37_bytes.pem", options);
+  ASSERT_TRUE(cert);
+
+  static const uint8_t expected_serial[37] = {
+      0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a,
+      0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14,
+      0x15, 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e,
+      0x1f, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25};
+  EXPECT_EQ(der::Input(expected_serial), cert->tbs().serial_number);
 }
 
 }  // namespace