Adding OCSP Verification

net/cert/internal/parse_ocsp.cc

Index: net/cert/internal/parse_ocsp.cc
diff --git a/net/cert/internal/parse_ocsp.cc b/net/cert/internal/parse_ocsp.cc
index e06b29abe616346278fa917ea2e43073448ed192..c899d6116fc399d7530c29e43076cd7901a70da8 100644
--- a/net/cert/internal/parse_ocsp.cc
+++ b/net/cert/internal/parse_ocsp.cc
@@ -2,11 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "net/cert/internal/parse_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/signature_policy.h"
+#include "net/cert/internal/verify_name_match.h"
+#include "net/cert/internal/verify_signed_data.h"
 
 namespace net {
 
@@ -529,4 +534,97 @@ bool GetOCSPCertStatus(const OCSPResponseData& response_data,
   return found;
 }
 
+namespace {
+
+// Checks that the ResponderID |id| matches the certificate |cert|.

[eroman, 2016/05/02 22:36:50]

Same comment here -- justify what parts of the standard this is implementing.

[svaldez, 2016/05/04 15:31:20]

Done.

+bool CheckResponder(const OCSPResponseData::ResponderID& id,
+                    const ParsedTbsCertificate& cert) {

[eroman, 2016/05/02 22:36:50]

WARN_UNUSED_RESULT

[svaldez, 2016/05/04 15:31:19]

Done.

+  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/02 22:36:50]

Is this sufficient?
The assumption is that id.name is a single SEQUENCE right?
However it doesn't look like the parsing code for ResponderID enforces that
(i.e. I believe there can be unconsumed in it here).

[svaldez, 2016/05/04 15:31:19]

The outer name I believe is guaranteed to be a single sequence containing a
bunch of RDNs.

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

[eroman, 2016/05/02 22:36:50]

don't include the else after a short-circuit.

Also would this make more sense as a switch rather than if/else, in case the
enum for for some reason changed?

[svaldez, 2016/05/04 15:31:19]

Fixed. Due to the lack of extensability with the ASN.1 structure here, we're
guaranteed for this not to change.

+    der::Parser parser(cert.spki_tlv);
+    der::Parser spki_parser;
+    der::BitString key_bits;
+    if (!parser.ReadSequence(&spki_parser))

[eroman, 2016/05/02 22:36:50]

Same thing as elsewhere -- describe intent.
Looks like you are doing SPKI parsing here.

[svaldez, 2016/05/04 15:31:19]

Done.

+      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);

[eroman, 2016/05/02 22:36:51]

I am not familiar with OCSP (beyond what I read to review this CL just now)...
is this a problem that the protocol implicitly uses SHA1 here?

[svaldez, 2016/05/04 15:31:20]

This is safe since we are only using the ResponderID to look up the correct
certificate, instead of any sort of verification. If there's a collision
(accidental or malicious), an attacker would also need to forge the actual OCSP
response signature from an *authorized* responder.

+    base::SHA1HashBytes(key.UnsafeData(), key.Length(), key_hash.data());
+    return key_hash.Equals(id.key_hash);
+  }
+}
+
+}  // namespace
+
+bool VerifyOCSPResponse(const OCSPResponse& response,

[eroman, 2016/05/02 22:36:51]

Please documentation throughout this implementation that references the
normative requirements in the standard these are implementing.

This is hard to read as written since I don't know what parts of the standard it
is aiming to implement, or review for omissions.

[svaldez, 2016/05/04 15:31:20]

Done.

+                        const ParsedCertificate& issuer_cert) {
+  SimpleSignaturePolicy signature_policy(1024);

[eroman, 2016/05/02 22:36:51]

Why this signature policy? 1024-bit RSA is weak. Does this need to be baked in
here, or user supplied?

[svaldez, 2016/05/04 15:31:19]

Done.

+
+  OCSPResponseData response_data;

[eroman, 2016/05/02 22:36:50]

I didn't review the rest of this in detail -- will wait for spec references and
comments.

[svaldez, 2016/05/04 15:31:19]

Done.

+  if (!ParseOCSPResponseData(response.data, &response_data))
+    return false;
+
+  ParsedTbsCertificate issuer;
+  ParsedTbsCertificate responder;
+  if (!ParseTbsCertificate(issuer_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_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;
+
+      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);
+        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;
+        }
+
+        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), response.data,
+                          response.signature, responder.spki_tlv,
+                          &signature_policy);
+}
+
 }  // namespace net