Extract Certificate Transparency SCTs from stapled OCSP responses

net/cert/asn1_util.cc

Index: net/cert/asn1_util.cc
diff --git a/net/cert/asn1_util.cc b/net/cert/asn1_util.cc
index 6dcff52ee65c90a4eb3ed033b226b0709a2e9b3d..a4d44b1c2b4fbd40352c70100b80c4e096c4fed7 100644
--- a/net/cert/asn1_util.cc
+++ b/net/cert/asn1_util.cc
@@ -326,6 +326,286 @@ bool ExtractCRLURLsFromDERCert(base::StringPiece cert,
   return true;
 }
 
+// ExtractSingleResponses extracts the ASN.1 'responses' element
+// (SEQUENCE OF SingleResponse contents, excluding the SEQUENCE tag and length)
+// from the 'tbsResponseData' of a BasicOCSPResponse. On successful return,
+// |single_responses| points into |ocsp_response|.
+static bool ExtractSingleResponses(base::StringPiece ocsp_response,
+                                   base::StringPiece* single_responses) {
+  // OCSPResponse ::= SEQUENCE {
+  //    responseStatus         OCSPResponseStatus,
+  //    responseBytes          [0] EXPLICIT ResponseBytes OPTIONAL }
+  base::StringPiece ocsp_response_sequence;
+  if (!GetElement(&ocsp_response, kSEQUENCE, &ocsp_response_sequence))
+    return false;
+  if (!ocsp_response.empty())
+    return false;
+
+  // responseStatus
+  if (!GetElement(&ocsp_response_sequence, kENUMERATED, NULL))
+    return false;
+
+  // responseBytes
+  base::StringPiece response_bytes_outer;
+  if (!GetElement(&ocsp_response_sequence,
+                  kOptional | kConstructed | kContextSpecific | 0,
+                  &response_bytes_outer)) {
+    return false;
+  }
+  if (!ocsp_response_sequence.empty())
+    return false;
+
+  // ResponseBytes ::=       SEQUENCE {
+  //     responseType   OBJECT IDENTIFIER,
+  //     response       OCTET STRING } -- DER encoding of BasicOCSPResponse
+  base::StringPiece response_bytes;
+  if (!GetElement(&response_bytes_outer, kSEQUENCE, &response_bytes))
+    return false;
+  if (!response_bytes_outer.empty())
+    return false;
+
+  // responseType
+  base::StringPiece response_type;
+  if (!GetElement(&response_bytes, kOID, &response_type))
+    return false;
+
+  // id-ad-ocsp: 1.3.6.1.5.5.7.48.1.1
+  static const uint8 kBasicOCSPResponseOID[] = { 0x2b, 0x06, 0x01, 0x05, 0x05,
+                                                 0x07, 0x30, 0x01, 0x01 };
+
+  if (response_type.size() != sizeof(kBasicOCSPResponseOID) ||
+      memcmp(response_type.data(), kBasicOCSPResponseOID,
+             response_type.size()) != 0) {
+    return false;
+  }
+
+  base::StringPiece wrapped_response;
+  if (!GetElement(&response_bytes, kOCTETSTRING, &wrapped_response))
+    return false;
+
+  if (!response_bytes.empty())
+    return false;
+
+  // BasicOCSPResponse       ::= SEQUENCE {
+  //       tbsResponseData      ResponseData,
+  //       signatureAlgorithm   AlgorithmIdentifier,
+  //       signature            BIT STRING,
+  //       certs                [0] EXPLICIT SEQUENCE OF Certificate OPTIONAL }
+  base::StringPiece basic_ocsp_response;
+  if (!GetElement(&wrapped_response, kSEQUENCE, &basic_ocsp_response))
+    return false;
+  if (!wrapped_response.empty())
+    return false;
+
+  // ResponseData ::= SEQUENCE {
+  //     version              [0] EXPLICIT Version DEFAULT v1,
+  //     responderID              ResponderID,
+  //     producedAt               GeneralizedTime,
+  //     responses                SEQUENCE OF SingleResponse,
+  //     responseExtensions   [1] EXPLICIT Extensions OPTIONAL }
+  base::StringPiece tbs_response_data;
+  if (!GetElement(&basic_ocsp_response, kSEQUENCE, &tbs_response_data))
+    return false;
+
+  // version
+  if (!GetElement(&tbs_response_data,
+                  kOptional | kConstructed | kContextSpecific | 0, NULL)) {
+    return false;
+  }
+
+  // ResponderID ::= CHOICE {
+  //    byName               [1] Name,
+  //    byKey                [2] KeyHash }
+  if (!GetElement(&tbs_response_data,
+                  kConstructed | kContextSpecific | 1, NULL) &&
+      !GetElement(&tbs_response_data,
+                  kConstructed | kContextSpecific | 2, NULL)) {
+    return false;
+  }
+
+  // producedAt
+  if (!GetElement(&tbs_response_data, kGENERALIZEDTIME, NULL))
+    return false;
+
+  // responses: SEQUENCE OF and SEQUENCE share the same tag.
+  base::StringPiece responses;
+  if (!GetElement(&tbs_response_data, kSEQUENCE, &responses))
+    return false;

[wtc, 2013/12/03 21:04:25]

Nit: you are meticulous in checking there is no junk after the expected
elements.

Here you are not checking that there is no junk in tbs_response_data after
responseExtensions.

We can add this check, or we can remove all the checks for extra junk to make
these functions smaller.

+
+  *single_responses = responses;
+  return true;
+}
+
+// ExtractExtensionsWithMatchingCertID extracts the 'singleExtensions' inner

[wtc, 2013/12/03 21:04:25]

Nit: this comment says "inner contents", but the code on line 508 uses the
variable name "single_extensions_outer". They seem to contradict each other.

+// contents (Extensions) of a SingleResponse matching the given certificate
+// identifiers. Returns False if ASN.1 parsing failed, or if the OCSP response

[wtc, 2013/12/03 21:04:25]

Nit: "identifiers" => "identifier" or "serial number" (singular)

+// did not contain a matching SingleResponse. On successful return,
+// |single_extensions| is either empty (no extensions found in the matching
+// response) or points into |single_responses|.
+static bool ExtractExtensionsWithMatchingCertID(
+    base::StringPiece single_responses,
+    base::StringPiece cert_serial_number,
+    base::StringPiece* single_extensions) {
+  while (single_responses.size() > 0) {

[wtc, 2013/12/03 21:04:25]

Nit: use !single_responses.empty(), to be consistent with the check on line 565.

+    // SingleResponse ::= SEQUENCE {
+    //   certID                       CertID,
+    //   certStatus                   CertStatus,
+    //   thisUpdate                   GeneralizedTime,
+    //   nextUpdate         [0]       EXPLICIT GeneralizedTime OPTIONAL,
+    //   singleExtensions   [1]       EXPLICIT Extensions OPTIONAL }
+    base::StringPiece single_response;
+    if (!GetElement(&single_responses, kSEQUENCE, &single_response))
+      return false;
+
+    // CertID ::= SEQUENCE {
+    //     hashAlgorithm            AlgorithmIdentifier,
+    //     issuerNameHash     OCTET STRING, -- Hash of Issuer's DN
+    //     issuerKeyHash      OCTET STRING, -- Hash of Issuers public key
+    //     serialNumber       CertificateSerialNumber }
+    base::StringPiece cert_id;
+    if (!GetElement(&single_response, kSEQUENCE, &cert_id))
+      return false;
+
+    // hashAlgorithm
+    if (!GetElement(&cert_id, kSEQUENCE, NULL))
+      return false;
+    // issuerNameHash
+    if (!GetElement(&cert_id, kOCTETSTRING, NULL))
+      return false;
+    // issuerKeyHash
+    if (!GetElement(&cert_id, kOCTETSTRING, NULL))
+      return false;
+
+    base::StringPiece serial_number;
+    if (!GetElement(&cert_id, kINTEGER, &serial_number))
+      return false;
+
+    if (serial_number != cert_serial_number) {
+      // Serial number mismatch - continue.
+      continue;
+    }
+    // Serial number match: continue to see if the CT extension is present.
+    // CertStatus ::= CHOICE {
+    //     good                [0]     IMPLICIT NULL,
+    //     revoked             [1]     IMPLICIT RevokedInfo,
+    //     unknown             [2]     IMPLICIT UnknownInfo }
+    if (!GetElement(&single_response, kContextSpecific | 0, NULL) &&
+        !GetElement(&single_response,
+                    kContextSpecific | kConstructed | 1, NULL) &&
+        !GetElement(&single_response, kContextSpecific | 2, NULL)) {
+      return false;
+    }
+
+    // thisUpdate
+    if (!GetElement(&single_response, kGENERALIZEDTIME, NULL))
+      return false;
+    // nextUpdate
+    if (!GetElement(&single_response,
+                    kConstructed | kOptional | kContextSpecific | 0, NULL)) {
+      return false;
+    }
+
+    base::StringPiece single_extensions_outer;
+    if (!GetElement(&single_response,
+                    kConstructed | kOptional | kContextSpecific | 1,
+                    &single_extensions_outer)) {
+      return false;
+    }
+    if (!single_response.empty())
+      return false;
+
+    *single_extensions = single_extensions_outer;
+    return true;
+  }
+
+  // Match not found.
+  return false;
+}
+
+bool ExtractSCTExtensionFromOCSPResponse(

[wtc, 2013/12/03 21:04:25]

I found that NSS has a function for decoding an OCSP response:
http://mxr.mozilla.org/nss/ident?i=CERT_DecodeOCSPResponse

In particular, the singleExtensions is available in the decoded OCSP response:
http://mxr.mozilla.org/nss/ident?i=singleExtensions

Unfortunately, the decoded OCSP response is an opaque struct type because the
header that defines those types is considered NSS internal. This can be fixed,
but until then we'll need to write our own decoding function.

It is worth considering using the NSS ASN.1 decoder to do this, essentially
copying the code of CERT_DecodeOCSPResponse. But we'll still need your
implementation for OpenSSL. Does OpenSSL also have a general-purpose ASN.1
decoder?

+    base::StringPiece ocsp_response,
+    base::StringPiece cert_serial_number,
+    base::StringPiece* sct_list_out) {
+  sct_list_out->clear();
+
+  base::StringPiece single_responses;
+  if (!ExtractSingleResponses(ocsp_response, &single_responses))
+    return false;
+
+  base::StringPiece single_extensions;
+  if (!ExtractExtensionsWithMatchingCertID(single_responses,
+                                           cert_serial_number,
+                                           &single_extensions)) {
+    return false;
+  }
+
+  if (single_extensions.empty()) {
+    // Extensions are optional.
+    return true;
+  }
+
+  // Extensions  ::=  SEQUENCE SIZE (1..MAX) OF Extension
+  base::StringPiece single_extensions_inner;
+  if (!GetElement(&single_extensions, kSEQUENCE, &single_extensions_inner))
+    return false;

[wtc, 2013/12/03 21:04:25]

Nit: you didn't check for extra junk at the end of single_extensions here:

  if (!single_extensions.empty())
    return false;

Perhaps we should just remove all the checks for extra junk.

+
+  if (single_extensions_inner.empty()) {
+    // Extensions, if present, should contain at least one element.
+    return false;
+  }
+
+  // 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
+  //      }
+  while (!single_extensions_inner.empty()) {
+    base::StringPiece extension;
+    if (!GetElement(&single_extensions_inner, kSEQUENCE, &extension))
+      return false;
+
+    base::StringPiece extn_id;
+    if (!GetElement(&extension, kOID, &extn_id))
+      return false;
+
+    static const uint8 kSignedCertTimestampListOID[] = {
+      0x2b, 0x06, 0x01, 0x04, 0x01, 0xd6, 0x79, 0x02, 0x04, 0x05
+    };
+    if (extn_id.size() != sizeof(kSignedCertTimestampListOID) ||
+        memcmp(extn_id.data(), kSignedCertTimestampListOID,
+               extn_id.size() != 0)) {

[wtc, 2013/12/03 21:04:25]

BUG: extn_id.size() != 0)) => extn_id.size()) != 0)

By the way, a bug of this kind was just recently found by PVS-Studio in
Chromium: https://codereview.chromium.org/99423002/
Otherwise I don't think I would have noticed it.

[ekasper, 2013/12/04 19:25:15]

This has gone away but... YIKES. Good catch.

+      continue;
+    }
+
+    // critical
+    if (!GetElement(&extension, kBOOLEAN, NULL))

[wtc, 2013/12/03 21:04:25]

IMPORTANT: Should we allow |critical| to be optional since it has a DEFAULT
value? Compare this with lines 253-254.

[ekasper, 2013/12/04 19:25:15]

No, this was a bug that tests didn't catch because I introduced it both here and
in the test data :(

I've fixed the test data and the decoding template for extensions now comes
directly from NSS.

+      return false;
+
+    // extnValue
+    base::StringPiece extn_value;
+    if (!GetElement(&extension, kOCTETSTRING, &extn_value))
+      return false;
+    if (!extension.empty())
+      return false;
+
+    // The value of this extension is an OCTET STRING:
+    // SignedCertificateTimestampList ::= OCTET STRING
+    base::StringPiece sct_list;
+    if (!GetElement(&extn_value, kOCTETSTRING, &sct_list))
+      return false;
+    *sct_list_out = sct_list;
+    break;
+  }
+
+  // Either we found the right extension or we looped through all of them
+  // without success. The latter is not an error because the presence of an
+  // SCT extension is optional.
+  return true;
+}
+
 } // namespace asn1
 
 } // namespace net