Do not hash the certificate twice.

Change X509Certificate::chain_fingerprint_ to
X509Certificate::ca_fingerprint_ to exclude the certificate
from this fingerprint.  This fingerprint covers the intermediate
CA certificates only.

This requires identifying an X509Certificate object by two
fingerprints: cert->fingerprint() and cert->ca_fingerprint().

R=agl@chromium.org,rsleevi@chromium.org
BUG=101555
TEST=unit tests updated

Review URL: http://codereview.chromium.org/8449004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@108756 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 92 insertions, 56 deletions

diff --git a/net/base/cert_verifier.cc b/net/base/cert_verifier.cc
index 2c703eb..4c4fa69 100644
--- a/net/base/cert_verifier.cc
+++ b/net/base/cert_verifier.cc
@@ -372,7 +372,8 @@ int CertVerifier::Verify(X509Certificate* cert,
 
   requests_++;
 
-  const RequestParams key = {cert->chain_fingerprint(), hostname, flags};
+  const RequestParams key(cert->fingerprint(), cert->ca_fingerprint(),
+                          hostname, flags);
   // First check the cache.
   std::map<RequestParams, CachedCertVerifyResult>::iterator i;
   i = cache_.find(key);
@@ -457,7 +458,8 @@ void CertVerifier::HandleResult(X509Certificate* cert,
   uint32 ttl = kTTLSecs;
   cached_result.expiry = current_time + base::TimeDelta::FromSeconds(ttl);
 
-  const RequestParams key = {cert->chain_fingerprint(), hostname, flags};
+  const RequestParams key(cert->fingerprint(), cert->ca_fingerprint(),
+                          hostname, flags);
 
   DCHECK_GE(max_cache_entries_, 1u);
   DCHECK_LE(cache_.size(), max_cache_entries_);
diff --git a/net/base/cert_verifier.h b/net/base/cert_verifier.h
index 68000fe..ac47196 100644
--- a/net/base/cert_verifier.h
+++ b/net/base/cert_verifier.h
@@ -132,30 +132,46 @@ class NET_EXPORT CertVerifier : NON_EXPORTED_BASE(public base::NonThreadSafe),
 
   // Input parameters of a certificate verification request.
   struct RequestParams {
+    RequestParams(const SHA1Fingerprint& cert_fingerprint_arg,
+                  const SHA1Fingerprint& ca_fingerprint_arg,
+                  const std::string& hostname_arg,
+                  int flags_arg)
+        : cert_fingerprint(cert_fingerprint_arg),
+          ca_fingerprint(ca_fingerprint_arg),
+          hostname(hostname_arg),
+          flags(flags_arg) {}
+
     bool operator==(const RequestParams& other) const {
-      // |flags| is compared before |cert_fingerprint| and |hostname| under
-      // assumption that integer comparisons are faster than memory and string
-      // comparisons.
+      // |flags| is compared before |cert_fingerprint|, |ca_fingerprint|, and
+      // |hostname| under assumption that integer comparisons are faster than
+      // memory and string comparisons.
       return (flags == other.flags &&
               memcmp(cert_fingerprint.data, other.cert_fingerprint.data,
                      sizeof(cert_fingerprint.data)) == 0 &&
+              memcmp(ca_fingerprint.data, other.ca_fingerprint.data,
+                     sizeof(ca_fingerprint.data)) == 0 &&
               hostname == other.hostname);
     }
 
     bool operator<(const RequestParams& other) const {
-      // |flags| is compared before |cert_fingerprint| and |hostname| under
-      // assumption that integer comparisons are faster than memory and string
-      // comparisons.
+      // |flags| is compared before |cert_fingerprint|, |ca_fingerprint|, and
+      // |hostname| under assumption that integer comparisons are faster than
+      // memory and string comparisons.
       if (flags != other.flags)
         return flags < other.flags;
       int rv = memcmp(cert_fingerprint.data, other.cert_fingerprint.data,
                       sizeof(cert_fingerprint.data));
       if (rv != 0)
         return rv < 0;
+      rv = memcmp(ca_fingerprint.data, other.ca_fingerprint.data,
+                  sizeof(ca_fingerprint.data));
+      if (rv != 0)
+        return rv < 0;
       return hostname < other.hostname;
     }
 
     SHA1Fingerprint cert_fingerprint;
+    SHA1Fingerprint ca_fingerprint;
     std::string hostname;
     int flags;
   };
diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index 90206c9..324dcc6 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -231,8 +231,14 @@ bool X509Certificate::LessThan::operator()(X509Certificate* lhs,
   if (lhs == rhs)
     return false;
 
-  SHA1FingerprintLessThan fingerprint_functor;
-  return fingerprint_functor(lhs->chain_fingerprint_, rhs->chain_fingerprint_);
+  int rv = memcmp(lhs->fingerprint_.data, rhs->fingerprint_.data,
+                  sizeof(lhs->fingerprint_.data));
+  if (rv != 0)
+    return rv < 0;
+
+  rv = memcmp(lhs->ca_fingerprint_.data, rhs->ca_fingerprint_.data,
+              sizeof(lhs->ca_fingerprint_.data));
+  return rv < 0;
 }
 
 X509Certificate::X509Certificate(const std::string& subject,
@@ -245,7 +251,7 @@ X509Certificate::X509Certificate(const std::string& subject,
       valid_expiry_(expiration_date),
       cert_handle_(NULL) {
   memset(fingerprint_.data, 0, sizeof(fingerprint_.data));
-  memset(chain_fingerprint_.data, 0, sizeof(chain_fingerprint_.data));
+  memset(ca_fingerprint_.data, 0, sizeof(ca_fingerprint_.data));
 }
 
 // static
diff --git a/net/base/x509_certificate.h b/net/base/x509_certificate.h
index e24d88e..04d0b8e 100644
--- a/net/base/x509_certificate.h
+++ b/net/base/x509_certificate.h
@@ -210,9 +210,9 @@ class NET_EXPORT X509Certificate
   // The fingerprint of this certificate.
   const SHA1Fingerprint& fingerprint() const { return fingerprint_; }
 
-  // The fingerprint of this certificate and its intermediate CA certificates.
-  const SHA1Fingerprint& chain_fingerprint() const {
-    return chain_fingerprint_;
+  // The fingerprint of the intermediate CA certificates.
+  const SHA1Fingerprint& ca_fingerprint() const {
+    return ca_fingerprint_;
   }
 
   // Gets the DNS names in the certificate.  Pursuant to RFC 2818, Section 3.1
@@ -434,9 +434,10 @@ class NET_EXPORT X509Certificate
   // (all zero) fingerprint on failure.
   static SHA1Fingerprint CalculateFingerprint(OSCertHandle cert_handle);
 
-  // Calculates the SHA-1 fingerprint of the certificate and its intermediate
-  // CA certificates.  Returns an empty (all zero) fingerprint on failure.
-  SHA1Fingerprint CalculateChainFingerprint() const;
+  // Calculates the SHA-1 fingerprint of the intermediate CA certificates.
+  // Returns an empty (all zero) fingerprint on failure.
+  static SHA1Fingerprint CalculateCAFingerprint(
+      const OSCertHandles& intermediates);
 
  private:
   friend class base::RefCountedThreadSafe<X509Certificate>;
@@ -540,8 +541,8 @@ class NET_EXPORT X509Certificate
   // The fingerprint of this certificate.
   SHA1Fingerprint fingerprint_;
 
-  // The fingerprint of this certificate and its intermediate CA certificates.
-  SHA1Fingerprint chain_fingerprint_;
+  // The fingerprint of the intermediate CA certificates.
+  SHA1Fingerprint ca_fingerprint_;
 
   // The serial number of this certificate, DER encoded.
   std::string serial_number_;
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index 1db5a3b..6b0c105 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -616,7 +616,7 @@ void X509Certificate::Initialize() {
                     &valid_expiry_);
 
   fingerprint_ = CalculateFingerprint(cert_handle_);
-  chain_fingerprint_ = CalculateChainFingerprint();
+  ca_fingerprint_ = CalculateCAFingerprint(intermediate_ca_certs_);
   serial_number_ = GetCertSerialNumber(cert_handle_);
 }
 
@@ -1124,7 +1124,9 @@ SHA1Fingerprint X509Certificate::CalculateFingerprint(
   return sha1;
 }
 
-SHA1Fingerprint X509Certificate::CalculateChainFingerprint() const {
+// static
+SHA1Fingerprint X509Certificate::CalculateCAFingerprint(
+    const OSCertHandles& intermediates) {
   SHA1Fingerprint sha1;
   memset(sha1.data, 0, sizeof(sha1.data));
 
@@ -1133,12 +1135,8 @@ SHA1Fingerprint X509Certificate::CalculateChainFingerprint() const {
   CC_SHA1_CTX sha1_ctx;
   CC_SHA1_Init(&sha1_ctx);
   CSSM_DATA cert_data;
-  OSStatus status = SecCertificateGetData(cert_handle_, &cert_data);
-  if (status)
-    return sha1;
-  CC_SHA1_Update(&sha1_ctx, cert_data.Data, cert_data.Length);
-  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
-    status = SecCertificateGetData(intermediate_ca_certs_[i], &cert_data);
+  for (size_t i = 0; i < intermediates.size(); ++i) {
+    OSStatus status = SecCertificateGetData(intermediates[i], &cert_data);
     if (status)
       return sha1;
     CC_SHA1_Update(&sha1_ctx, cert_data.Data, cert_data.Length);
diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index c00ee5b..bbb5cef 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -677,7 +677,7 @@ void X509Certificate::Initialize() {
   ParseDate(&cert_handle_->validity.notAfter, &valid_expiry_);
 
   fingerprint_ = CalculateFingerprint(cert_handle_);
-  chain_fingerprint_ = CalculateChainFingerprint();
+  ca_fingerprint_ = CalculateCAFingerprint(intermediate_ca_certs_);
 
   serial_number_ = std::string(
       reinterpret_cast<char*>(cert_handle_->serialNumber.data),
@@ -1004,7 +1004,9 @@ SHA1Fingerprint X509Certificate::CalculateFingerprint(
   return sha1;
 }
 
-SHA1Fingerprint X509Certificate::CalculateChainFingerprint() const {
+// static
+SHA1Fingerprint X509Certificate::CalculateCAFingerprint(
+    const OSCertHandles& intermediates) {
   SHA1Fingerprint sha1;
   memset(sha1.data, 0, sizeof(sha1.data));
 
@@ -1012,9 +1014,8 @@ SHA1Fingerprint X509Certificate::CalculateChainFingerprint() const {
   if (!sha1_ctx)
     return sha1;
   HASH_Begin(sha1_ctx);
-  HASH_Update(sha1_ctx, cert_handle_->derCert.data, cert_handle_->derCert.len);
-  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
-    CERTCertificate* ca_cert = intermediate_ca_certs_[i];
+  for (size_t i = 0; i < intermediates.size(); ++i) {
+    CERTCertificate* ca_cert = intermediates[i];
     HASH_Update(sha1_ctx, ca_cert->derCert.data, ca_cert->derCert.len);
   }
   unsigned int result_len;
diff --git a/net/base/x509_certificate_openssl.cc b/net/base/x509_certificate_openssl.cc
index 5612d61..8b48fc6 100644
--- a/net/base/x509_certificate_openssl.cc
+++ b/net/base/x509_certificate_openssl.cc
@@ -386,7 +386,7 @@ void X509Certificate::FreeOSCertHandle(OSCertHandle cert_handle) {
 void X509Certificate::Initialize() {
   crypto::EnsureOpenSSLInit();
   fingerprint_ = CalculateFingerprint(cert_handle_);
-  chain_fingerprint_ = CalculateChainFingerprint();
+  ca_fingerprint_ = CalculateCAFingerprint(intermediate_ca_certs_);
 
   ASN1_INTEGER* serial_num = X509_get_serialNumber(cert_handle_);
   if (serial_num) {
@@ -424,18 +424,17 @@ SHA1Fingerprint X509Certificate::CalculateFingerprint(OSCertHandle cert) {
   return sha1;
 }
 
-SHA1Fingerprint X509Certificate::CalculateChainFingerprint() const {
+// static
+SHA1Fingerprint X509Certificate::CalculateCAFingerprint(
+    const OSCertHandles& intermediates) {
   SHA1Fingerprint sha1;
   memset(sha1.data, 0, sizeof(sha1.data));
 
   SHA_CTX sha1_ctx;
   SHA1_Init(&sha1_ctx);
   DERCache der_cache;
-  if (!GetDERAndCacheIfNeeded(cert_handle_, &der_cache))
-    return sha1;
-  SHA1_Update(&sha1_ctx, der_cache.data, der_cache.data_length);
-  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
-    if (!GetDERAndCacheIfNeeded(intermediate_ca_certs_[i], &der_cache))
+  for (size_t i = 0; i < intermediates.size(); ++i) {
+    if (!GetDERAndCacheIfNeeded(intermediates[i], &der_cache))
       return sha1;
     SHA1_Update(&sha1_ctx, der_cache.data, der_cache.data_length);
   }
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index 010aba73..f483362 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -436,7 +436,7 @@ TEST(X509CertificateTest, SerialNumbers) {
                      paypal_null_serial, sizeof(paypal_null_serial)) == 0);
 }
 
-TEST(X509CertificateTest, ChainFingerprints) {
+TEST(X509CertificateTest, CAFingerprints) {
   FilePath certs_dir = GetTestCertsDirectory();
 
   scoped_refptr<X509Certificate> server_cert =
@@ -463,18 +463,31 @@ TEST(X509CertificateTest, ChainFingerprints) {
       X509Certificate::CreateFromHandle(server_cert->os_cert_handle(),
                                         intermediates);
 
-  static const uint8 cert_chain1_fingerprint[20] = {
-    0x67, 0x78, 0x81, 0xd7, 0x78, 0xca, 0xd5, 0x04, 0x73, 0xf8,
-    0x95, 0xff, 0xf3, 0x39, 0xe4, 0xcd, 0x5e, 0xf0, 0x79, 0x76
+  // No intermediate CA certicates.
+  intermediates.clear();
+  scoped_refptr<X509Certificate> cert_chain3 =
+      X509Certificate::CreateFromHandle(server_cert->os_cert_handle(),
+                                        intermediates);
+
+  static const uint8 cert_chain1_ca_fingerprint[20] = {
+    0xc2, 0xf0, 0x08, 0x7d, 0x01, 0xe6, 0x86, 0x05, 0x3a, 0x4d,
+    0x63, 0x3e, 0x7e, 0x70, 0xd4, 0xef, 0x65, 0xc2, 0xcc, 0x4f
+  };
+  static const uint8 cert_chain2_ca_fingerprint[20] = {
+    0xd5, 0x59, 0xa5, 0x86, 0x66, 0x9b, 0x08, 0xf4, 0x6a, 0x30,
+    0xa1, 0x33, 0xf8, 0xa9, 0xed, 0x3d, 0x03, 0x8e, 0x2e, 0xa8
   };
-  static const uint8 cert_chain2_fingerprint[20] = {
-    0x8c, 0x93, 0x85, 0xb0, 0x15, 0xd3, 0xa3, 0x0e, 0xe7, 0x4f,
-    0x42, 0xf4, 0x30, 0xc3, 0xe9, 0x14, 0x12, 0x54, 0xb9, 0x9d
+  // The SHA-1 hash of nothing.
+  static const uint8 cert_chain3_ca_fingerprint[20] = {
+    0xda, 0x39, 0xa3, 0xee, 0x5e, 0x6b, 0x4b, 0x0d, 0x32, 0x55,
+    0xbf, 0xef, 0x95, 0x60, 0x18, 0x90, 0xaf, 0xd8, 0x07, 0x09
   };
-  EXPECT_TRUE(memcmp(cert_chain1->chain_fingerprint().data,
-                     cert_chain1_fingerprint, 20) == 0);
-  EXPECT_TRUE(memcmp(cert_chain2->chain_fingerprint().data,
-                     cert_chain2_fingerprint, 20) == 0);
+  EXPECT_TRUE(memcmp(cert_chain1->ca_fingerprint().data,
+                     cert_chain1_ca_fingerprint, 20) == 0);
+  EXPECT_TRUE(memcmp(cert_chain2->ca_fingerprint().data,
+                     cert_chain2_ca_fingerprint, 20) == 0);
+  EXPECT_TRUE(memcmp(cert_chain3->ca_fingerprint().data,
+                     cert_chain3_ca_fingerprint, 20) == 0);
 }
 
 // A regression test for http://crbug.com/31497.
diff --git a/net/base/x509_certificate_win.cc b/net/base/x509_certificate_win.cc
index 4905b0b..1c89abb 100644
--- a/net/base/x509_certificate_win.cc
+++ b/net/base/x509_certificate_win.cc
@@ -576,7 +576,7 @@ void X509Certificate::Initialize() {
   valid_expiry_ = Time::FromFileTime(cert_handle_->pCertInfo->NotAfter);
 
   fingerprint_ = CalculateFingerprint(cert_handle_);
-  chain_fingerprint_ = CalculateChainFingerprint();
+  ca_fingerprint_ = CalculateCAFingerprint(intermediate_ca_certs_);
 
   const CRYPT_INTEGER_BLOB* serial = &cert_handle_->pCertInfo->SerialNumber;
   scoped_array<uint8> serial_bytes(new uint8[serial->cbData]);
@@ -1092,7 +1092,9 @@ SHA1Fingerprint X509Certificate::CalculateFingerprint(
 // TODO(wtc): This function is implemented with NSS low-level hash
 // functions to ensure it is fast.  Reimplement this function with
 // CryptoAPI.  May need to cache the HCRYPTPROV to reduce the overhead.
-SHA1Fingerprint X509Certificate::CalculateChainFingerprint() const {
+// static
+SHA1Fingerprint X509Certificate::CalculateCAFingerprint(
+    const OSCertHandles& intermediates) {
   SHA1Fingerprint sha1;
   memset(sha1.data, 0, sizeof(sha1.data));
 
@@ -1100,10 +1102,8 @@ SHA1Fingerprint X509Certificate::CalculateChainFingerprint() const {
   if (!sha1_ctx)
     return sha1;
   SHA1_Begin(sha1_ctx);
-  SHA1_Update(sha1_ctx, cert_handle_->pbCertEncoded,
-              cert_handle_->cbCertEncoded);
-  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
-    PCCERT_CONTEXT ca_cert = intermediate_ca_certs_[i];
+  for (size_t i = 0; i < intermediates.size(); ++i) {
+    PCCERT_CONTEXT ca_cert = intermediates[i];
     SHA1_Update(sha1_ctx, ca_cert->pbCertEncoded, ca_cert->cbCertEncoded);
   }
   unsigned int result_len;