Add error details to ParseCertificate test data.

BUG=634443

Review-Url: https://codereview.chromium.org/2337373003
Cr-Commit-Position: refs/heads/master@{#419346}

net/cert/internal/parse_certificate.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/strings/string_util.h"
+#include "net/cert/internal/cert_errors.h"
 #include "net/der/input.h"
 #include "net/der/parse_values.h"
 #include "net/der/parser.h"
@@ -15,6 +16,20 @@ namespace net {
 
 namespace {
 
+DEFINE_CERT_ERROR_ID(kCertificateNotSequence,
+                     "Failed parsing Certificate SEQUENCE");
+DEFINE_CERT_ERROR_ID(kUnconsumedDataInsideCertificateSequence,
+                     "Unconsumed data inside Certificate SEQUENCE");
+DEFINE_CERT_ERROR_ID(kUnconsumedDataAfterCertificateSequence,
+                     "Unconsumed data after Certificate SEQUENCE");
+DEFINE_CERT_ERROR_ID(kTbsCertificateNotSequence,
+                     "Couldn't read tbsCertificate as SEQUENCE");
+DEFINE_CERT_ERROR_ID(
+    kSignatureAlgorithmNotSequence,
+    "Couldn't read Certificate.signatureAlgorithm as SEQUENCE");
+DEFINE_CERT_ERROR_ID(kSignatureValueNotBitString,
+                     "Couldn't read Certificate.signatureValue as BIT STRING");
+
 // Returns true if |input| is a SEQUENCE and nothing else.
 WARN_UNUSED_RESULT bool IsSequenceTLV(const der::Input& input) {
   der::Parser parser(input);
@@ -172,35 +187,54 @@ bool ParseCertificate(const der::Input& certificate_tlv,
                       der::Input* out_signature_algorithm_tlv,
                       der::BitString* out_signature_value,
                       CertErrors* out_errors) {
-  // TODO(crbug.com/634443): Fill |out_errors| (which may be null) with error
-  // information.
+  // |out_errors| is optional. But ensure it is non-null for the remainder of
+  // this function.
+  if (!out_errors) {
+    CertErrors unused_errors;
+    return ParseCertificate(certificate_tlv, out_tbs_certificate_tlv,
+                            out_signature_algorithm_tlv, out_signature_value,
+                            &unused_errors);
+  }
+
   der::Parser parser(certificate_tlv);
 
   //   Certificate  ::=  SEQUENCE  {
   der::Parser certificate_parser;
-  if (!parser.ReadSequence(&certificate_parser))
+  if (!parser.ReadSequence(&certificate_parser)) {
+    out_errors->AddError(kCertificateNotSequence);
     return false;
+  }
 
   //        tbsCertificate       TBSCertificate,
-  if (!ReadSequenceTLV(&certificate_parser, out_tbs_certificate_tlv))
+  if (!ReadSequenceTLV(&certificate_parser, out_tbs_certificate_tlv)) {
+    out_errors->AddError(kTbsCertificateNotSequence);
     return false;
+  }
 
   //        signatureAlgorithm   AlgorithmIdentifier,
-  if (!ReadSequenceTLV(&certificate_parser, out_signature_algorithm_tlv))
+  if (!ReadSequenceTLV(&certificate_parser, out_signature_algorithm_tlv)) {
+    out_errors->AddError(kSignatureAlgorithmNotSequence);
     return false;
+  }
 
   //        signatureValue       BIT STRING  }
-  if (!certificate_parser.ReadBitString(out_signature_value))
+  if (!certificate_parser.ReadBitString(out_signature_value)) {
+    out_errors->AddError(kSignatureValueNotBitString);
     return false;
+  }
 
   // There isn't an extension point at the end of Certificate.
-  if (certificate_parser.HasMore())
+  if (certificate_parser.HasMore()) {
+    out_errors->AddError(kUnconsumedDataInsideCertificateSequence);
     return false;
+  }
 
   // By definition the input was a single Certificate, so there shouldn't be
   // unconsumed data.
-  if (parser.HasMore())
+  if (parser.HasMore()) {
+    out_errors->AddError(kUnconsumedDataAfterCertificateSequence);
     return false;
+  }
 
   return true;
 }

net/cert/internal/parse_certificate_unittest.cc

@@ -28,10 +28,12 @@ std::string GetFilePath(const std::string& file_name) {
 }
 
 // Loads certificate data and expectations from the PEM file |file_name|.
-// Verifies that parsing the Certificate succeeds, and each parsed field matches
-// the expectations.
-void EnsureParsingCertificateSucceeds(const std::string& file_name) {
+// Verifies that parsing the Certificate matches expectations:
+//   * If expected to fail, emits the expected errors
+//   * If expected to succeeds, the parsed fields match expectations
+void RunCertificateTest(const std::string& file_name) {
   std::string data;
+  std::string expected_errors;
   std::string expected_tbs_certificate;
   std::string expected_signature_algorithm;
   std::string expected_signature;
@@ -39,92 +41,82 @@ void EnsureParsingCertificateSucceeds(const std::string& file_name) {
   // Read the certificate data and test expectations from a single PEM file.
   const PemBlockMapping mappings[] = {
       {"CERTIFICATE", &data},
-      {"SIGNATURE", &expected_signature},
-      {"SIGNATURE ALGORITHM", &expected_signature_algorithm},
-      {"TBS CERTIFICATE", &expected_tbs_certificate},
-  };
-  ASSERT_TRUE(ReadTestDataFromPemFile(GetFilePath(file_name), mappings));
-
-  // Parsing the certificate should succeed.
-  der::Input tbs_certificate_tlv;
-  der::Input signature_algorithm_tlv;
-  der::BitString signature_value;
-  ASSERT_TRUE(ParseCertificate(der::Input(&data), &tbs_certificate_tlv,
-                               &signature_algorithm_tlv, &signature_value,
-                               nullptr));
-
-  // Ensure that the parsed certificate matches expectations.
-  EXPECT_EQ(0, signature_value.unused_bits());
-  EXPECT_EQ(der::Input(&expected_signature), signature_value.bytes());
-  EXPECT_EQ(der::Input(&expected_signature_algorithm), signature_algorithm_tlv);
-  EXPECT_EQ(der::Input(&expected_tbs_certificate), tbs_certificate_tlv);
-}
-
-// Loads certificate data from the PEM file |file_name| and verifies that the
-// Certificate parsing fails.
-void EnsureParsingCertificateFails(const std::string& file_name) {
-  std::string data;
-
-  const PemBlockMapping mappings[] = {
-      {"CERTIFICATE", &data},
+      {"ERRORS", &expected_errors, true /*optional*/},
+      {"SIGNATURE", &expected_signature, true /*optional*/},
+      {"SIGNATURE ALGORITHM", &expected_signature_algorithm, true /*optional*/},
+      {"TBS CERTIFICATE", &expected_tbs_certificate, true /*optional*/},
   };
+  std::string test_file_path = GetFilePath(file_name);
+  ASSERT_TRUE(ReadTestDataFromPemFile(test_file_path, mappings));
 
-  ASSERT_TRUE(ReadTestDataFromPemFile(GetFilePath(file_name), mappings));
+  // Note that empty expected_errors doesn't necessarily mean success.
+  bool expected_result = !expected_tbs_certificate.empty();
 
-  // Parsing the Certificate should fail.
+  // Parsing the certificate.
   der::Input tbs_certificate_tlv;
   der::Input signature_algorithm_tlv;
   der::BitString signature_value;
   CertErrors errors;
-  ASSERT_FALSE(ParseCertificate(der::Input(&data), &tbs_certificate_tlv,
-                                &signature_algorithm_tlv, &signature_value,
-                                &errors));
-  // TODO(crbug.com/634443): Verify |errors| to make sure it failed for the
-  //                         expected reason.
+  bool actual_result =
+      ParseCertificate(der::Input(&data), &tbs_certificate_tlv,
+                       &signature_algorithm_tlv, &signature_value, &errors);
+
+  EXPECT_EQ(expected_result, actual_result);
+  EXPECT_EQ(expected_errors, errors.ToDebugString()) << "Test file: "
+                                                     << test_file_path;
+
+  // Ensure that the parsed certificate matches expectations.
+  if (expected_result && actual_result) {
+    EXPECT_EQ(0, signature_value.unused_bits());
+    EXPECT_EQ(der::Input(&expected_signature), signature_value.bytes());
+    EXPECT_EQ(der::Input(&expected_signature_algorithm),
+              signature_algorithm_tlv);
+    EXPECT_EQ(der::Input(&expected_tbs_certificate), tbs_certificate_tlv);
+  }
 }
 
 // Tests parsing a Certificate.
 TEST(ParseCertificateTest, Version3) {
-  EnsureParsingCertificateSucceeds("cert_version3.pem");
+  RunCertificateTest("cert_version3.pem");
 }
 
 // Tests parsing a simplified Certificate-like structure (the sub-fields for
 // algorithm and tbsCertificate are not actually valid, but ParseCertificate()
 // doesn't check them)
 TEST(ParseCertificateTest, Skeleton) {
-  EnsureParsingCertificateSucceeds("cert_skeleton.pem");
+  RunCertificateTest("cert_skeleton.pem");
 }
 
 // Tests parsing a Certificate that is not a sequence fails.
 TEST(ParseCertificateTest, NotSequence) {
-  EnsureParsingCertificateFails("cert_not_sequence.pem");
+  RunCertificateTest("cert_not_sequence.pem");
 }
 
 // Tests that uncomsumed data is not allowed after the main SEQUENCE.
 TEST(ParseCertificateTest, DataAfterSignature) {
-  EnsureParsingCertificateFails("cert_data_after_signature.pem");
+  RunCertificateTest("cert_data_after_signature.pem");
 }
 
 // Tests that parsing fails if the signature BIT STRING is missing.
 TEST(ParseCertificateTest, MissingSignature) {
-  EnsureParsingCertificateFails("cert_missing_signature.pem");
+  RunCertificateTest("cert_missing_signature.pem");
 }
 
 // Tests that parsing fails if the signature is present but not a BIT STRING.
 TEST(ParseCertificateTest, SignatureNotBitString) {
-  EnsureParsingCertificateFails("cert_signature_not_bit_string.pem");
+  RunCertificateTest("cert_signature_not_bit_string.pem");
 }
 
 // Tests that parsing fails if the main SEQUENCE is empty (missing all the
 // fields).
 TEST(ParseCertificateTest, EmptySequence) {
-  EnsureParsingCertificateFails("cert_empty_sequence.pem");
+  RunCertificateTest("cert_empty_sequence.pem");
 }
 
 // Tests what happens when the signature algorithm is present, but has the wrong
 // tag.
 TEST(ParseCertificateTest, AlgorithmNotSequence) {
-  EnsureParsingCertificateFails("cert_algorithm_not_sequence.pem");
+  RunCertificateTest("cert_algorithm_not_sequence.pem");
 }
 
 // Loads tbsCertificate data and expectations from the PEM file |file_name|.

net/data/parse_certificate_unittest/cert_algorithm_not_sequence.pem

@@ -11,3 +11,9 @@ $ openssl asn1parse -i < [CERTIFICATE]
 -----BEGIN CERTIFICATE-----
 MAwwAgUAAgIFAAMCAKs=
 -----END CERTIFICATE-----
+
+[Error] Couldn't read Certificate.signatureAlgorithm as SEQUENCE
+
+-----BEGIN ERRORS-----
+W0Vycm9yXSBDb3VsZG4ndCByZWFkIENlcnRpZmljYXRlLnNpZ25hdHVyZUFsZ29yaXRobSBhcyBTRVFVRU5DRQo=
+-----END ERRORS-----

net/data/parse_certificate_unittest/cert_data_after_signature.pem

@@ -13,3 +13,9 @@ $ openssl asn1parse -i < [CERTIFICATE]
 -----BEGIN CERTIFICATE-----
 MA4wAgUAMAIFAAMCAKwFAA==
 -----END CERTIFICATE-----
+
+[Error] Unconsumed data inside Certificate SEQUENCE
+
+-----BEGIN ERRORS-----
+W0Vycm9yXSBVbmNvbnN1bWVkIGRhdGEgaW5zaWRlIENlcnRpZmljYXRlIFNFUVVFTkNFCg==
+-----END ERRORS-----

net/data/parse_certificate_unittest/cert_empty_sequence.pem

@@ -7,3 +7,9 @@ $ openssl asn1parse -i < [CERTIFICATE]
 -----BEGIN CERTIFICATE-----
 MAA=
 -----END CERTIFICATE-----
+
+[Error] Couldn't read tbsCertificate as SEQUENCE
+
+-----BEGIN ERRORS-----
+W0Vycm9yXSBDb3VsZG4ndCByZWFkIHRic0NlcnRpZmljYXRlIGFzIFNFUVVFTkNFCg==
+-----END ERRORS-----

net/data/parse_certificate_unittest/cert_missing_signature.pem

@@ -11,3 +11,9 @@ $ openssl asn1parse -i < [CERTIFICATE]
 -----BEGIN CERTIFICATE-----
 MAgwAgUAMAIFAA==
 -----END CERTIFICATE-----
+
+[Error] Couldn't read Certificate.signatureValue as BIT STRING
+
+-----BEGIN ERRORS-----
+W0Vycm9yXSBDb3VsZG4ndCByZWFkIENlcnRpZmljYXRlLnNpZ25hdHVyZVZhbHVlIGFzIEJJVCBTVFJJTkcK
+-----END ERRORS-----

net/data/parse_certificate_unittest/cert_not_sequence.pem

@@ -7,3 +7,9 @@ $ openssl asn1parse -i < [CERTIFICATE]
 -----BEGIN CERTIFICATE-----
 AhAwBgUAMAIFADACBQADAgCs
 -----END CERTIFICATE-----
+
+[Error] Failed parsing Certificate SEQUENCE
+
+-----BEGIN ERRORS-----
+W0Vycm9yXSBGYWlsZWQgcGFyc2luZyBDZXJ0aWZpY2F0ZSBTRVFVRU5DRQo=
+-----END ERRORS-----

net/data/parse_certificate_unittest/cert_signature_not_bit_string.pem

@@ -12,3 +12,9 @@ $ openssl asn1parse -i < [CERTIFICATE]
 -----BEGIN CERTIFICATE-----
 MAwwAgUAMAIFAAQCAQI=
 -----END CERTIFICATE-----
+
+[Error] Couldn't read Certificate.signatureValue as BIT STRING
+
+-----BEGIN ERRORS-----
+W0Vycm9yXSBDb3VsZG4ndCByZWFkIENlcnRpZmljYXRlLnNpZ25hdHVyZVZhbHVlIGFzIEJJVCBTVFJJTkcK
+-----END ERRORS-----

net/data/verify_certificate_chain_unittest/rebase-errors.py

@@ -93,11 +93,10 @@ def fixup_pem_file(path, actual_errors):
   m = errors_block_regex.search(contents)
 
   if not m:
-    print "Couldn't find ERRORS block in %s" % (path)
-    return
-
-  contents = replace_string(contents, m.start(1), m.end(1),
-                            common.text_data_to_pem('ERRORS', actual_errors))
+    contents += '\n' + common.text_data_to_pem('ERRORS', actual_errors)
+  else:
+    contents = replace_string(contents, m.start(1), m.end(1),
+                              common.text_data_to_pem('ERRORS', actual_errors))
 
   # Update the file.
   write_string_to_file(contents, path)