Adding OCSP Verification

net/cert/internal/ocsp.cc

Index: net/cert/internal/ocsp.cc
diff --git a/net/cert/internal/parse_ocsp.cc b/net/cert/internal/ocsp.cc
similarity index 70%
rename from net/cert/internal/parse_ocsp.cc
rename to net/cert/internal/ocsp.cc
index e06b29abe616346278fa917ea2e43073448ed192..24464e023311a6a2adad609233bc8823e09af3d4 100644
--- a/net/cert/internal/parse_ocsp.cc
+++ b/net/cert/internal/ocsp.cc
@@ -2,11 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "net/cert/internal/ocsp.h"
+
 #include <algorithm>
 
 #include "base/sha1.h"
 #include "crypto/sha2.h"
-#include "net/cert/internal/parse_ocsp.h"
+#include "net/cert/internal/extended_key_usage.h"
+#include "net/cert/internal/verify_name_match.h"
+#include "net/cert/internal/verify_signed_data.h"
 
 namespace net {
 
@@ -22,7 +26,7 @@ OCSPResponseData::~OCSPResponseData() {}
 OCSPResponse::OCSPResponse() {}
 OCSPResponse::~OCSPResponse() {}
 
-der::Input BasicOCSPResponseOid() {
+WARN_UNUSED_RESULT der::Input BasicOCSPResponseOid() {
   // From RFC 6960:
   //
   // id-pkix-ocsp           OBJECT IDENTIFIER ::= { id-ad-ocsp }
@@ -40,7 +44,8 @@ der::Input BasicOCSPResponseOid() {
 //    issuerKeyHash           OCTET STRING, -- Hash of issuer's public key
 //    serialNumber            CertificateSerialNumber
 // }
-bool ParseOCSPCertID(const der::Input& raw_tlv, OCSPCertID* out) {
+WARN_UNUSED_RESULT bool ParseOCSPCertID(const der::Input& raw_tlv,
+                                        OCSPCertID* out) {
   der::Parser outer_parser(raw_tlv);
   der::Parser parser;
   if (!outer_parser.ReadSequence(&parser))
@@ -75,7 +80,8 @@ namespace {
 //      revocationTime              GeneralizedTime,
 //      revocationReason    [0]     EXPLICIT CRLReason OPTIONAL
 // }
-bool ParseRevokedInfo(const der::Input& raw_tlv, OCSPCertStatus* out) {
+WARN_UNUSED_RESULT bool ParseRevokedInfo(const der::Input& raw_tlv,
+                                         OCSPCertStatus* out) {
   der::Parser parser(raw_tlv);
   if (!parser.ReadGeneralizedTime(&(out->revocation_time)))
     return false;
@@ -118,7 +124,8 @@ bool ParseRevokedInfo(const der::Input& raw_tlv, OCSPCertStatus* out) {
 // }
 //
 // UnknownInfo ::= NULL
-bool ParseCertStatus(const der::Input& raw_tlv, OCSPCertStatus* out) {
+WARN_UNUSED_RESULT bool ParseCertStatus(const der::Input& raw_tlv,
+                                        OCSPCertStatus* out) {
   der::Parser parser(raw_tlv);
   der::Tag status_tag;
   der::Input status;
@@ -150,8 +157,8 @@ bool ParseCertStatus(const der::Input& raw_tlv, OCSPCertStatus* out) {
 //      nextUpdate         [0]       EXPLICIT GeneralizedTime OPTIONAL,
 //      singleExtensions   [1]       EXPLICIT Extensions OPTIONAL
 // }
-bool ParseOCSPSingleResponse(const der::Input& raw_tlv,
-                             OCSPSingleResponse* out) {
+WARN_UNUSED_RESULT bool ParseOCSPSingleResponse(const der::Input& raw_tlv,
+                                                OCSPSingleResponse* out) {
   der::Parser outer_parser(raw_tlv);
   der::Parser parser;
   if (!outer_parser.ReadSequence(&parser))
@@ -199,8 +206,8 @@ namespace {
 //      byName               [1] Name,
 //      byKey                [2] KeyHash
 // }
-bool ParseResponderID(const der::Input& raw_tlv,
-                      OCSPResponseData::ResponderID* out) {
+WARN_UNUSED_RESULT bool ParseResponderID(const der::Input& raw_tlv,
+                                         OCSPResponseData::ResponderID* out) {
   der::Parser parser(raw_tlv);
   der::Tag id_tag;
   der::Input id_input;
@@ -239,7 +246,8 @@ bool ParseResponderID(const der::Input& raw_tlv,
 //      responses                SEQUENCE OF SingleResponse,
 //      responseExtensions   [1] EXPLICIT Extensions OPTIONAL
 // }
-bool ParseOCSPResponseData(const der::Input& raw_tlv, OCSPResponseData* out) {
+WARN_UNUSED_RESULT bool ParseOCSPResponseData(const der::Input& raw_tlv,
+                                              OCSPResponseData* out) {
   der::Parser outer_parser(raw_tlv);
   der::Parser parser;
   if (!outer_parser.ReadSequence(&parser))
@@ -309,7 +317,8 @@ namespace {
 //      signature            BIT STRING,
 //      certs            [0] EXPLICIT SEQUENCE OF Certificate OPTIONAL
 // }
-bool ParseBasicOCSPResponse(const der::Input& raw_tlv, OCSPResponse* out) {
+WARN_UNUSED_RESULT bool ParseBasicOCSPResponse(const der::Input& raw_tlv,
+                                               OCSPResponse* out) {
   der::Parser outer_parser(raw_tlv);
   der::Parser parser;
   if (!outer_parser.ReadSequence(&parser))
@@ -363,7 +372,8 @@ bool ParseBasicOCSPResponse(const der::Input& raw_tlv, OCSPResponse* out) {
 //      responseType   OBJECT IDENTIFIER,
 //      response       OCTET STRING
 // }
-bool ParseOCSPResponse(const der::Input& raw_tlv, OCSPResponse* out) {
+WARN_UNUSED_RESULT bool ParseOCSPResponse(const der::Input& raw_tlv,
+                                          OCSPResponse* out) {
   der::Parser outer_parser(raw_tlv);
   der::Parser parser;
   if (!outer_parser.ReadSequence(&parser))
@@ -420,9 +430,9 @@ bool ParseOCSPResponse(const der::Input& raw_tlv, OCSPResponse* out) {
 namespace {
 
 // Checks that the |type| hash of |value| is equal to |hash|
-bool VerifyHash(HashValueTag type,
-                const der::Input& hash,
-                const der::Input& value) {
+WARN_UNUSED_RESULT bool VerifyHash(HashValueTag type,
+                                   const der::Input& hash,
+                                   const der::Input& value) {
   HashValue target(type);
   if (target.size() != hash.Length())
     return false;
@@ -443,10 +453,10 @@ bool VerifyHash(HashValueTag type,
 
 // Checks that the input |id_tlv| parses to a valid CertID and matches the
 // issuer |issuer| name and key, as well as the serial number |serial_number|.
-bool CheckCertID(const der::Input& id_tlv,
-                 const ParsedTbsCertificate& certificate,
-                 const ParsedTbsCertificate& issuer,
-                 const der::Input& serial_number) {
+WARN_UNUSED_RESULT bool CheckCertID(const der::Input& id_tlv,
+                                    const ParsedTbsCertificate& certificate,
+                                    const ParsedTbsCertificate& issuer,
+                                    const der::Input& serial_number) {
   OCSPCertID id;
   if (!ParseOCSPCertID(id_tlv, &id))
     return false;
@@ -492,10 +502,10 @@ bool CheckCertID(const der::Input& id_tlv,
 
 }  // namespace
 
-bool GetOCSPCertStatus(const OCSPResponseData& response_data,
-                       const ParsedCertificate& issuer,
-                       const ParsedCertificate& cert,
-                       OCSPCertStatus* out) {
+WARN_UNUSED_RESULT bool GetOCSPCertStatus(const OCSPResponseData& response_data,
+                                          const ParsedCertificate& issuer,
+                                          const ParsedCertificate& cert,
+                                          OCSPCertStatus* out) {
   out->status = OCSPCertStatus::Status::GOOD;
 
   ParsedTbsCertificate tbs_cert;
@@ -529,4 +539,117 @@ bool GetOCSPCertStatus(const OCSPResponseData& response_data,
   return found;
 }
 
+namespace {
+
+// Checks that the ResponderID |id| matches the certificate |cert| either
+// by verifying the name matches that of the certificate or that the hash
+// matches the certificate's public key hash (RFC 6960, 4.2.2.3).
+WARN_UNUSED_RESULT bool CheckResponder(const OCSPResponseData::ResponderID& id,

[eroman, 2016/05/31 19:12:46]

Can you use a more specific name -- how about ResponderIdMatches(ID, Cert), to
reflect what check on the responder it is doing? (In particular it was confusing
when seeing the caller, as I thought the actual responder check that the "else"
branch was doing would go here).

+                                       const ParsedTbsCertificate& cert) {
+  if (id.type == OCSPResponseData::ResponderType::NAME) {
+    der::Input name_rdn;
+    der::Input cert_rdn;
+    if (!der::Parser(id.name).ReadTag(der::kSequence, &name_rdn) ||

[eroman, 2016/05/31 19:12:46]

Can you add documentation for ResponderID indicating this assumption? (i.e. that
ResponderID after parsing is a single SEQUENCE?)

+        !der::Parser(cert.subject_tlv).ReadTag(der::kSequence, &cert_rdn))
+      return false;
+    return VerifyNameMatch(name_rdn, cert_rdn);
+  }

[eroman, 2016/05/31 19:12:46]

For future proofing my preference is a switch(). But could also use this
structure and add an else or a DCHECK() for the remaining possibility for
id.type.

+
+  // Otherwise we extract the SPKI and compare the public key hash to key

[eroman, 2016/05/31 19:12:46]

avoid "we" in comments.

+  // in the ResponderID.
+  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
+
+WARN_UNUSED_RESULT bool VerifyOCSPResponse(
+    const OCSPResponse& response,
+    const ParsedCertificate& issuer_cert,
+    const SignaturePolicy& signature_policy) {
+  OCSPResponseData response_data;
+  if (!ParseOCSPResponseData(response.data, &response_data))
+    return false;
+
+  ParsedTbsCertificate issuer;
+  ParsedTbsCertificate responder;
+  if (!ParseTbsCertificate(issuer_cert.tbs_certificate_tlv, &issuer))
+    return false;
+
+  // In order to verify the OCSP signature, a valid responder matching the OCSP
+  // Responder ID must be located (RFC 6960, 4.2.2.2). The responder is allowed
+  // to be either the certificate issuer or a delegated authority directly
+  // signed by the issuer.
+  if (CheckResponder(response_data.responder_id, issuer)) {
+    responder = issuer;
+  } else {
+    bool found = false;
+    // If the authority to sign OCSP responses has been delegated to another
+    // authority, the OCSP Response will contain the responder certificate.

[eroman, 2016/05/31 19:12:46]

Is there any normative reference for this logic?

I couldn't find it in 6960 at least, which just said:

   The responder MAY include certificates in the certs field of
   BasicOCSPResponse that help the OCSP client verify the responder's
   signature.

Which says nothing about their ordering, validity, or interpretation.

This code checks only signatures of certificates (nothing else, including
presence of critical unsupported extensions).

Also, should it permit a chain of certs from the issuer to the responder? (here
the responder is assumed to be directly signed by issuer).

+    for (const auto& responder_cert_tlv : response.certs) {
+      ParsedCertificate responder_cert;
+      ParsedTbsCertificate tbs_cert;
+      if (!ParseCertificate(responder_cert_tlv, &responder_cert))
+        return false;
+      if (!ParseTbsCertificate(responder_cert.tbs_certificate_tlv, &tbs_cert))
+        return false;
+
+      // The OCSP Responder ID is checked against each provided certificate to
+      // determine whether it is the correct Authorized Responder.
+      if (CheckResponder(response_data.responder_id, tbs_cert)) {
+        found = true;
+        responder = tbs_cert;
+
+        // The Authorized Responder must be directly signed by the issuer of the
+        // certificate being checked.
+        std::unique_ptr<SignatureAlgorithm> signature_algorithm =
+            SignatureAlgorithm::CreateFromDer(
+                responder_cert.signature_algorithm_tlv);
+        if (!signature_algorithm)
+          return false;
+        der::Input issuer_spki = issuer.spki_tlv;
+        if (!VerifySignedData(*signature_algorithm,
+                              responder_cert.tbs_certificate_tlv,
+                              responder_cert.signature_value, issuer_spki,
+                              &signature_policy)) {
+          return false;
+        }
+
+        // The Authorized Responder must include the value id-kp-OCSPSigning as
+        // part of the extended key usage extension.
+        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;
+
+        // TODO(svaldez): Add Revocation Checking of Authorized Responder.
+        break;

[eroman, 2016/05/31 19:12:46]

Can you pull this out to a separate function instead of the "break" and "found"
logic?

+      }
+    }
+    if (!found)
+      return false;
+  }
+  // Once a valid responder has been found, the signature of the OCSP
+  // response is verified against the public key of the responder.
+  return VerifySignedData(*(response.signature_algorithm), response.data,
+                          response.signature, responder.spki_tlv,
+                          &signature_policy);
+}
+
 }  // namespace net