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

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

BUG=410574
Committed: https://crrev.com/357ec5b3c222e728632cdbd44ed382fc1f193675
Cr-Commit-Position: refs/heads/master@{#344109}

[eroman, 2015-08-07 05:00:24]

eroman@chromium.org changed reviewers:
+ davidben@chromium.org, rsleevi@chromium.org

[eroman, 2015-08-07 05:27:22]

I am still in the process of adding unit-tests, but would appreciate an initial
review for the rest.

DESIGN:

I didn't want to create a single mega parsing function that fully parses
everything, since I think it is more flexible to pick the pieces out gradually
as needed.

... This does however lead to some ambiguity as to which layer of code is
responsible for which validations.

The way I have tried to arrange things is that anything that is not a sequence
gets immediately validated. So if the result is in a form which looks usable,
the minimal validations have been done.

As for sequence fields (spki, algorithmIdentifiers, validity, subject, etc),
their parsing is deferred, and the responsibility of a next layer of helpers. As
long as other code is using those fields at some point, they should eventually
get parsed and validated.

On the other hand, parsing the entirety of Certificate/TBSCertificate upfront
into our own in-memory representation would give greater confidence that
structurally malformed data is always rejected. (We can always go that route
later once we have all the building blocks in place.)

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

Some initial comments. Looks pretty reasonable I think?

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:21: // certificate versions in use
today):
Do we need to handle this unspecified v4 thing?

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:123: ParsedCertificate* out) {
[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.]

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:150: return false;
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.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:194: return false;
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.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:234: if
(!der::ParseBitString(issuer_unique_id, &out->issuer_unique_id))
[Verified that ParseBitString does NOT expect a TLV wrapper and only parses the
contents.]

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:283: return false;
Nit: Ditto re parser vs tbs_parser order.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:70: struct NET_EXPORT ParsedCertificate {
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.)

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:75: // made on the tag or value (might not
be a sequence for instance).
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.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:184: // made on the tag or value.
This includes the TLV for Extensions, but not the explicit [3] tag. The comment
should probably be clear on that.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate_unittest.cc (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate_unittest.cc:20: // These functions are used
by GTEST to support EXPECT_EQ() for
Nit: I think GTest is the more common capitalization? Judging by a search of
"GTest".

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate_unittest.cc:23: void PrintTo(const Input&
data, ::std::ostream* os) {
#include <ostream>

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

Thanks for the initial look over!

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:21: // certificate versions in use
today):
On 2015/08/11 20:31:56, David Benjamin wrote:
> Do we need to handle this unspecified v4 thing?

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.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:123: ParsedCertificate* out) {
On 2015/08/11 20:31:56, David Benjamin wrote:
> [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.]

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.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:150: return false;
On 2015/08/11 20:31:56, David Benjamin wrote:
> 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.

Will do.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:194: return false;
On 2015/08/11 20:31:56, David Benjamin wrote:
> 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.

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"?

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:70: struct NET_EXPORT ParsedCertificate {
On 2015/08/11 20:31:56, David Benjamin wrote:
> 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.

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.
> 
> 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.)

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?

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:75: // made on the tag or value (might not
be a sequence for instance).
On 2015/08/11 20:31:57, David Benjamin wrote:
> 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.

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-11 22:39:21]

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:21: // certificate versions in use
today):
On 2015/08/11 21:13:33, eroman wrote:
> On 2015/08/11 20:31:56, David Benjamin wrote:
> > Do we need to handle this unspecified v4 thing?
> 
> 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.

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.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:123: ParsedCertificate* out) {
On 2015/08/11 21:13:33, eroman wrote:
> On 2015/08/11 20:31:56, David Benjamin wrote:
> > [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.]
> 
> 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.

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

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:194: return false;
On 2015/08/11 21:13:33, eroman wrote:
> On 2015/08/11 20:31:56, David Benjamin wrote:
> > 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.
> 
> 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"?

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]

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:21: // certificate versions in use
today):
On 2015/08/11 22:39:20, David Benjamin wrote:
> On 2015/08/11 21:13:33, eroman wrote:
> > On 2015/08/11 20:31:56, David Benjamin wrote:
> > > Do we need to handle this unspecified v4 thing?
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:150: return false;
On 2015/08/11 21:13:33, eroman wrote:
> On 2015/08/11 20:31:56, David Benjamin wrote:
> > 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.
> 
> Will do.

Done.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:194: return false;
On 2015/08/11 22:39:20, David Benjamin wrote:
> On 2015/08/11 21:13:33, eroman wrote:
> > On 2015/08/11 20:31:56, David Benjamin wrote:
> > > 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.
> > 
> > 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"?
> 
> 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.

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).

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:283: return false;
On 2015/08/11 20:31:56, David Benjamin wrote:
> Nit: Ditto re parser vs tbs_parser order.

Done.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:70: struct NET_EXPORT ParsedCertificate {
On 2015/08/11 21:13:34, eroman wrote:
> On 2015/08/11 20:31:56, David Benjamin wrote:
> > 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.
> 
> 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.
> > 
> > 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.)
> 
> 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?

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...)

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:184: // made on the tag or value.
On 2015/08/11 20:31:57, David Benjamin wrote:
> This includes the TLV for Extensions, but not the explicit [3] tag. The
comment
> should probably be clear on that.

Done.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate_unittest.cc (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate_unittest.cc:20: // These functions are used
by GTEST to support EXPECT_EQ() for
On 2015/08/11 20:31:57, David Benjamin wrote:
> Nit: I think GTest is the more common capitalization? Judging by a search of
> "GTest".

Done.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate_unittest.cc:23: void PrintTo(const Input&
data, ::std::ostream* os) {
On 2015/08/11 20:31:57, David Benjamin wrote:
> #include <ostream>

Done.

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

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:70: struct NET_EXPORT ParsedCertificate {
On 2015/08/11 21:13:34, eroman wrote:
> On 2015/08/11 20:31:56, David Benjamin wrote:
> > 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.
> 
> 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.
> > 
> > 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.)
> 
> 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?

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.

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:75: // made on the tag or value (might not
be a sequence for instance).
On 2015/08/11 21:13:33, eroman wrote:
> On 2015/08/11 20:31:57, David Benjamin wrote:
> > 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.
> 
> 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.

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]

I split the Certificate parsing to a separate CL
(https://codereview.chromium.org/1288193003/) and kept this just about
TBSCertificate.

(Found it a bit more readable that way, especially to understand which tests are
being done for each).

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1279963003/diff/20001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:75: // made on the tag or value (might not
be a sequence for instance).
On 2015/08/12 16:20:59, David Benjamin wrote:
> On 2015/08/11 21:13:33, eroman wrote:
> > On 2015/08/11 20:31:57, David Benjamin wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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

[davidben, 2015-08-14 17:51:43]

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:84: // For reference, here is what RFC
5280 section 4.1.2.2 says:
Nit: s/here is what // is marginally less verbose.

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:122: }
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. :-) )

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:282: // SEQUENCE.
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.

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:17: struct ParsedTbsCertificate;
[Whatever we end up doing for the other CL, do for this one too.]

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:33: // on success and sets the results in
|out|.
Add: The resulting ParsedTbsCertificate is valid as long as |tbs_tlv|'s contents
are.

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:186: // EXPLICIT outter tag was stripped).
outter -> outer
was -> is?
). -> .)  (I was always taught that you put the period inside the parens if a
full sentence is a parenthetical.)

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
File net/cert/internal/parse_certificate_unittest.cc (right):

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate_unittest.cc:114: void
EnsureParsingTbsSucceds(const std::string& file_name,
Succeds -> Succeeds

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate_unittest.cc:155:
EXPECT_FALSE(parsed.has_subject_unique_id);
You weren't able to find any certificates with these to test against, right?

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate_unittest.cc:234: 
Think it's worth parsing a real-world certificate as well? I think all of these
tests use bogus ones.

https://codereview.chromium.org/1279963003/diff/170001/net/data/parse_certifi...
File net/data/parse_certificate_unittest/tbs_explicit_v1.pem (right):

https://codereview.chromium.org/1279963003/diff/170001/net/data/parse_certifi...
net/data/parse_certificate_unittest/tbs_explicit_v1.pem:1: This is is an
otherwise valid v1 certificate, except the version has been encoded explicitly
as 0. Instead it should have been omitted.
Nit: Probably should wrap these lines and others.

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

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:122: }
On 2015/08/14 17:51:42, David Benjamin wrote:
> 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. :-) )

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]

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:122: }
On 2015/08/14 18:11:59, eroman wrote:
> On 2015/08/14 17:51:42, David Benjamin wrote:
> > 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. :-) )
> 
> 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.

SGTM

[eroman, 2015-08-14 21:26:13]

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:122: }
On 2015/08/14 18:19:22, David Benjamin wrote:
> On 2015/08/14 18:11:59, eroman wrote:
> > On 2015/08/14 17:51:42, David Benjamin wrote:
> > > 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. :-) )
> > 
> > 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.
> 
> SGTM

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

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:282: // SEQUENCE.
On 2015/08/14 17:51:42, David Benjamin wrote:
> 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.

Done.

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:17: struct ParsedTbsCertificate;
On 2015/08/14 17:51:42, David Benjamin wrote:
> [Whatever we end up doing for the other CL, do for this one too.]

Acknowledged.

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:33: // on success and sets the results in
|out|.
On 2015/08/14 17:51:42, David Benjamin wrote:
> Add: The resulting ParsedTbsCertificate is valid as long as |tbs_tlv|'s
contents
> are.

Done.

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:186: // EXPLICIT outter tag was stripped).
On 2015/08/14 17:51:42, David Benjamin wrote:
> outter -> outer
> was -> is?
> ). -> .)  (I was always taught that you put the period inside the parens if a
> full sentence is a parenthetical.)

Done.

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
File net/cert/internal/parse_certificate_unittest.cc (right):

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate_unittest.cc:114: void
EnsureParsingTbsSucceds(const std::string& file_name,
On 2015/08/14 17:51:42, David Benjamin wrote:
> Succeds -> Succeeds

Done. Hah, the power of auto-complete to propagate this mistake throughout the
file!

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate_unittest.cc:155:
EXPECT_FALSE(parsed.has_subject_unique_id);
On 2015/08/14 17:51:42, David Benjamin wrote:
> You weren't able to find any certificates with these to test against, right?

Correct. I added a TODO and will add some. (Either in this CL or a follow-up,
depends on the timing :)

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate_unittest.cc:234: 
On 2015/08/14 17:51:42, David Benjamin wrote:
> Think it's worth parsing a real-world certificate as well? I think all of
these
> tests use bogus ones.

Done --> tbs_v3_real.pem

https://codereview.chromium.org/1279963003/diff/170001/net/data/parse_certifi...
File net/data/parse_certificate_unittest/tbs_explicit_v1.pem (right):

https://codereview.chromium.org/1279963003/diff/170001/net/data/parse_certifi...
net/data/parse_certificate_unittest/tbs_explicit_v1.pem:1: This is is an
otherwise valid v1 certificate, except the version has been encoded explicitly
as 0. Instead it should have been omitted.
On 2015/08/14 17:51:42, David Benjamin wrote:
> Nit: Probably should wrap these lines and others.

Done (throughout)

[eroman, 2015-08-15 01:57:41]

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
File net/cert/internal/parse_certificate_unittest.cc (right):

https://codereview.chromium.org/1279963003/diff/170001/net/cert/internal/pars...
net/cert/internal/parse_certificate_unittest.cc:155:
EXPECT_FALSE(parsed.has_subject_unique_id);
On 2015/08/14 21:26:13, eroman wrote:
> On 2015/08/14 17:51:42, David Benjamin wrote:
> > You weren't able to find any certificates with these to test against, right?
> 
> Correct. I added a TODO and will add some. (Either in this CL or a follow-up,
> depends on the timing :)

Done -- I added tests for v2 and its fields (issuerUniqueId / subjectUniqueId)

[davidben, 2015-08-17 21:16:02]

lgtm!

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:113: has_extensions(false) {}
Optional: Non-static initializers are allowed now, so you can also just set
those in the header.

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:47: // updated during parsing, whereas
others were not changed.
Nit: were not -> may not have been

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:200: // EXPLICIT outter tag was stripped.)
outter -> outer
was -> is

[eroman, 2015-08-18 00:20:50]

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:113: has_extensions(false) {}
On 2015/08/17 21:16:02, David Benjamin wrote:
> Optional: Non-static initializers are allowed now, so you can also just set
> those in the header.

Done. 

As an aside, I have gotten feedback from Ryan discouraging such initializers in
the past, i.e.
https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...

But hey, I like them :)

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:47: // updated during parsing, whereas
others were not changed.
On 2015/08/17 21:16:02, David Benjamin wrote:
> Nit: were not -> may not have been

Done (also changed the wording in ParseCertificate to match).

[davidben, 2015-08-18 00:22:56]

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:113: has_extensions(false) {}
On 2015/08/18 00:20:50, eroman wrote:
> On 2015/08/17 21:16:02, David Benjamin wrote:
> > Optional: Non-static initializers are allowed now, so you can also just set
> > those in the header.
> 
> Done. 
> 
> As an aside, I have gotten feedback from Ryan discouraging such initializers
in
> the past, i.e.
>
https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
> 
> But hey, I like them :)

Interesting! I'm pretty sure I've gotten feedback from him to use them. :-)
Though I guess that one didn't have an explicit constructor. Yeah I dunno.
chromium-cpp allows them but the style guide doesn't seem to have much explicit
guidance.

[eroman, 2015-08-18 00:35:52]

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.cc (right):

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:113: has_extensions(false) {}
On 2015/08/18 00:22:56, David Benjamin wrote:
> On 2015/08/18 00:20:50, eroman wrote:
> > On 2015/08/17 21:16:02, David Benjamin wrote:
> > > Optional: Non-static initializers are allowed now, so you can also just
set
> > > those in the header.
> > 
> > Done. 
> > 
> > As an aside, I have gotten feedback from Ryan discouraging such initializers
> in
> > the past, i.e.
> >
>
https://codereview.chromium.org/1115903002/diff/40001/net/cert/multi_threaded...
> > 
> > But hey, I like them :)
> 
> Interesting! I'm pretty sure I've gotten feedback from him to use them. :-)
> Though I guess that one didn't have an explicit constructor. Yeah I dunno.
> chromium-cpp allows them but the style guide doesn't seem to have much
explicit
> guidance.

As an aside: I understand member initializers better now than I did back then
and can answer sleevi's comment. Conceptually these initializations are applied
before entering any constructors (delegated or not) So you can have constructors
that use initializer lists and that is fine. However if the member in question
is set by an initializer list that will take precedence.

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/1279963003/diff/330001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:200: // EXPLICIT outter tag was stripped.)
On 2015/08/17 21:16:02, David Benjamin wrote:
> outter -> outer
> was -> is

Done.

[eroman, 2015-08-18 23:55:37]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-08-18 23:55:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1279963003/#ps360001
(title: "one more comment fix")

[commit-bot: I haz the power, 2015-08-18 23:56:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1279963003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1279963003/360001

[commit-bot: I haz the power, 2015-08-19 01:03:57]

Committed patchset #22 (id:360001)

[commit-bot: I haz the power, 2015-08-19 01:04:48]

Patchset 22 (id:??) landed as
https://crrev.com/357ec5b3c222e728632cdbd44ed382fc1f193675
Cr-Commit-Position: refs/heads/master@{#344109}