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

net/cert/internal/parse_certificate.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/cert/internal/parse_certificate.h"
+
+#include "net/der/input.h"
+#include "net/der/parse_values.h"
+#include "net/der/parser.h"
+
+namespace net {
+
+namespace {
+
+// 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):

[davidben, 2015/08/11 20:31:56]

Do we need to handle this unspecified v4 thing?

[eroman, 2015/08/11 21:13:33]

From what I can tell v4 is not really a thing yet (at least it is not present in
the CT database).
But I will defer to you and Ryan if we need to add support.

If we do want to add it, I think it would best be added as a follow-up
iteration, since I presume it will change the parsing requirements.

[davidben, 2015/08/11 22:39:20]

I know nothing about what v4 is. I think Ryan said it was some Microsoft thing?
Doing it in a separate CL from the stuff that's actually standardized seems a
good plan.

You might want to check on how the CT database works for querying it. It's
possible it rejects v4 certs or whatever quirk we're interested in.

[eroman, 2015/08/12 00:37:10]

Acknowledged. I will try and follow-up with v4 requirements, but do not plan to
support it in this CL.

+//
+//     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:
+//
+//     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.
+  }
+
+  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) {

[davidben, 2015/08/11 20:31:56]

[I probably would have written this function in BoringSSL as:

  //   Certificate  ::=  SEQUENCE  {
  //        tbsCertificate       TBSCertificate,
  //        signatureAlgorithm   AlgorithmIdentifier,
  //        signatureValue       BIT STRING  }
  der::Parser certificate_parser;
  if (!parser.ReadSequence(&certificate_parser) ||
      !certificate_parser.ReadRawTLV(&out->tbs_certificate_tlv) ||
      !certificate_parser.ReadRawTLV(&out->signature_algorithm_tlv) ||
      !certificate_parser.ReadBitString(&out->signature_value) ||
      certificate_parser.HasMore() ||
      parser.hasMore()) {
    return false;
  }
  return true;

which is much more compact. Though the interleaved style generalizes better when
you need to do something more complex. *shrug* Your call.]

[eroman, 2015/08/11 21:13:33]

This is a good point. I found it convenient in the TBSCertifiate parsing to have
a bit more verbosity and room for comments, so opted for this style throughout.
I also have a branch that builds off of this which adds unique errors for each
of the parse failures (for experimentation, not sure if it makes sense to have
that granularity of errors in the final code).

If you are OK with it, my preference is to leave it in the verbose style.

[davidben, 2015/08/11 22:39:20]

Either's fine by me. I have noticed that Chromium is less enthusiastic chaining
things with ||s than BoringSSL is.

+  der::Parser parser(certificate_tlv);
+
+  //   Certificate  ::=  SEQUENCE  {
+  der::Parser certificate_parser;
+  if (!parser.ReadSequence(&certificate_parser))
+    return false;
+
+  //        tbsCertificate       TBSCertificate,
+  if (!certificate_parser.ReadRawTLV(&out->tbs_certificate_tlv))
+    return false;
+
+  //        signatureAlgorithm   AlgorithmIdentifier,
+  if (!certificate_parser.ReadRawTLV(&out->signature_algorithm_tlv))
+    return false;
+
+  //        signatureValue       BIT STRING  }
+  if (!certificate_parser.ReadBitString(&out->signature_value))
+    return false;
+
+  // By definition the input was a single Certificate, so there shouldn't be
+  // unconsumed data.
+  if (parser.HasMore())
+    return false;
+
+  // There isn't an extension point at the end of Certificate.
+  if (certificate_parser.HasMore())
+    return false;

[davidben, 2015/08/11 20:31:56]

Nit: It seems better to check certificate_parser before parser. That way you're
completely finished parsing one element before going back to manipulate the
parent.

[eroman, 2015/08/11 21:13:33]

Will do.

[eroman, 2015/08/12 00:37:10]

Done.

+
+  return true;
+}
+
+// From RFC 5280 section 4.1:
+//
+//   Certificate  ::=  SEQUENCE  {
+//        tbsCertificate       TBSCertificate,
+//        signatureAlgorithm   AlgorithmIdentifier,
+//        signatureValue       BIT STRING  }
+//
+//   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;

[davidben, 2015/08/11 20:31:56]

Although it seems we can't enforce it, there is a DEFAULT v1 in there, which
means that getting CertificateVersion::V1 out of this function is invalid DER.
We should at least make a note that we're intentionally allowing it.

[eroman, 2015/08/11 21:13:33]

Good point!
I should enforce that here, and will do so. (I have not encountered any certs
that contradict this in the CT database.)
What do you mean by "we can't enforce it"?

[davidben, 2015/08/11 22:39:20]

I believe mozilla::pkix doesn't. Although, actually, I think the bug for it was
because of OCSP, so we probably can enforce for X.509 but not OCSP.

[eroman, 2015/08/12 00:37:10]

Acknowledged thanks for the context!
I have added the stricter enforcement of v1

(that said, I have only come across a single v1 certificate so this is unlikely
to become a compat issue one way or another).

+  } 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 (!tbs_parser.ReadRawTLV(&out->signature_algorithm_tlv))
+    return false;
+
+  //        issuer               Name,
+  if (!tbs_parser.ReadRawTLV(&out->issuer_tlv))
+    return false;
+
+  //        validity             Validity,
+  if (!tbs_parser.ReadRawTLV(&out->validity_tlv))
+    return false;
+
+  //        subject              Name,
+  if (!tbs_parser.ReadRawTLV(&out->subject_tlv))
+    return false;
+
+  //        subjectPublicKeyInfo SubjectPublicKeyInfo,
+  if (!tbs_parser.ReadRawTLV(&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))

[davidben, 2015/08/11 20:31:56]

[Verified that ParseBitString does NOT expect a TLV wrapper and only parses the
contents.]

+      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) {
+    if (out->version != CertificateVersion::V3)
+      return false;
+  }
+
+  // By definition the input was a single TBSCertificate, so there shouldn't be
+  // unconsumed data.
+  if (parser.HasMore())
+    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;

[davidben, 2015/08/11 20:31:56]

Nit: Ditto re parser vs tbs_parser order.

[eroman, 2015/08/12 00:37:10]

Done.

+
+  return true;
+}
+
+}  // namespace net