Add a function for parsing an RFC 5280 Extension.

Add a function for parsing an RFC 5280 Extension.

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

[eroman, 2015-10-01 21:29:31]

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

[eroman, 2015-10-01 21:29:31]

Tested over CT certificates and didn't find any compat issues with this simple
function

[eroman, 2015-10-22 20:05:13]

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

[eroman, 2015-10-22 20:05:14]

Changing reviewer davidben --> rsleevi

[Ryan Sleevi, 2015-10-28 22:06:00]

I'm surprised you didn't find compat issues, given what Mozilla's seen from
GoDaddy certificates.

Are you sure your test harness is correct?

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

https://codereview.chromium.org/1383873002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:388: return false;  // DER-encoding
requires DEFAULT values be omitted.
See http://hg.mozilla.org/mozilla-central/rev/0c50a30b5be7

This is common enough, particularly for GoDaddy certificates, that it seems it
should be permitted.

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

https://codereview.chromium.org/1383873002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:233: bool critical;
bool critical = false;

[eroman, 2015-11-02 20:59:43]

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

https://codereview.chromium.org/1383873002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:388: return false;  // DER-encoding
requires DEFAULT values be omitted.
On 2015/10/28 22:05:59, Ryan Sleevi (OOO. no rvws plz) wrote:
> See http://hg.mozilla.org/mozilla-central/rev/0c50a30b5be7
> 
> This is common enough, particularly for GoDaddy certificates, that it seems it
> should be permitted.

The problem with "BOOLEAN DEFAULT FALSE" seems to be limited to the
BasicConstraints extension encoding of "ca" (which is addressed in parsing of
BasicConstraints).

I have not see it as a problem in the critical field for extensions (at least
using the CT corpus), so I will remain strict on that parsing.

I will confirm that I did in fact run these tests..

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

https://codereview.chromium.org/1383873002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:233: bool critical;
On 2015/10/28 22:05:59, Ryan Sleevi (OOO. no rvws plz) wrote:
> bool critical = false;

Done.

[eroman, 2015-11-02 21:07:00]

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

[eroman, 2015-11-02 21:07:01]

Changing reviewer back to David/Matt, as Ryan OOO :)

[mattm, 2015-11-03 04:20:47]

lgtm

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

https://codereview.chromium.org/1383873002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:388: return false;  // DER-encoding
requires DEFAULT values be omitted.
On 2015/11/02 20:59:43, eroman wrote:
> On 2015/10/28 22:05:59, Ryan Sleevi (OOO. no rvws plz) wrote:
> > See http://hg.mozilla.org/mozilla-central/rev/0c50a30b5be7
> > 
> > This is common enough, particularly for GoDaddy certificates, that it seems
it
> > should be permitted.
> 
> The problem with "BOOLEAN DEFAULT FALSE" seems to be limited to the
> BasicConstraints extension encoding of "ca" (which is addressed in parsing of
> BasicConstraints).
> 
> I have not see it as a problem in the critical field for extensions (at least
> using the CT corpus), so I will remain strict on that parsing.
> 
> I will confirm that I did in fact run these tests..

By "have not seen it" you mean in significant numbers? I saw it 5 times in the
CT db when looking only at the certificate policies extension. (5 out of
9602650. That also didn't check whether they were expired certs or not.)

[eroman, 2015-11-03 04:53:18]

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

https://codereview.chromium.org/1383873002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_certificate.cc:388: return false;  // DER-encoding
requires DEFAULT values be omitted.
On 2015/11/03 04:20:47, mattm wrote:
> On 2015/11/02 20:59:43, eroman wrote:
> > On 2015/10/28 22:05:59, Ryan Sleevi (OOO. no rvws plz) wrote:
> > > See http://hg.mozilla.org/mozilla-central/rev/0c50a30b5be7
> > > 
> > > This is common enough, particularly for GoDaddy certificates, that it
seems
> it
> > > should be permitted.
> > 
> > The problem with "BOOLEAN DEFAULT FALSE" seems to be limited to the
> > BasicConstraints extension encoding of "ca" (which is addressed in parsing
of
> > BasicConstraints).
> > 
> > I have not see it as a problem in the critical field for extensions (at
least
> > using the CT corpus), so I will remain strict on that parsing.
> > 
> > I will confirm that I did in fact run these tests..
> 
> By "have not seen it" you mean in significant numbers? I saw it 5 times in the
> CT db when looking only at the certificate policies extension. (5 out of
> 9602650. That also didn't check whether they were expired certs or not.)

Correct, those are the same results I got.

5 certificates specified the DEFAULT encoding for criticality.
4 of those expired in 2011.

The remaining one is an intermediary (SHECA G2-1) issued by UniTrust. It uses
SHA-1 and has a validity date into 2028. So being strict would impact any cert
issued from that intermediary. What do you all think, does this meet our bar for
compatibility?
Also, it is a SHA-1 certificate, 
Not sure that is sufficient grounds to relax the bool parsing. What do you
think?

[Ryan Sleevi, 2015-11-03 05:30:24]

On 2015/11/03 04:53:18, eroman wrote:
> What do you all think, does this meet our bar for
> compatibility?

No

> Also, it is a SHA-1 certificate, 
> Not sure that is sufficient grounds to relax the bool parsing. What do you
> think?

SHECA has had another issues. For something as clear as this, it's fine to
break. IIRC it is only trusted by MSFT.

[davidben, 2015-11-04 17:56:12]

lgtm

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

https://codereview.chromium.org/1383873002/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:403: return false;
Super nitpicky nit: I'd swap the order of this check and line 397. That way you
check for emptiness in stack order.

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

https://codereview.chromium.org/1383873002/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:230: // |value| will contain the CONTENTS
of the OCTET STRING. For instance for
Nit: 'CONTENTS' in all caps is kinda confusing since it looks like an ASN.1
type. Maybe just lowercase. Could also maybe use '*contents*' if you want
emphasis?

[eroman, 2015-11-07 00:35:35]

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

https://codereview.chromium.org/1383873002/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.cc:403: return false;
On 2015/11/04 17:56:12, davidben wrote:
> Super nitpicky nit: I'd swap the order of this check and line 397. That way
you
> check for emptiness in stack order.

Done.

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

https://codereview.chromium.org/1383873002/diff/100001/net/cert/internal/pars...
net/cert/internal/parse_certificate.h:230: // |value| will contain the CONTENTS
of the OCTET STRING. For instance for
On 2015/11/04 17:56:12, davidben wrote:
> Nit: 'CONTENTS' in all caps is kinda confusing since it looks like an ASN.1
> type. Maybe just lowercase. Could also maybe use '*contents*' if you want
> emphasis?

Done.
Not sure why I was shouting at the time...

[eroman, 2015-11-10 19:50:54]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-11-10 19:50:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/1383873002/#ps140001
(title: "Fix U-A-F in tests")

[commit-bot: I haz the power, 2015-11-10 19:51:37]

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

[commit-bot: I haz the power, 2015-11-10 21:46:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-11-10 21:47:57]

Patchset 8 (id:??) landed as
https://crrev.com/bbc5d0867f8a5ebc21ea9541126f5b2412231d7e
Cr-Commit-Position: refs/heads/master@{#358914}