Add error details to TBSCertificate parsing function and tests.

BUG=634443

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

net/cert/internal/parse_certificate.cc

@@ -30,6 +30,9 @@ DEFINE_CERT_ERROR_ID(
 DEFINE_CERT_ERROR_ID(kSignatureValueNotBitString,
                      "Couldn't read Certificate.signatureValue as BIT STRING");
 
+DEFINE_CERT_ERROR_ID(kUnconsumedDataInsideTbsCertificateSequence,
+                     "Unconsumed data inside TBSCertificate");
+
 // Returns true if |input| is a SEQUENCE and nothing else.
 WARN_UNUSED_RESULT bool IsSequenceTLV(const der::Input& input) {
   der::Parser parser(input);
@@ -258,7 +261,16 @@ bool ParseCertificate(const der::Input& certificate_tlv,
 //        }
 bool ParseTbsCertificate(const der::Input& tbs_tlv,
                          const ParseCertificateOptions& options,
-                         ParsedTbsCertificate* out) {
+                         ParsedTbsCertificate* out,
+                         CertErrors* errors) {
+  // The rest of this function assumes that |errors| is non-null.
+  if (!errors) {
+    CertErrors unused_errors;
+    return ParseTbsCertificate(tbs_tlv, options, out, &unused_errors);
+  }
+
+  // TODO(crbug.com/634443): Add useful error information to |errors|.
+
   der::Parser parser(tbs_tlv);
 
   //   Certificate  ::=  SEQUENCE  {
@@ -374,8 +386,10 @@ bool ParseTbsCertificate(const der::Input& tbs_tlv,
   // However because only v1, v2, and v3 certificates are supported by the
   // parsing, there shouldn't be any subsequent data in those versions, so
   // reject.
-  if (tbs_parser.HasMore())
+  if (tbs_parser.HasMore()) {
+    errors->AddError(kUnconsumedDataInsideTbsCertificateSequence);
     return false;
+  }
 
   // By definition the input was a single TBSCertificate, so there shouldn't be
   // unconsumed data.

net/cert/internal/parse_certificate.h

@@ -102,6 +102,9 @@ NET_EXPORT bool ParseCertificate(const der::Input& certificate_tlv,
 // on success and sets the results in |out|. Certain invalid inputs may
 // be accepted based on the provided |options|.
 //
+// If |errors| was non-null then any warnings/errors that occur during parsing
+// are added to it.
+//
 // Note that on success |out| aliases data from the input |tbs_tlv|.
 // Hence the fields of the ParsedTbsCertificate are only valid as long as
 // |tbs_tlv| remains valid.
@@ -129,8 +132,8 @@ NET_EXPORT bool ParseCertificate(const der::Input& certificate_tlv,
 //            }
 NET_EXPORT bool ParseTbsCertificate(const der::Input& tbs_tlv,
                                     const ParseCertificateOptions& options,
-                                    ParsedTbsCertificate* out)
-    WARN_UNUSED_RESULT;
+                                    ParsedTbsCertificate* out,
+                                    CertErrors* errors) WARN_UNUSED_RESULT;
 
 // Represents a "Version" from RFC 5280:
 //         Version  ::=  INTEGER  {  v1(0), v2(1), v3(2)  }

net/cert/internal/parse_certificate_fuzzer.cc

@@ -41,7 +41,7 @@ void ParseCertificateForFuzzer(const der::Input& in) {
       SignatureAlgorithm::CreateFromDer(signature_algorithm_tlv));
 
   ParsedTbsCertificate tbs;
-  if (!ParseTbsCertificate(tbs_certificate_tlv, {}, &tbs))
+  if (!ParseTbsCertificate(tbs_certificate_tlv, {}, &tbs, &errors))
     return;
 
   RDNSequence subject;

net/cert/internal/parse_ocsp.cc

@@ -6,6 +6,7 @@
 
 #include "base/sha1.h"
 #include "crypto/sha2.h"
+#include "net/cert/internal/cert_errors.h"
 #include "net/cert/internal/parse_ocsp.h"
 #include "net/der/encode_values.h"
 
@@ -500,10 +501,13 @@ bool GetOCSPCertStatus(const OCSPResponseData& response_data,
   out->status = OCSPRevocationStatus::GOOD;
 
   ParsedTbsCertificate tbs_cert;
-  if (!ParseTbsCertificate(cert_tbs_certificate_tlv, {}, &tbs_cert))
+  // TODO(crbug.com/634443): Propagate the errors.
+  CertErrors errors;
+  if (!ParseTbsCertificate(cert_tbs_certificate_tlv, {}, &tbs_cert, &errors))
     return false;
   ParsedTbsCertificate issuer_tbs_cert;
-  if (!ParseTbsCertificate(issuer_tbs_certificate_tlv, {}, &issuer_tbs_cert))
+  if (!ParseTbsCertificate(issuer_tbs_certificate_tlv, {}, &issuer_tbs_cert,
+                           &errors))
     return false;
 
   bool found = false;

net/cert/internal/parsed_certificate.cc

@@ -98,8 +98,8 @@ scoped_refptr<ParsedCertificate> ParsedCertificate::CreateInternal(
     return nullptr;
   }
 
-  if (!ParseTbsCertificate(result->tbs_certificate_tlv_, options,
-                           &result->tbs_)) {
+  if (!ParseTbsCertificate(result->tbs_certificate_tlv_, options, &result->tbs_,
+                           errors)) {
     return nullptr;
   }
 

net/cert/x509_util.cc

@@ -133,7 +133,7 @@ bool ParseCertificateSandboxed(const base::StringPiece& certificate,
 
   ParsedTbsCertificate parsed_tbs_cert;
   if (!ParseTbsCertificate(tbs_cert, ParseCertificateOptions(),
-                           &parsed_tbs_cert))
+                           &parsed_tbs_cert, nullptr))
     return false;
 
   if (!GetCommonName(parsed_tbs_cert.subject_tlv, subject))

net/data/parse_certificate_unittest/tbs_v3_data_after_extensions.pem

@@ -26,3 +26,9 @@ $ openssl asn1parse -i < [TBS CERTIFICATE]
 MEWgAwIBAgIBATADBAEBMAMEAQUwHhcNMTIxMDE4MDMxMjAwWhcNMTMxMDE4MTQ1OTU5WjADBAG
 DMAMEAfOjBTADBAHdBQA=
 -----END TBS CERTIFICATE-----
+
+[Error] Unconsumed data inside TBSCertificate
+
+-----BEGIN ERRORS-----
+W0Vycm9yXSBVbmNvbnN1bWVkIGRhdGEgaW5zaWRlIFRCU0NlcnRpZmljYXRlCg==
+-----END ERRORS-----