Add a function for parsing an RFC 5280 Extension.

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 9535459bb8ad27ccb39b0807e24d210d4c5212ff..8de3a336548a2d080bbd03cb77d82e0371c6d762 100644
--- a/net/cert/internal/parse_certificate.cc
+++ b/net/cert/internal/parse_certificate.cc
@@ -353,4 +353,56 @@ bool ParseTbsCertificate(const der::Input& tbs_tlv, ParsedTbsCertificate* out) {
   return true;
 }
 
+// From RFC 5280:
+//
+//    Extension  ::=  SEQUENCE  {
+//            extnID      OBJECT IDENTIFIER,
+//            critical    BOOLEAN DEFAULT FALSE,
+//            extnValue   OCTET STRING
+//                        -- contains the DER encoding of an ASN.1 value
+//                        -- corresponding to the extension type identified
+//                        -- by extnID
+//            }
+bool ParseExtension(const der::Input& extension_tlv, ParsedExtension* out) {
+  der::Parser parser(extension_tlv);
+
+  //    Extension  ::=  SEQUENCE  {
+  der::Parser extension_parser;
+  if (!parser.ReadSequence(&extension_parser))
+    return false;
+
+  //            extnID      OBJECT IDENTIFIER,
+  if (!extension_parser.ReadTag(der::kOid, &out->oid))
+    return false;
+
+  //            critical    BOOLEAN DEFAULT FALSE,
+  out->critical = false;
+  bool has_critical;
+  der::Input critical;
+  if (!extension_parser.ReadOptionalTag(der::kBool, &critical, &has_critical))
+    return false;
+  if (has_critical) {
+    if (!der::ParseBool(critical, &out->critical))
+      return false;
+    if (!out->critical)
+      return false;  // DER-encoding requires DEFAULT values be omitted.

[Ryan Sleevi, 2015/10/28 22:05:59]

See http://hg.mozilla.org/mozilla-central/rev/0c50a30b5be7

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

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

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

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

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]

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?

+  }
+
+  //            extnValue   OCTET STRING
+  if (!extension_parser.ReadTag(der::kOctetString, &out->value))
+    return false;
+
+  // By definition the input was a single Extension sequence, so there shouldn't
+  // be unconsumed data.
+  if (parser.HasMore())
+    return false;
+
+  // The Extension type does not have an extension point (everything goes in
+  // extnValue).
+  if (extension_parser.HasMore())
+    return false;
+
+  return true;
+}
+
 }  // namespace net