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

net/cert/internal/parse_certificate.h

Index: net/cert/internal/parse_certificate.h
diff --git a/net/cert/internal/parse_certificate.h b/net/cert/internal/parse_certificate.h
new file mode 100644
index 0000000000000000000000000000000000000000..ed6fb1a5940bc6fe1d62c5ea649c9c864303725d
--- /dev/null
+++ b/net/cert/internal/parse_certificate.h
@@ -0,0 +1,193 @@
+// 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.
+
+#ifndef NET_CERT_INTERNAL_PARSE_CERTIFICATE_H_
+#define NET_CERT_INTERNAL_PARSE_CERTIFICATE_H_
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "net/base/net_export.h"
+#include "net/der/input.h"
+#include "net/der/parse_values.h"
+
+namespace net {
+
+struct ParsedCertificate;
+struct ParsedTbsCertificate;
+
+// Parses a DER-encoded "Certificate" as specified by RFC 5280. Returns true on
+// success and sets the results in |out|.
+//
+// Refer to the per-field documention of the ParsedCertificate structure for
+// details on what validity checks parsing performs.
+//
+//       Certificate  ::=  SEQUENCE  {
+//            tbsCertificate       TBSCertificate,
+//            signatureAlgorithm   AlgorithmIdentifier,
+//            signatureValue       BIT STRING  }
+NET_EXPORT bool ParseCertificate(const der::Input& certificate_tlv,
+                                 ParsedCertificate* out) WARN_UNUSED_RESULT;
+
+// Parses a DER-encoded "TBSCertificate" as specified by RFC 5280. Returns true
+// on success and sets the results in |out|.
+//
+// Refer to the per-field documentation of ParsedTbsCertificate for details on
+// what validity checks parsing performs.
+//
+//       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
+//            }
+NET_EXPORT bool ParseTbsCertificate(const der::Input& tbs_tlv,
+                                    ParsedTbsCertificate* out)
+    WARN_UNUSED_RESULT;
+
+// Represents a "Version" from RFC 5280:
+//         Version  ::=  INTEGER  {  v1(0), v2(1), v3(2)  }
+enum class CertificateVersion {
+  V1,
+  V2,
+  V3,
+};
+
+// ParsedCertificate contains pointers to the main fields of a DER-encoded RFC
+// 5280 "Certificate".
+//
+// ParsedCertificate is expected to be filled by ParseCertificate(), so
+// subsequent field descriptions are in terms of what ParseCertificate() sets.
+struct NET_EXPORT ParsedCertificate {

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

I don't know if you want ot share code or not bother, but certificates and CRLs
both have the same structure (SEQUENCE of tbsSomething, signatureAlgorithm,
signatureValue). BasicOCSPResponse too except that there's stuff after
signatureValue. You could imagine folding those into one thing; NSS has a
CERTSignedData thing that's common to these.

I think Windows also funnels them together in similar ways?
sha256_legacy_support_win.cc implements some common code for this, although I
shouldn't have made the BoringSSL implementation use X509_ALGOR.

https://chromium.googlesource.com/chromium/src/+/master/net/cert/sha256_legac...
https://chromium.googlesource.com/chromium/src/+/master/net/cert/sha256_legac...

(No opinions as to whether this code should be usable for those interception
bits. I'm happy just leaving it implemented with CBS.)

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

That sounds reasonable, let me give it a try (although I may still keep specific
method names for Certificate, but call into a generic function).

The bigger change I will make is to the test data. Currently I express the test
inputs as a single CERTIFICATE, and use this to drive both the Certificate and
TBSCertificate parsing. To accomodate this I will factor things out to simply
input files for Certificate parsing (devoid of interesting TBSCertificate), and
separate TBSCertificate inputs.

Should be a bit easier/cleaner to understand what is being tested that way,
however the TBSCertificate on its own is not as easily manipulated with existing
tools (whereas the big Certificate DER is).

Will report back when this has been updated.
Just to make sure I understood this comment: Your are saying we could update
sha256_legacy_support_win.cc to use this new method, rather than relying on
BoringSSL's CBS_* methods?

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

I split up the tests for separate Certificate and TBSCertificate
input+expectations. I have not however generalized to a "CertSignedData" at this
time. Should be easy enough to generalize later as needed, so will defer to when
it is needed to keep the tests more focused (still need to add some missing
tests, so boring...)

[davidben, 2015/08/12 16:20:59]

Mostly as an example of something which treats this structure generically. I
don't have strong opinions as to which stack that file uses. We could do either.

+  // Corresponds with "tbsCertificate" from RFC 5280:
+  //         tbsCertificate       TBSCertificate,
+  //
+  // This contains the full (unverified) Tag-Length-Value. No guarantees are
+  // made on the tag or value (might not be a sequence for instance).

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

If you want to at least check the SEQUENCE, BoringSSL has both CBS_get_asn1 and
CBS_get_asn1_element for this purpose. The latter can still check the tag but
returns the entire element.

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

Is your suggestion here to use BoringSSL's CBS_get_asn1() in place of
net::der::Parser, or to:
  (a) use net::der::Parser(), and keep tbs_certificate_tlv, but verify the tag
was for a SEQUENCE
  (b) use net::der::Parser() but change tbs_certificate_tlv to be the contents
of the sequence (i.e. verify and strip the tag)

I chose to keep the full TLV in this initial design, since I found it made the
inputs easier to understand both in the test files, and in the helpers
themselves, but interested in your feedback!

(a) has the quirk in that we end up double-checking the tag here and in the
parsing of TBSCertificate. Whereas (b) maybe doesn't generalize as well. For
instance helpers that take SPKI currently expect the full sequence rather than
just the contents of the sequence. Similarly if the helper to parse a
SignatureAlgorithm were to take the contents of a sequence (rather than the full
sequence TLV), then every consumer that extracts and parses a SignatureAlgorithm
needs to know that a SignatureAlgorithm is a sequence and enforce such tag
before calling the SignatureAlgorithm parsing function. This seems a bit more
fidgety then internalizing the SEQUENCE check in the SignatureAlgorithm-specific
parsing code.

[davidben, 2015/08/12 16:20:59]

I just meant that (a) is an option. What you have now is fine though. I agree
(b) is problematic for the reason you describe. On further thought, I think I
agree with you about (a), not so much because we check twice (we also end up
parsing the length twice and checking twice that the der::Input is a single TLV
and not, say, two of them), but because it requires that the ParsedCertificate
code know that SignatureAlgorithm is a SEQUENCE, even though it doesn't know
about the rest of the object.

Arguably (a) would generalize better to, say a CHOICE of SEQUENCE or INTEGER, or
perhaps an OPTIONAL SEQUENCE. Something where, rather than tagging with some
context wrappers, the structure decided to use the fact that SEQUENCE and
INTEGER have distinct types. But that's the weird corner of ASN.1 where they
failed to separate the requirements of the BER family of encodings from the
abstract data model. Having any code which must parse that sort of thing also be
slightly weird seems fine.

[eroman, 2015/08/13 00:31:46]

Done -- I am now enforcing that all those TLVs must have tag SEQUENCE, and
documented the guarantee in the API contract.

+  //
+  // This can be further parsed using ParseTbsCertificate().
+  der::Input tbs_certificate_tlv;
+
+  // Corresponds with "signatureAlgorithm" from RFC 5280:
+  //         signatureAlgorithm   AlgorithmIdentifier,
+  //
+  // This contains the full (unverified) Tag-Length-Value. No guarantees are
+  // made on the tag or value (might not be a sequence for instance).
+  //
+  // This can be further parsed using SignatureValue::CreateFromDer().
+  der::Input signature_algorithm_tlv;
+
+  // Corresponds with "signatureValue" from RFC 5280:
+  //         signatureValue       BIT STRING  }
+  //
+  // Parsing guarantees that this is a valid BIT STRING.
+  der::BitString signature_value;
+};
+
+// ParsedTbsCertificate contains pointers to the main fields of a DER-encoded
+// RFC 5280 "TBSCertificate".
+//
+// ParsedTbsCertificate is expected to be filled by ParseTbsCertificate(), so
+// subsequent field descriptions are in terms of what ParseTbsCertificate()
+// sets.
+struct NET_EXPORT ParsedTbsCertificate {
+  ParsedTbsCertificate();
+  ~ParsedTbsCertificate();
+
+  // Corresponds with "version" from RFC 5280:
+  //         version         [0]  EXPLICIT Version DEFAULT v1,
+  //
+  // Parsing guarantees that the version is one of v1, v2, or v3.
+  CertificateVersion version;
+
+  // Corresponds with "serialNumber" from RFC 5280:
+  //         serialNumber         CertificateSerialNumber,
+  //
+  // This field specifically contains the content bytes of the INTEGER. So for
+  // instance if the serial number was 1000 then this would contain bytes
+  // {0x03, 0xE8}.
+  //
+  // In addition to being a valid DER-encoded INTEGER, parsing guarantees that
+  // the serial number is at most 20 bytes long. Parsing does NOT guarantee
+  // that the integer is positive (might be zero or negative).
+  der::Input serial_number;
+
+  // Corresponds with "signatureAlgorithm" from RFC 5280:
+  //         signatureAlgorithm   AlgorithmIdentifier,
+  //
+  // This contains the full (unverified) Tag-Length-Value. No guarantees are
+  // made on the tag or value (might not be a sequence for instance).
+  //
+  // This can be further parsed using SignatureValue::CreateFromDer().
+  der::Input signature_algorithm_tlv;
+
+  // Corresponds with "issuer" from RFC 5280:
+  //         issuer               Name,
+  //
+  // This contains the full (unverified) Tag-Length-Value. No guarantees are
+  // made on the tag or value.
+  der::Input issuer_tlv;
+
+  // Corresponds with "validity" from RFC 5280:
+  //         validity             Validity,
+  //
+  // This contains the full (unverified) Tag-Length-Value. No guarantees are
+  // made on the tag or value.
+  der::Input validity_tlv;
+
+  // Corresponds with "subject" from RFC 5280:
+  //         subject              Name,
+  //
+  // This contains the full (unverified) Tag-Length-Value. No guarantees are
+  // made on the tag or value.
+  der::Input subject_tlv;
+
+  // Corresponds with "subjectPublicKeyInfo" from RFC 5280:
+  //         subjectPublicKeyInfo SubjectPublicKeyInfo,
+  //
+  // This contains the full (unverified) Tag-Length-Value. No guarantees are
+  // made on the tag or value.
+  der::Input spki_tlv;
+
+  // Corresponds with "issuerUniqueID" from RFC 5280:
+  //         issuerUniqueID  [1]  IMPLICIT UniqueIdentifier OPTIONAL,
+  //                              -- If present, version MUST be v2 or v3
+  //
+  // Parsing guarantees that if issuer_unique_id is present it is a valid BIT
+  // STRING, and that the version is either v2 or v3
+  bool has_issuer_unique_id;
+  der::BitString issuer_unique_id;
+
+  // Corresponds with "subjectUniqueID" from RFC 5280:
+  //         subjectUniqueID [2]  IMPLICIT UniqueIdentifier OPTIONAL,
+  //                              -- If present, version MUST be v2 or v3
+  //
+  // Parsing guarantees that if subject_unique_id is present it is a valid BIT
+  // STRING, and that the version is either v2 or v3
+  bool has_subject_unique_id;
+  der::BitString subject_unique_id;
+
+  // Corresponds with "extensions" from RFC 5280:
+  //         extensions      [3]  EXPLICIT Extensions OPTIONAL
+  //                              -- If present, version MUST be v3
+  //
+  // This contains the full (unverified) Tag-Length-Value. No guarantees are
+  // made on the tag or value.

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

This includes the TLV for Extensions, but not the explicit [3] tag. The comment
should probably be clear on that.

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

Done.

+  //
+  // Parsing guarantees that if extensions is present the version is v3.
+  bool has_extensions;
+  der::Input extensions_tlv;
+};
+
+}  // namespace net
+
+#endif  // NET_CERT_INTERNAL_PARSE_CERTIFICATE_H_