Adding OCSP Parser

net/cert/ocsp_parser.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.

[eroman, 2016/02/03 22:54:44]

can change to 2016

[svaldez, 2016/02/04 19:03:24]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <algorithm>
+
+#include "base/base64.h"
+#include "base/sha1.h"
+#include "net/cert/internal/extended_key_usage.h"
+#include "net/cert/internal/signature_policy.h"
+#include "net/cert/internal/verify_name_match.h"
+#include "net/cert/internal/verify_signed_data.h"
+#include "net/cert/ocsp_parser.h"
+
+namespace net {
+
+namespace cert {
+
+OCSPSingleResponse::OCSPSingleResponse() {}
+OCSPSingleResponse::~OCSPSingleResponse() {}
+
+OCSPResponseData::OCSPResponseData() {}
+OCSPResponseData::~OCSPResponseData() {}
+
+OCSPResponse::OCSPResponse() {}
+OCSPResponse::~OCSPResponse() {}
+
+der::Input BasicOCSPResponseOid() {
+  // From RFC 6960:
+  //
+  // id-pkix-ocsp           OBJECT IDENTIFIER ::= { id-ad-ocsp }
+  // id-pkix-ocsp-basic     OBJECT IDENTIFIER ::= { id-pkix-ocsp 1 }
+  //
+  // In dotted notation: 1.3.6.1.5.5.7.48.1.1
+  static const uint8_t oid[] = {0x2b, 0x06, 0x01, 0x05, 0x05,
+                                0x07, 0x30, 0x01, 0x01};
+  return der::Input(oid);
+}
+
+bool ParseOCSPSingleResponse(der::Input raw_tlv, OCSPSingleResponse* out) {

[eroman, 2016/02/03 22:54:45]

Please provide comments somewhere that reflect the structure being parsed.
Preferably at the top of the function, and optionally also inline when reading
the various parts:

SingleResponse ::= SEQUENCE {
   certID                  CertID,
   certStatus              CertStatus,
   thisUpdate              GeneralizedTime,
   nextUpdate          [0] EXPLICIT GeneralizedTime OPTIONAL,
   singleExtensions    [1] EXPLICIT Extensions OPTIONAL }

[svaldez, 2016/02/04 19:03:24]

Should this be both here and in the header file, or is the header sufficient?

+  der::Parser response_parser(raw_tlv);
+  der::Parser parser;
+  if (!response_parser.ReadSequence(&parser))

[eroman, 2016/02/03 22:54:45]

It doesn't look like unconsumed data is being checked for. In other words
shouldn't ParseOCSPSingleResponse fail if:

  * There is something after the sequence in |raw_tlv|
  * There is something after singleExtensions

Other code handles this by checking HasMore() in both places, unless there are
explicit extension points in the structure.

[svaldez, 2016/02/04 19:03:24]

Done.

+    return false;
+
+  der::Input cert_id;
+  if (!parser.ReadTag(der::kSequence, &cert_id))

[eroman, 2016/02/03 22:54:45]

The status parsing is a good candidate to move to a separate helper function.

[svaldez, 2016/02/04 19:03:24]

Done.

+    return false;
+  out->cert_id = cert_id.AsString();
+  der::Tag status_tag;
+  der::Input status;
+  if (!parser.ReadTagAndValue(&status_tag, &status))
+    return false;
+  if (status_tag == der::ContextSpecificPrimitive(0)) {
+    out->cert_status = OCSPSingleResponse::CERT_GOOD;
+  } else if (status_tag == der::ContextSpecificPrimitive(1)) {
+    out->cert_status = OCSPSingleResponse::CERT_REVOKED;
+    der::Parser revoked_info_parser(status);

[eroman, 2016/02/03 22:54:45]

Same issue here, what if there is unconsumed data at the end?

[svaldez, 2016/02/04 19:03:24]

Done.

+    der::Input revocation_time;
+    der::Input revocation_reason;
+    if (!revoked_info_parser.ReadGeneralizedTime(&(out->revocation_time)))
+      return false;
+    if (!revoked_info_parser.ReadTag(der::kEnumerated, &revocation_reason))
+      return false;
+    uint8_t revocation_reason_value;
+    if (!der::ParseUint8(revocation_reason, &revocation_reason_value))
+      return false;
+    out->revocation_reason = static_cast<OCSPSingleResponse::RevocationReason>(

[eroman, 2016/02/03 22:54:45]

This needs to ensure that the number is in range, or else use a generic integer
instead of a RevocationReason (since it is unexpected to end up with a value
that isn't part of enum).

[svaldez, 2016/02/04 19:03:24]

Done.

+        revocation_reason_value);
+  } else if (status_tag == der::ContextSpecificPrimitive(2)) {
+    out->cert_status = OCSPSingleResponse::CERT_UNKNOWN;

[eroman, 2016/02/03 22:54:44]

In the case of !CERT_REVOKED would be a good idea to reset the fields that are
not applicable in case they are mistakenly accessed. The documentation should
also make clear that those fields are not to be used unless if the status is
revoked.

[svaldez, 2016/02/04 19:03:24]

Done.

+  } else {
+    return false;
+  }
+
+  der::Input this_update;
+  if (!parser.ReadGeneralizedTime(&(out->this_update)))
+    return false;
+  der::Input next_update_input;
+  bool next_update_present;
+  if (!parser.ReadOptionalTag(der::ContextSpecificConstructed(0),
+                              &next_update_input, &next_update_present)) {
+    return false;
+  }
+
+  if (next_update_present) {
+    der::Parser next_update_parser(next_update_input);

[eroman, 2016/02/03 22:54:45]

Same issue here, this should fail if there was unconsumed data after the
generalized time

[svaldez, 2016/02/04 19:03:24]

Done.

+    der::Input next_update;
+    if (!next_update_parser.ReadGeneralizedTime(&(out->next_update)))

[eroman, 2016/02/03 22:54:45]

How does a caller distinguish when it was present? I don't see that next_update
is reset in the case where it was not parsed.

[svaldez, 2016/02/04 19:03:24]

Done.

Adding has_... for all optional fields.

+      return false;
+  }
+  bool extensions_present;
+  if (!parser.ReadOptionalTag(der::ContextSpecificConstructed(1),
+                              &(out->extensions), &extensions_present)) {

[eroman, 2016/02/03 22:54:45]

Same question here: how do callers of the function distinguish when there were
extensions or not? Also unless the expectation is that |out| is a default
initialized value, then out->extensions probably needs to be reset in the case
where it is not present. An empty input though would still be ambiguous as to
whether it was simply not present, or whether it was present but empty (which I
believe is invalid per the definition of extensions)

[svaldez, 2016/02/04 19:03:24]

Done.

+    return false;
+  }
+
+  return true;
+}
+
+bool ParseOCSPResponseData(der::Input raw_tlv, OCSPResponseData* out) {

[eroman, 2016/02/03 22:54:45]

Document the structure being parsed, referencing the RFC

[svaldez, 2016/02/04 19:03:24]

Acknowledged.

+  der::Parser parser;
+  if (!der::Parser(raw_tlv).ReadSequence(&parser))

[eroman, 2016/02/03 22:54:44]

Fail on unconsumed data?

[svaldez, 2016/02/04 19:03:24]

Done.

+    return false;
+
+  der::Input version_input;
+  bool version_present;
+  if (!parser.ReadOptionalTag(der::ContextSpecificConstructed(0),
+                              &version_input, &version_present)) {
+    return false;
+  }
+
+  if (version_present) {
+    der::Parser version_parser(version_input);
+    der::Input version;
+    if (!version_parser.ReadTag(der::kInteger, &version))
+      return false;
+    if (!der::ParseUint8(version, &(out->version)))

[eroman, 2016/02/03 22:54:44]

Because the version is DEFAULT v1, a strict DER parser should also reject if the
value was provided as 0 (since DEFAULT values are not to be encoded.

As I recall there are a number of bugs in the wild around this though so
compatibility wise will likely need to allow it. But should call this out in the
code at least.

[svaldez, 2016/02/04 19:03:24]

Done.

+      return false;
+  } else {
+    out->version = 0;
+  }
+
+  der::Tag id_tag;
+  der::Input responder_id_input;
+  OCSPResponseData::ResponderID responder_id;
+  if (!parser.ReadTagAndValue(&id_tag, &responder_id_input))
+    return false;
+  if (id_tag == der::ContextSpecificConstructed(1)) {
+    responder_id.type = OCSPResponseData::NAME;
+    responder_id.name = responder_id_input;
+  } else if (id_tag == der::ContextSpecificConstructed(2)) {
+    der::Parser key_parser(responder_id_input);
+    der::Input responder_key;
+    if (!key_parser.ReadTag(der::kOctetString, &responder_key))
+      return false;
+
+    if (responder_key.Length() != base::kSHA1Length)

[eroman, 2016/02/03 22:54:44]

given the memcpy below, I would say compare the length against
sizeof(key_hash.data) instead to make it clear there isn't possibility of
overrun.

[svaldez, 2016/02/04 19:03:24]

Done.

+      return false;
+    SHA1HashValue key_hash;
+    memcpy(key_hash.data, responder_key.AsString().data(), base::kSHA1Length);

[eroman, 2016/02/03 22:54:44]

Unnecessary to call AsString() that will construct a temporary throw-away
string. Instead I guess UnsafeData().

[svaldez, 2016/02/04 19:03:24]

Done.

+    responder_id.type = OCSPResponseData::KEY_HASH;
+    responder_id.key_hash = HashValue(key_hash);
+  } else {
+    return false;
+  }
+  out->responder_id = responder_id;
+
+  der::Input produced_at;
+  if (!parser.ReadGeneralizedTime(&(out->produced_at)))
+    return false;
+
+  der::Parser responses_parser;
+  if (!parser.ReadSequence(&responses_parser))
+    return false;
+  while (responses_parser.HasMore()) {
+    der::Input single_response;
+    if (!responses_parser.ReadRawTLV(&single_response))
+      return false;
+    out->responses.push_back(single_response);

[eroman, 2016/02/03 22:54:44]

Should out->responses not be cleared before starting? (Documentation doesn't say
that the output must be default initialized.) Implication will be if re-using an
object while parsing, will get left-over goop from previous run.

[svaldez, 2016/02/04 19:03:24]

Done.

+  }
+
+  bool extensions_present;
+  if (!parser.ReadOptionalTag(der::ContextSpecificConstructed(1),
+                              &(out->extensions), &extensions_present)) {

[eroman, 2016/02/03 22:54:45]

Same question here about disambiguating present and empty vs omitted.

Is Extensions here the same as from RFC  5280? Because we already expose a
function for parsing that.

[svaldez, 2016/02/04 19:03:24]

Done.

+    return false;
+  }
+  return true;
+}
+
+bool ParseOCSPResponse(der::Input ocsp_response, OCSPResponse* out) {

[eroman, 2016/02/03 22:54:45]

document the structure being parsed and reference to RFC

[svaldez, 2016/02/04 19:03:24]

Acknowledged.

+  der::Parser parser(ocsp_response);

[eroman, 2016/02/03 22:54:45]

What about unconsumed data?

[svaldez, 2016/02/04 19:03:24]

Done.

+  der::Input response_status;
+  der::Parser ocsp_response_parser;
+  if (!parser.ReadSequence(&ocsp_response_parser))
+    return false;
+  if (!ocsp_response_parser.ReadTag(der::kEnumerated, &response_status))
+    return false;
+  uint8_t response_status_value;
+  if (!der::ParseUint8(response_status, &response_status_value))
+    return false;
+  out->status =
+      static_cast<OCSPResponse::ResponseStatus>(response_status_value);
+  if (out->status != OCSPResponse::SUCCESSFUL)

[eroman, 2016/02/03 22:54:44]

Why is this a failure?

[svaldez, 2016/02/04 19:03:24]

It isn't. "return true" since we don't have to parse the rest of the data.

+    return true;
+
+  der::Input response_type_oid;
+  der::Input response_string;
+  der::Parser response_bytes_input_parser;
+  der::Parser response_bytes_parser;
+  if (!ocsp_response_parser.ReadConstructed(der::ContextSpecificConstructed(0),
+                                            &response_bytes_input_parser)) {

[eroman, 2016/02/03 22:54:44]

What about unconsumed data?

[svaldez, 2016/02/04 19:03:24]

Done.

+    return false;
+  }
+  if (!response_bytes_input_parser.ReadSequence(&response_bytes_parser))
+    return false;
+  if (!response_bytes_parser.ReadTag(der::kOid, &response_type_oid))
+    return false;
+  if (!response_type_oid.Equals(BasicOCSPResponseOid()))
+    return false;
+  if (!response_bytes_parser.ReadTag(der::kOctetString, &response_string))
+    return false;
+
+  der::Parser response_parser(response_string);

[eroman, 2016/02/03 22:54:45]

What about unconsumed data?

[svaldez, 2016/02/04 19:03:24]

Done.

+  der::Parser basic_response_parser;
+  der::Parser response_data_parser;
+  der::Input sigalg_tlv;

[eroman, 2016/02/03 22:54:44]

This function is big. It needs some documentation to guide what parts are being
parsed and what the corresponding ASN.1 is, or break it up into smaller
functions for the sub-parts.

+  if (!response_parser.ReadSequence(&basic_response_parser))
+    return false;
+  if (!basic_response_parser.ReadRawTLV(&(out->data)))
+    return false;
+  if (!basic_response_parser.ReadRawTLV(&sigalg_tlv))
+    return false;
+  out->signature_algorithm = SignatureAlgorithm::CreateFromDer(sigalg_tlv);
+
+  if (!basic_response_parser.ReadBitString(&(out->signature)))
+    return false;
+  der::Input certs_input;
+  bool certs_present;
+  if (!basic_response_parser.ReadOptionalTag(der::ContextSpecificConstructed(0),
+                                             &certs_input, &certs_present)) {
+    return false;
+  }
+
+  if (!certs_present)
+    return true;
+  der::Parser certs_input_parser(certs_input);

[eroman, 2016/02/03 22:54:44]

Unconsumed data?

[eroman, 2016/02/03 22:54:45]

Same comment as previously. (It is hard to follow which sub-parts are being
parsed in this function)

[svaldez, 2016/02/04 19:03:24]

Done.

+  der::Parser certs_parser;
+  if (!certs_input_parser.ReadSequence(&certs_parser))
+    return false;
+  while (certs_parser.HasMore()) {
+    ParsedCertificate parsed_cert;
+    der::Input cert_tlv;
+    if (!certs_parser.ReadRawTLV(&cert_tlv))
+      return false;
+    if (!ParseCertificate(cert_tlv, &parsed_cert))

[eroman, 2016/02/03 22:54:44]

I am not convinced that parsing the certificates should be done here.

+      return false;
+    out->certs.push_back(parsed_cert);
+  }
+  return true;
+}
+
+namespace {
+
+bool CheckResponder(OCSPResponseData::ResponderID id,
+                    ParsedTbsCertificate cert) {
+  if (id.type == OCSPResponseData::NAME) {
+    return VerifyNameMatch(id.name, cert.issuer_tlv);
+  } else {
+    der::Parser parser(cert.spki_tlv);
+    der::Parser spki_parser;
+    der::BitString key_bits;
+    if (!parser.ReadSequence(&spki_parser))
+      return false;
+    if (!spki_parser.SkipTag(der::kSequence))
+      return false;
+    if (!spki_parser.ReadBitString(&key_bits))
+      return false;
+
+    der::Input key = key_bits.bytes();
+    HashValue key_hash(HASH_VALUE_SHA1);
+    base::SHA1HashBytes(key.UnsafeData(), key.Length(), key_hash.data());
+    return key_hash.Equals(id.key_hash);
+  }
+}
+
+}  // namespace
+
+bool VerifyOCSPResponse(OCSPResponse* response, ParsedCertificate* cert) {

[eroman, 2016/02/03 22:54:44]

I didn't review this function yet, but I am skeptical about how it re-implements
stuff from VerifyCertificateChain. I imagine it should be calling that instead
with a mode for indicating it is a CRL.

[svaldez, 2016/02/04 19:03:24]

Unfortunately the certificate "chain" used for OCSP handling has very different
rules from normal chain verification. The chain attached to the OCSPResponse is
allowed to be an arbitrary set of certs, and we are only allowed to chain one
step from the issuer of the certificate.

+  SimpleSignaturePolicy signature_policy(1024);
+
+  OCSPResponseData response_data;
+  if (!ParseOCSPResponseData(response->data, &response_data))
+    return false;
+
+  ParsedTbsCertificate issuer;
+  ParsedTbsCertificate responder;
+  if (!cert || !ParseTbsCertificate(cert->tbs_certificate_tlv, &issuer))
+    return false;
+
+  if (CheckResponder(response_data.responder_id, issuer)) {
+    responder = issuer;
+  } else {
+    bool found = false;
+    for (const auto& responder_cert : response->certs) {
+      ParsedTbsCertificate tbs_cert;
+      if (!ParseTbsCertificate(responder_cert.tbs_certificate_tlv, &tbs_cert))
+        return false;
+
+      if (CheckResponder(response_data.responder_id, tbs_cert)) {
+        found = true;
+        responder = tbs_cert;
+
+        scoped_ptr<SignatureAlgorithm> signature_algorithm =
+            SignatureAlgorithm::CreateFromDer(
+                responder_cert.signature_algorithm_tlv);
+        der::Input issuer_spki = issuer.spki_tlv;
+        if (!VerifySignedData(*(signature_algorithm.get()),
+                              responder_cert.tbs_certificate_tlv,
+                              responder_cert.signature_value, issuer_spki,
+                              &signature_policy)) {
+          return false;
+        }
+
+        std::map<der::Input, ParsedExtension> extensions;
+        std::vector<der::Input> eku;
+        if (!ParseExtensions(responder.extensions_tlv, &extensions))
+          return false;
+        if (!ParseEKUExtension(extensions[ExtKeyUsageOid()].value, &eku))
+          return false;
+        if (std::find(eku.begin(), eku.end(), OCSPSigning()) == eku.end())
+          return false;
+        break;
+      }
+    }
+    if (!found)
+      return false;
+  }
+
+  return VerifySignedData(*(response->signature_algorithm.get()),
+                          response->data, response->signature,
+                          responder.spki_tlv, &signature_policy);
+}
+
+}  // namespace cert
+
+}  // namespace net