Add a function for parsing RFC 5280's "TBSCertificate".

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 c8b58c07606cfcdeb0c1240fc358598d87cc972d..3a04e3ad17977feca5d1756286421d871fb9578b 100644
--- a/net/cert/internal/parse_certificate.cc
+++ b/net/cert/internal/parse_certificate.cc
@@ -28,8 +28,112 @@ WARN_UNUSED_RESULT bool ReadSequenceTLV(der::Parser* parser, der::Input* out) {
   return parser->ReadRawTLV(out) && IsSequenceTLV(*out);
 }
 
+// Parses a Version according to RFC 5280:
+//
+//     Version  ::=  INTEGER  {  v1(0), v2(1), v3(2)  }
+//
+// No value other that v1, v2, or v3 is allowed (and if given will fail). RFC
+// 5280 minimally requires the handling of v3 (and overwhelmingly these are the
+// certificate versions in use today):
+//
+//     Implementations SHOULD be prepared to accept any version certificate.
+//     At a minimum, conforming implementations MUST recognize version 3
+//     certificates.
+WARN_UNUSED_RESULT bool ParseVersion(const der::Input& in,
+                                     CertificateVersion* version) {
+  der::Parser parser(in);
+  uint64_t version64;
+  if (!parser.ReadUint64(&version64))
+    return false;
+
+  switch (version64) {
+    case 0:
+      *version = CertificateVersion::V1;
+      break;
+    case 1:
+      *version = CertificateVersion::V2;
+      break;
+    case 2:
+      *version = CertificateVersion::V3;
+      break;
+    default:
+      // Don't allow any other version identifier.
+      return false;
+  }
+
+  // By definition the input to this function was a single INTEGER, so there
+  // shouldn't be anything else after it.
+  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:

[davidben, 2015/08/14 17:51:42]

Nit: s/here is what // is marginally less verbose.

+//
+//     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) {
+  der::ByteReader reader(value);
+
+  if (value.Length() == 0)
+    return false;  // Not a valid DER-encoded INTEGER.
+
+  // Check if the serial number is too long per RFC 5280.
+  if (value.Length() > 20)
+    return false;
+
+  // Accept any single-byte serial number (including zero and negatives).
+  if (value.Length() == 1)
+    return true;
+
+  // INTEGER values in DER should be minimal. They should only contain a leading
+  // zero if the second octet has its most significant bit set to 1 (since
+  // without the leading zero the described number would be negative).
+  uint8_t first_byte;
+  if (!reader.ReadByte(&first_byte))
+    return false;  // Unexpected
+
+  if (first_byte == 0) {
+    uint8_t second_byte;
+    if (!reader.ReadByte(&second_byte))
+      return false;  // Unexpected
+
+    if ((second_byte & 0x80) == 0)
+      return false;  // MSB must be 1.
+  }

[davidben, 2015/08/14 17:51:42]

If you're allowing negative numbers, probably good to also check they're
minimally-encoded.

If you want something compact, I think this works:

  uint8_t first_byte;
  uint8_t second_byte;
  if (!reader.ReadByte(&first_byte) ||
      !reader.ReadByte(&second_byte) ||
    return false;
  }
  if ((first_byte == 0x00 || first_byte == 0xFF) &&
      (first_byte & 0x80) == (second_byte & 0x80)) {
    return false;
  }

(Or you could write it more explicitly which might easier to understand and less
clever. :-) )

[eroman, 2015/08/14 18:11:59]

Good point, I forgot about that!

This sounds general enough that I should extract to parse_values.h. Perhaps:
   bool der::IsValidInteger(const der::Input& value);

Unless you object I will go ahead and do that, and fix this problem in the
process.

[davidben, 2015/08/14 18:19:22]

SGTM

[eroman, 2015/08/14 21:26:12]

Done. (Part of https://codereview.chromium.org/1295943002/)

+
+  return true;
+}
+
 }  // namespace
 
+ParsedTbsCertificate::ParsedTbsCertificate()
+    : version(CertificateVersion::V1),
+      has_issuer_unique_id(false),
+      has_subject_unique_id(false),
+      has_extensions(false) {}
+
+ParsedTbsCertificate::~ParsedTbsCertificate() {}
+
 bool ParseCertificate(const der::Input& certificate_tlv,
                       ParsedCertificate* out) {
   der::Parser parser(certificate_tlv);
@@ -63,4 +167,141 @@ bool ParseCertificate(const der::Input& certificate_tlv,
   return true;
 }
 
+// From RFC 5280 section 4.1:
+//
+//   TBSCertificate  ::=  SEQUENCE  {
+//        version         [0]  EXPLICIT Version DEFAULT v1,
+//        serialNumber         CertificateSerialNumber,
+//        signature            AlgorithmIdentifier,
+//        issuer               Name,
+//        validity             Validity,
+//        subject              Name,
+//        subjectPublicKeyInfo SubjectPublicKeyInfo,
+//        issuerUniqueID  [1]  IMPLICIT UniqueIdentifier OPTIONAL,
+//                             -- If present, version MUST be v2 or v3
+//        subjectUniqueID [2]  IMPLICIT UniqueIdentifier OPTIONAL,
+//                             -- If present, version MUST be v2 or v3
+//        extensions      [3]  EXPLICIT Extensions OPTIONAL
+//                             -- If present, version MUST be v3
+//        }
+bool ParseTbsCertificate(const der::Input& tbs_tlv, ParsedTbsCertificate* out) {
+  der::Parser parser(tbs_tlv);
+
+  //   Certificate  ::=  SEQUENCE  {
+  der::Parser tbs_parser;
+  if (!parser.ReadSequence(&tbs_parser))
+    return false;
+
+  //        version         [0]  EXPLICIT Version DEFAULT v1,
+  der::Input version;
+  bool has_version;
+  if (!tbs_parser.ReadOptionalTag(der::ContextSpecificConstructed(0), &version,
+                                  &has_version)) {
+    return false;
+  }
+  if (has_version) {
+    if (!ParseVersion(version, &out->version))
+      return false;
+    if (out->version == CertificateVersion::V1) {
+      // The correct way to specify v1 is to omit the version field since v1 is
+      // the DEFAULT.
+      return false;
+    }
+  } else {
+    out->version = CertificateVersion::V1;
+  }
+
+  //        serialNumber         CertificateSerialNumber,
+  if (!tbs_parser.ReadTag(der::kInteger, &out->serial_number))
+    return false;
+  if (!VerifySerialNumber(out->serial_number))
+    return false;
+
+  //        signature            AlgorithmIdentifier,
+  if (!ReadSequenceTLV(&tbs_parser, &out->signature_algorithm_tlv))
+    return false;
+
+  //        issuer               Name,
+  if (!ReadSequenceTLV(&tbs_parser, &out->issuer_tlv))
+    return false;
+
+  //        validity             Validity,
+  if (!ReadSequenceTLV(&tbs_parser, &out->validity_tlv))
+    return false;
+
+  //        subject              Name,
+  if (!ReadSequenceTLV(&tbs_parser, &out->subject_tlv))
+    return false;
+
+  //        subjectPublicKeyInfo SubjectPublicKeyInfo,
+  if (!ReadSequenceTLV(&tbs_parser, &out->spki_tlv))
+    return false;
+
+  //        issuerUniqueID  [1]  IMPLICIT UniqueIdentifier OPTIONAL,
+  //                             -- If present, version MUST be v2 or v3
+  der::Input issuer_unique_id;
+  if (!tbs_parser.ReadOptionalTag(der::ContextSpecificPrimitive(1),
+                                  &issuer_unique_id,
+                                  &out->has_issuer_unique_id)) {
+    return false;
+  }
+  if (out->has_issuer_unique_id) {
+    if (!der::ParseBitString(issuer_unique_id, &out->issuer_unique_id))
+      return false;
+    if (out->version != CertificateVersion::V2 &&
+        out->version != CertificateVersion::V3) {
+      return false;
+    }
+  }
+
+  //        subjectUniqueID [2]  IMPLICIT UniqueIdentifier OPTIONAL,
+  //                             -- If present, version MUST be v2 or v3
+  der::Input subject_unique_id;
+  if (!tbs_parser.ReadOptionalTag(der::ContextSpecificPrimitive(2),
+                                  &subject_unique_id,
+                                  &out->has_subject_unique_id)) {
+    return false;
+  }
+  if (out->has_subject_unique_id) {
+    if (!der::ParseBitString(subject_unique_id, &out->subject_unique_id))
+      return false;
+    if (out->version != CertificateVersion::V2 &&
+        out->version != CertificateVersion::V3) {
+      return false;
+    }
+  }
+
+  //        extensions      [3]  EXPLICIT Extensions OPTIONAL
+  //                             -- If present, version MUST be v3
+  if (!tbs_parser.ReadOptionalTag(der::ContextSpecificConstructed(3),
+                                  &out->extensions_tlv, &out->has_extensions)) {
+    return false;
+  }
+  if (out->has_extensions) {
+    // ParseTbsCertificate()'s contract guarantees that extensions is a
+    // SEQUENCE.

[davidben, 2015/08/14 17:51:42]

Maybe:

  // extensions_tlv must be a single element. Also check that it is a SEQUENCE.

Specifically to call attention that the SEQUENCE check isn't the only thing
here; you need to check that there's a single TLV in it, even if you didn't
check the tag.

[eroman, 2015/08/14 21:26:12]

Done.

+    if (!IsSequenceTLV(out->extensions_tlv))
+      return false;
+    if (out->version != CertificateVersion::V3)
+      return false;
+  }
+
+  // Note that there IS an extension point at the end of TBSCertificate
+  // (according to RFC 5912), so from that interpretation, unconsumed data would
+  // be allowed in |tbs_parser|.
+  //
+  // 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())
+    return false;
+
+  // By definition the input was a single TBSCertificate, so there shouldn't be
+  // unconsumed data.
+  if (parser.HasMore())
+    return false;
+
+  return true;
+}
+
 }  // namespace net