Update CertVerifier::Verify to use RequestParams instead

CertVerifier::Verify() has some inconsistencies with
respect to how it handles additional trust anchors, which
complicates the layering and the caching.

Rather than just add an additional parameter to Verify(),
convert everything to use RequestParams, which also defines how
the caching/hashing is done.

BUG=612655
TBR=brettw

Review-Url: https://codereview.chromium.org/1994353002
Cr-Commit-Position: refs/heads/master@{#398707}

blimp/net/exact_match_cert_verifier.cc

@@ -33,10 +33,7 @@ ExactMatchCertVerifier::ExactMatchCertVerifier(
 
 ExactMatchCertVerifier::~ExactMatchCertVerifier() {}
 
-int ExactMatchCertVerifier::Verify(net::X509Certificate* cert,
-                                   const std::string& hostname,
-                                   const std::string& ocsp_response,
-                                   int flags,
+int ExactMatchCertVerifier::Verify(const RequestParams& params,
                                    net::CRLSet* crl_set,
                                    net::CertVerifyResult* verify_result,
                                    const net::CompletionCallback& callback,
@@ -45,7 +42,7 @@ int ExactMatchCertVerifier::Verify(net::X509Certificate* cert,
   verify_result->Reset();
   verify_result->verified_cert = engine_cert_;
 
-  if (!cert->Equals(engine_cert_.get())) {
+  if (!params.certificate()->Equals(engine_cert_.get())) {
     verify_result->cert_status = net::CERT_STATUS_INVALID;
     return net::ERR_CERT_INVALID;
   }

blimp/net/exact_match_cert_verifier.h

@@ -28,10 +28,7 @@ class BLIMP_NET_EXPORT ExactMatchCertVerifier : public net::CertVerifier {
   ~ExactMatchCertVerifier() override;
 
   // net::CertVerifier implementation.
-  int Verify(net::X509Certificate* cert,
-             const std::string& hostname,
-             const std::string& ocsp_response,
-             int flags,
+  int Verify(const RequestParams& params,
              net::CRLSet* crl_set,
              net::CertVerifyResult* verify_result,
              const net::CompletionCallback& callback,

chrome/browser/chromeos/policy/policy_cert_verifier.cc

@@ -69,10 +69,7 @@ void PolicyCertVerifier::SetTrustAnchors(
 }
 
 int PolicyCertVerifier::Verify(
-    net::X509Certificate* cert,
-    const std::string& hostname,
-    const std::string& ocsp_response,
-    int flags,
+    const RequestParams& params,
     net::CRLSet* crl_set,
     net::CertVerifyResult* verify_result,
     const net::CompletionCallback& completion_callback,
@@ -85,9 +82,8 @@ int PolicyCertVerifier::Verify(
                  anchor_used_callback_,
                  completion_callback,
                  verify_result);
-  int error =
-      delegate_->Verify(cert, hostname, ocsp_response, flags, crl_set,
-                        verify_result, wrapped_callback, out_req, net_log);
+  int error = delegate_->Verify(params, crl_set, verify_result,
+                                wrapped_callback, out_req, net_log);
   MaybeSignalAnchorUse(error, anchor_used_callback_, *verify_result);
   return error;
 }

chrome/browser/chromeos/policy/policy_cert_verifier.h

@@ -47,10 +47,7 @@ class PolicyCertVerifier : public net::CertVerifier,
 
   // CertVerifier:
   // Note: |callback| can be null.
-  int Verify(net::X509Certificate* cert,
-             const std::string& hostname,
-             const std::string& ocsp_response,
-             int flags,
+  int Verify(const RequestParams& params,
              net::CRLSet* crl_set,
              net::CertVerifyResult* verify_result,
              const net::CompletionCallback& callback,

chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc

@@ -71,9 +71,12 @@ class PolicyCertVerifierTest : public testing::Test {
       const net::TestCompletionCallback& test_callback,
       net::CertVerifyResult* verify_result,
       std::unique_ptr<net::CertVerifier::Request>* request) {
-    return cert_verifier_->Verify(
-        test_server_cert_.get(), "127.0.0.1", std::string(), 0, NULL,
-        verify_result, test_callback.callback(), request, net::BoundNetLog());
+    return cert_verifier_->Verify(net::CertVerifier::RequestParams(
+                                      test_server_cert_.get(), "127.0.0.1", 0,
+                                      std::string(), net::CertificateList()),
+                                  nullptr, verify_result,
+                                  test_callback.callback(), request,
+                                  net::BoundNetLog());
   }
 
   bool SupportsAdditionalTrustAnchors() {

chrome/browser/extensions/api/gcd_private/privet_v3_context_getter.cc

@@ -24,17 +24,14 @@ class PrivetV3ContextGetter::CertVerifier : public net::CertVerifier {
  public:
   CertVerifier() {}
 
-  int Verify(net::X509Certificate* cert,
-             const std::string& hostname,
-             const std::string& ocsp_response,
-             int flags,
+  int Verify(const RequestParams& params,
              net::CRLSet* crl_set,
              net::CertVerifyResult* verify_result,
              const net::CompletionCallback& callback,
              std::unique_ptr<Request>* out_req,
              const net::BoundNetLog& net_log) override {
     verify_result->Reset();
-    verify_result->verified_cert = cert;
+    verify_result->verified_cert = params.certificate();
 
     // Because no trust anchor checking is being performed, don't indicate that
     // it came from an OS-trusted root.
@@ -56,9 +53,10 @@ class PrivetV3ContextGetter::CertVerifier : public net::CertVerifier {
     // container clean.
     verify_result->public_key_hashes.clear();
 
-    verify_result->cert_status = CheckFingerprint(cert, hostname)
-                                     ? 0
-                                     : net::CERT_STATUS_AUTHORITY_INVALID;
+    verify_result->cert_status =
+        CheckFingerprint(params.certificate(), params.hostname())
+            ? 0
+            : net::CERT_STATUS_AUTHORITY_INVALID;
     return net::IsCertStatusError(verify_result->cert_status)
                ? net::MapCertStatusToNetError(verify_result->cert_status)
                : net::OK;
@@ -70,7 +68,7 @@ class PrivetV3ContextGetter::CertVerifier : public net::CertVerifier {
   }
 
  private:
-  bool CheckFingerprint(net::X509Certificate* cert,
+  bool CheckFingerprint(const scoped_refptr<net::X509Certificate>& cert,
                         const std::string& hostname) const {
     auto it = fingerprints_.find(hostname);
     if (it == fingerprints_.end())

chrome/browser/extensions/api/platform_keys/verify_trust_api.cc

@@ -196,7 +196,9 @@ void VerifyTrustAPI::IOPart::Verify(std::unique_ptr<Params> params,
                  base::Passed(&verify_result), base::Owned(request_state)));
 
   const int return_value = verifier->Verify(
-      cert_chain.get(), details.hostname, ocsp_response, flags,
+      net::CertVerifier::RequestParams(std::move(cert_chain), details.hostname,
+                                       flags, ocsp_response,
+                                       net::CertificateList()),
       net::SSLConfigService::GetCRLSet().get(), verify_result_ptr,
       bound_callback, &request_state->request, *net_log);
 

extensions/browser/api/cast_channel/cast_socket.cc

@@ -80,17 +80,14 @@ class FakeCertVerifier : public net::CertVerifier {
   FakeCertVerifier() {}
   ~FakeCertVerifier() override {}
 
-  int Verify(net::X509Certificate* cert,
-             const std::string&,
-             const std::string&,
-             int,
+  int Verify(const RequestParams& params,
              net::CRLSet*,
              net::CertVerifyResult* verify_result,
              const net::CompletionCallback&,
-             std::unique_ptr<net::CertVerifier::Request>*,
+             std::unique_ptr<Request>*,
              const net::BoundNetLog&) override {
     verify_result->Reset();
-    verify_result->verified_cert = cert;
+    verify_result->verified_cert = params.certificate();
     return net::OK;
   }
 };

google_apis/gcm/tools/mcs_probe.cc

@@ -177,10 +177,7 @@ class MyTestCertVerifier : public net::CertVerifier {
   MyTestCertVerifier() {}
   ~MyTestCertVerifier() override {}
 
-  int Verify(net::X509Certificate* cert,
-             const std::string& hostname,
-             const std::string& ocsp_response,
-             int flags,
+  int Verify(const RequestParams& params,
              net::CRLSet* crl_set,
              net::CertVerifyResult* verify_result,
              const net::CompletionCallback& callback,

ios/web/net/cert_verifier_block_adapter.cc

@@ -87,10 +87,12 @@ void CertVerifierBlockAdapter::Verify(
     completion_handler(context->result, error);
   });
   std::unique_ptr<net::CertVerifier::Request> request;
-  int error = cert_verifier_->Verify(params.cert.get(), params.hostname,
-                                     params.ocsp_response, params.flags,
-                                     params.crl_set.get(), &(context->result),
-                                     callback, &request, context->net_log);
+  int error = cert_verifier_->Verify(
+      net::CertVerifier::RequestParams(params.cert.get(), params.hostname,
+                                       params.flags, params.ocsp_response,
+                                       net::CertificateList()),
+      params.crl_set.get(), &(context->result), callback, &request,
+      context->net_log);
   if (error == net::ERR_IO_PENDING) {
     // Keep the |net::CertVerifier::Request| alive until verification completes.
     // Because |context| is kept alive by |callback| (through base::BindBlock),

net/cert/cert_verifier.cc

@@ -4,11 +4,13 @@
 
 #include "net/cert/cert_verifier.h"
 
+#include <openssl/sha.h>
+
 #include <algorithm>
 #include <memory>
 
 #include "base/memory/ptr_util.h"
-#include "base/sha1.h"
+#include "base/strings/string_util.h"
 #include "build/build_config.h"
 #include "net/cert/cert_verify_proc.h"
 
@@ -21,25 +23,39 @@
 namespace net {
 
 CertVerifier::RequestParams::RequestParams(
-    X509Certificate* certificate,
+    scoped_refptr<X509Certificate> certificate,
     const std::string& hostname,
     int flags,
     const std::string& ocsp_response,
-    const CertificateList& additional_trust_anchors)
-    : hostname_(hostname), flags_(flags) {
-  // Rather than store all of the original data, create a fingerprint based
-  // on the hash of the request data.
-  SHA1HashValue ocsp_hash;
-  base::SHA1HashBytes(
-      reinterpret_cast<const unsigned char*>(ocsp_response.data()),
-      ocsp_response.size(), ocsp_hash.data);
-
-  request_data_.reserve(additional_trust_anchors.size() + 3);
-  request_data_.push_back(ocsp_hash);
-  request_data_.push_back(certificate->fingerprint());
-  request_data_.push_back(certificate->ca_fingerprint());
-  for (const auto& trust_anchor : additional_trust_anchors)
-    request_data_.push_back(trust_anchor->fingerprint());
+    CertificateList additional_trust_anchors)
+    : certificate_(std::move(certificate)),
+      hostname_(hostname),
+      flags_(flags),
+      ocsp_response_(ocsp_response),
+      additional_trust_anchors_(std::move(additional_trust_anchors)) {
+  // For efficiency sake, rather than compare all of the fields for each
+  // comparison, compute a hash of their values. This is done directly in
+  // this class, rather than as an overloaded hash operator, for efficiency's
+  // sake.
+  SHA256_CTX ctx;
+  SHA256_Init(&ctx);
+  std::string cert_der;
+  X509Certificate::GetDEREncoded(certificate_->os_cert_handle(), &cert_der);
+  SHA256_Update(&ctx, cert_der.data(), cert_der.size());
+  for (const auto& cert_handle : certificate_->GetIntermediateCertificates()) {
+    X509Certificate::GetDEREncoded(cert_handle, &cert_der);
+    SHA256_Update(&ctx, cert_der.data(), cert_der.size());
+  }
+  SHA256_Update(&ctx, hostname_.data(), hostname.size());
+  SHA256_Update(&ctx, &flags, sizeof(flags));
+  SHA256_Update(&ctx, ocsp_response.data(), ocsp_response.size());
+  for (const auto& trust_anchor : additional_trust_anchors_) {
+    X509Certificate::GetDEREncoded(trust_anchor->os_cert_handle(), &cert_der);
+    SHA256_Update(&ctx, cert_der.data(), cert_der.size());
+  }
+  SHA256_Final(reinterpret_cast<uint8_t*>(
+                   base::WriteInto(&key_, SHA256_DIGEST_LENGTH + 1)),
+               &ctx);
 }
 
 CertVerifier::RequestParams::RequestParams(const RequestParams& other) =
@@ -48,13 +64,7 @@ CertVerifier::RequestParams::~RequestParams() {}
 
 bool CertVerifier::RequestParams::operator<(
     const CertVerifier::RequestParams& other) const {
-  if (flags_ != other.flags_)
-    return flags_ < other.flags_;
-  if (hostname_ != other.hostname_)
-    return hostname_ < other.hostname_;
-  return std::lexicographical_compare(
-      request_data_.begin(), request_data_.end(), other.request_data_.begin(),
-      other.request_data_.end(), SHA1HashValueLessThan());
+  return key_ < other.key_;
 }
 
 bool CertVerifier::SupportsOCSPStapling() {

net/cert/cert_verifier.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "net/base/completion_callback.h"
 #include "net/base/hash_value.h"
 #include "net/base/net_export.h"
@@ -76,30 +77,57 @@ class NET_EXPORT CertVerifier {
     VERIFY_REV_CHECKING_REQUIRED_LOCAL_ANCHORS = 1 << 4,
   };
 
-  // The parameters for doing a Verify(). |certificate|, |hostname|, and
-  // |flags| are required. The rest are optional.
+  // Parameters to verify |certificate| against the supplied
+  // |hostname| as an SSL server.
+  //
+  // |hostname| should be a canonicalized hostname (in A-Label form) or IP
+  // address in string form, following the rules of a URL host portion. In
+  // the case of |hostname| being a domain name, it may contain a trailing
+  // dot (e.g. "example.com."), as used to signal to DNS not to perform
+  // suffix search, and it will safely be ignored. If |hostname| is an IPv6
+  // address, it MUST be in URL form - that is, surrounded in square
+  // brackets, such as "[::1]".
+  //
+  // |flags| is a bitwise OR of VerifyFlags.
+  //
+  // |ocsp_response| is optional, but if non-empty, should contain an OCSP
+  // response obtained via OCSP stapling. It may be ignored by the
+  // CertVerifier.
+  //
+  // |additional_trust_anchors| is optional, but if non-empty, should contain
+  // additional certificates to be treated as trust anchors. It may be ignored
+  // by the CertVerifier.
   class NET_EXPORT RequestParams {
    public:
-    RequestParams(X509Certificate* certificate,
+    RequestParams(scoped_refptr<X509Certificate> certificate,
                   const std::string& hostname,
                   int flags,
                   const std::string& ocsp_response,
-                  const CertificateList& additional_trust_anchors);
+                  CertificateList additional_trust_anchors);
     RequestParams(const RequestParams& other);
     ~RequestParams();
 
+    const scoped_refptr<X509Certificate>& certificate() const {
+      return certificate_;
+    }
     const std::string& hostname() const { return hostname_; }
     int flags() const { return flags_; }
-    const std::vector<SHA1HashValue> request_data() const {
-      return request_data_;
+    const std::string& ocsp_response() const { return ocsp_response_; }
+    const CertificateList& additional_trust_anchors() const {
+      return additional_trust_anchors_;
     }
 
     bool operator<(const RequestParams& other) const;
 
    private:
+    scoped_refptr<X509Certificate> certificate_;
     std::string hostname_;
     int flags_;
-    std::vector<SHA1HashValue> request_data_;
+    std::string ocsp_response_;
+    CertificateList additional_trust_anchors_;
+
+    // Used to optimize sorting/indexing comparisons.
+    std::string key_;
   };
 
   // When the verifier is destroyed, all certificate verification requests are
@@ -115,17 +143,6 @@ class NET_EXPORT CertVerifier {
   // |verify_result->cert_status|, and the error code for the most serious
   // error is returned.
   //
-  // |ocsp_response|, if non-empty, is a stapled OCSP response to use.
-  //
-  // |flags| is bitwise OR'd of VerifyFlags.
-  // If VERIFY_REV_CHECKING_ENABLED is set in |flags|, certificate revocation
-  // checking is performed.
-  //
-  // If VERIFY_EV_CERT is set in |flags| too, EV certificate verification is
-  // performed.  If |flags| is VERIFY_EV_CERT (that is,
-  // VERIFY_REV_CHECKING_ENABLED is not set), EV certificate verification will
-  // not be performed.
-  //
   // |crl_set| points to an optional CRLSet structure which can be used to
   // avoid revocation checks over the network.
   //
@@ -140,12 +157,7 @@ class NET_EXPORT CertVerifier {
   // If Verify() completes synchronously then |out_req| *may* be reset to
   // nullptr. However it is not guaranteed that all implementations will reset
   // it in this case.
-  //
-  // TODO(rsleevi): Update this to use RequestParams as part of the signature.
-  virtual int Verify(X509Certificate* cert,
-                     const std::string& hostname,
-                     const std::string& ocsp_response,
-                     int flags,
+  virtual int Verify(const RequestParams& params,
                      CRLSet* crl_set,
                      CertVerifyResult* verify_result,
                      const CompletionCallback& callback,