Remove the fingerprint and ca_fingerprint from X509Certificate

net/cert/x509_certificate.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/cert/x509_certificate.h"
 
 #include <limits.h>
 #include <stdlib.h>
 
 #include <algorithm>
@@ skipping 65 matching lines @@
   void InsertOrUpdate(X509Certificate::OSCertHandle* cert_handle);
 
   // Decrements the cache reference count for |cert_handle|, a handle that was
   // previously obtained by calling InsertOrUpdate(). If this is the last
   // cached reference held, this will remove the handle from the cache. The
   // caller retains ownership of |cert_handle| and remains responsible for
   // calling FreeOSCertHandle() to release the underlying OS certificate
   void Remove(X509Certificate::OSCertHandle cert_handle);
 
  private:
-  // A single entry in the cache. Certificates will be keyed by their SHA1
+  // A single entry in the cache. Certificates will be keyed by their SHA-256
   // fingerprints, but will not be considered equivalent unless the entire
   // certificate data matches.
   struct Entry {
     Entry() : cert_handle(NULL), ref_count(0) {}
 
     X509Certificate::OSCertHandle cert_handle;
 
     // Increased by each call to InsertOrUpdate(), and balanced by each call
     // to Remove(). When it equals 0, all references created by
     // InsertOrUpdate() have been released, so the cache entry will be removed
     // the cached OS certificate handle will be freed.
     int ref_count;
   };
-  typedef std::map<SHA1HashValue, Entry, SHA1HashValueLessThan> CertMap;
+  typedef std::map<SHA256HashValue, Entry, SHA256HashValueLessThan> CertMap;
 
   // Obtain an instance of X509CertificateCache via a LazyInstance.
   X509CertificateCache() {}
   ~X509CertificateCache() {}
   friend struct base::DefaultLazyInstanceTraits<X509CertificateCache>;
 
   // You must acquire this lock before using any private data of this object
   // You must not block while holding this lock.
   base::Lock lock_;
 
   // The certificate cache.  You must acquire |lock_| before using |cache_|.
   CertMap cache_;
 
   DISALLOW_COPY_AND_ASSIGN(X509CertificateCache);
 };
 
 base::LazyInstance<X509CertificateCache>::Leaky
     g_x509_certificate_cache = LAZY_INSTANCE_INITIALIZER;
 
 void X509CertificateCache::InsertOrUpdate(
     X509Certificate::OSCertHandle* cert_handle) {
   DCHECK(cert_handle);
-  SHA1HashValue fingerprint =
-      X509Certificate::CalculateFingerprint(*cert_handle);
+  SHA256HashValue fingerprint =
+      X509Certificate::CalculateFingerprint256(*cert_handle);
 
   X509Certificate::OSCertHandle old_handle = NULL;
   {
     base::AutoLock lock(lock_);
     CertMap::iterator pos = cache_.find(fingerprint);
     if (pos == cache_.end()) {
       // A cached entry was not found, so initialize a new entry. The entry
       // assumes ownership of the current |*cert_handle|.
       Entry cache_entry;
       cache_entry.cert_handle = *cert_handle;
       cache_entry.ref_count = 0;
       CertMap::value_type cache_value(fingerprint, cache_entry);
       pos = cache_.insert(cache_value).first;
     } else {
       bool is_same_cert =
           X509Certificate::IsSameOSCert(*cert_handle, pos->second.cert_handle);

[eroman, 2016/05/26 01:46:26]

If we are able to get hash collisions on SHA-256 we have bigger problems...
Remove this?

[Ryan Sleevi, 2016/05/26 07:48:38]

This seems like an unrelated request, and I'm not sure I agree with your
assessment - that is, I suspect there are some steps I'm missing from why you
believe it's good to remove this. I can see possible arguments, but I'd prefer
if you clarified (so I don't overwhelm you with all the possible
interpretations)

[eroman, 2016/05/26 20:28:55]

My concern starts with the comment below:

  // Two certificates don't match, due to a SHA-256 hash collision. Given
  // the low probability, the simplest solution is to not cache the

The implication here is that the code is handling a case where we have two
different certs, c1 !=c2, and yet SHA256(c1) == SHA256(c2).

Why is that a reasonable thing for the code to handle?

If that were computationally feasible, then we have bigger problems. Not the
least of which, MultiThreadedCertVerifier::FindJob() would also be wrong, as it
doesn't check for equality!

The only reason I could see for comparing equality after a hash match is if the
data hashed by CalculateFingerprint256() were incomplete, or the hash truncated.

AFAICT that is not the case. IsSameOSCert() just compares the cert DER for
equality, same as what CalculateFingerprint256() does. Neither considers
intermediates for equality.

Why then would we worry about collisions in this function but no-where else?

       if (!is_same_cert) {
-        // Two certificates don't match, due to a SHA1 hash collision. Given
+        // Two certificates don't match, due to a SHA-256 hash collision. Given
         // the low probability, the simplest solution is to not cache the
         // certificate, which should not affect performance too negatively.
         return;
       }
       // A cached entry was found and will be used instead of the caller's
       // handle. Ensure the caller's original handle will be freed, since
       // ownership is assumed.
       old_handle = *cert_handle;
     }
     // Whether an existing cached handle or a new handle, increment the
     // cache's reference count and return a handle that the caller can own.
     ++pos->second.ref_count;
     *cert_handle = X509Certificate::DupOSCertHandle(pos->second.cert_handle);
   }
   // If the caller's handle was replaced with a cached handle, free the
   // original handle now. This is done outside of the lock because
   // |old_handle| may be the only handle for this particular certificate, so
   // freeing it may be complex or resource-intensive and does not need to
   // be guarded by the lock.
   if (old_handle) {
     X509Certificate::FreeOSCertHandle(old_handle);
 #ifndef NDEBUG
     LOCAL_HISTOGRAM_BOOLEAN("X509CertificateReuseCount", true);
 #endif
   }
 }
 
 void X509CertificateCache::Remove(X509Certificate::OSCertHandle cert_handle) {
-  SHA1HashValue fingerprint =
-      X509Certificate::CalculateFingerprint(cert_handle);
+  SHA256HashValue fingerprint =
+      X509Certificate::CalculateFingerprint256(cert_handle);
   base::AutoLock lock(lock_);
 
   CertMap::iterator pos = cache_.find(fingerprint);
   if (pos == cache_.end())
     return;  // A hash collision where the winning cert was already freed.
 
   bool is_same_cert = X509Certificate::IsSameOSCert(cert_handle,

[eroman, 2016/05/26 01:46:26]

Same here.

                                                     pos->second.cert_handle);
   if (!is_same_cert)
     return;  // A hash collision where the winning cert is still around.
 
   if (--pos->second.ref_count == 0) {
     // The last reference to |cert_handle| has been removed, so release the
     // Entry's OS handle and remove the Entry. The caller still holds a
     // reference to |cert_handle| and is responsible for freeing it.
     X509Certificate::FreeOSCertHandle(pos->second.cert_handle);
     cache_.erase(pos);
@@ skipping 28 matching lines @@
     *left = src;
     right->clear();
   } else {
     *left = src.substr(0, pos);
     *right = src.substr(pos);
   }
 }
 
 }  // namespace
 
 bool X509Certificate::LessThan::operator()(

[Ryan Sleevi, 2016/05/20 06:02:31]

The thing that makes me unhappy is this ends up being used by the CertStoreImpl
(which is templated to use a T::LessThan functor as a map key).

My thinking is that if this does become perf-sensitive, we change the
CertStoreImpl to memoize on a meta-structure, and store the computed
fingerprints there as the optimization.

[eroman, 2016/05/26 01:46:26]

Doing a codesearch didn't turn up references for this (although probably
bugged).
Seems fine if just used in a few places.

[Ryan Sleevi, 2016/05/26 07:48:38]

Yup, that's why I thought this was safer than it was - and then the compile
failures in content started.

     const scoped_refptr<X509Certificate>& lhs,
     const scoped_refptr<X509Certificate>& rhs) const {
-  if (lhs.get() == rhs.get())
+  if (lhs == rhs)
     return false;
 
-  int rv = memcmp(lhs->fingerprint_.data, rhs->fingerprint_.data,
-                  sizeof(lhs->fingerprint_.data));
-  if (rv != 0)
-    return rv < 0;
+  std::string lhs_der;
+  X509Certificate::GetDEREncoded(lhs->os_cert_handle(), &lhs_der);
 
-  rv = memcmp(lhs->ca_fingerprint_.data, rhs->ca_fingerprint_.data,
-              sizeof(lhs->ca_fingerprint_.data));
-  return rv < 0;
+  std::string rhs_der;
+  X509Certificate::GetDEREncoded(rhs->os_cert_handle(), &rhs_der);

[eroman, 2016/05/26 01:46:26]

None of this code checks for failure from GetDEREncoded().

That could lead to problems if failures in GetDEREncoded() truly are reachable.

At a first glance, the Mac implementation can fail on non-null cert handles if
SecCertificateGetData().

If that happens then we can get into trouble in a number of ways:

  - End up with a non-deterministic comparator (sometimes an element compares
less, other times not) -- if used by a sorting algorithm could lead to crashes
or incorrect results.

Or worse:

  - Believe two certs are equal when they aren't, base don the comparison (!(a
<b) && !(b < a)). This can come up with standard implementations like binary
search and map<>.

Whatever we decide to do for LessThan, I think it is worth doing something at a
higher level too:

 - Either make the return value WARN_UNUSED_RESULT (some other callers don't
check either)
 - Or change the API contract so it can't fail (CHECK instead if we don't
believe it is reachable in practice).

WDYT?

[Ryan Sleevi, 2016/05/26 07:48:38]

This is the correct answer, sorry for not documenting that in this CL. It
won't/can't fail in practice anymore (esp now that we dropped < 10.7)

+  if (lhs_der != rhs_der)
+    return lhs_der < rhs_der;
+
+  const X509Certificate::OSCertHandles& lhs_intermediates =
+      lhs->GetIntermediateCertificates();
+  const X509Certificate::OSCertHandles& rhs_intermediates =
+      rhs->GetIntermediateCertificates();
+  return std::lexicographical_compare(

[eroman, 2016/05/30 20:20:30]

This is worth noting in the documentation for LessThan.

Most code (including the rest of this file) assumes certificate equality when
the DER data matches -- using IsSameOSCert() / X509Certificate::Equals() which
do not consider intermediates.

Whereas the equivalence class defined by this strict weak ordering is different.

+      lhs_intermediates.begin(), lhs_intermediates.end(),
+      rhs_intermediates.begin(), rhs_intermediates.end(),
+      [](const X509Certificate::OSCertHandle& left,
+         const X509Certificate::OSCertHandle& right) {
+        std::string left_encoded;
+        X509Certificate::GetDEREncoded(left, &left_encoded);
+        std::string right_encoded;
+        X509Certificate::GetDEREncoded(right, &right_encoded);
+        return left_encoded < right_encoded;

[eroman, 2016/05/30 20:20:30]

To confirm: The ordering itself wasn't being relied on anywhere, just the fact
that there was *some* total ordering? (Since this obviously changes the sort
ordering).

+      });
 }
 
 X509Certificate::X509Certificate(const std::string& subject,
                                  const std::string& issuer,
                                  base::Time start_date,
                                  base::Time expiration_date)
     : subject_(subject),
       issuer_(issuer),
       valid_start_(start_date),
       valid_expiry_(expiration_date),
       cert_handle_(NULL) {
-  memset(fingerprint_.data, 0, sizeof(fingerprint_.data));
-  memset(ca_fingerprint_.data, 0, sizeof(ca_fingerprint_.data));
 }
 
 // static
 scoped_refptr<X509Certificate> X509Certificate::CreateFromHandle(
     OSCertHandle cert_handle,
     const OSCertHandles& intermediates) {
   DCHECK(cert_handle);
   return new X509Certificate(cert_handle, intermediates);
 }
 
@@ skipping 438 matching lines @@
   for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
     if (!GetPEMEncoded(intermediate_ca_certs_[i], &pem_data))
       return false;
     encoded_chain.push_back(pem_data);
   }
   pem_encoded->swap(encoded_chain);
   return true;
 }
 
 // static
-SHA256HashValue X509Certificate::CalculateCAFingerprint256(
-    const OSCertHandles& intermediates) {
-  SHA256HashValue sha256;
-  memset(sha256.data, 0, sizeof(sha256.data));
-
-  std::unique_ptr<crypto::SecureHash> hash(
-      crypto::SecureHash::Create(crypto::SecureHash::SHA256));
-
-  for (size_t i = 0; i < intermediates.size(); ++i) {
-    std::string der_encoded;
-    if (!GetDEREncoded(intermediates[i], &der_encoded))
-      return sha256;
-    hash->Update(der_encoded.data(), der_encoded.length());
-  }
-  hash->Finish(sha256.data, sizeof(sha256.data));
-
-  return sha256;
-}
-
-// static
 SHA256HashValue X509Certificate::CalculateChainFingerprint256(
     OSCertHandle leaf,
     const OSCertHandles& intermediates) {
   OSCertHandles chain;
   chain.push_back(leaf);
   chain.insert(chain.end(), intermediates.begin(), intermediates.end());
 
   return CalculateCAFingerprint256(chain);
 }
 
@@ skipping 19 matching lines @@
     RemoveFromCache(cert_handle_);
     FreeOSCertHandle(cert_handle_);
   }
   for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
     RemoveFromCache(intermediate_ca_certs_[i]);
     FreeOSCertHandle(intermediate_ca_certs_[i]);
   }
 }
 
 }  // namespace net