Remove the fingerprint and ca_fingerprint from X509Certificate

net/cert/x509_certificate.cc

Index: net/cert/x509_certificate.cc
diff --git a/net/cert/x509_certificate.cc b/net/cert/x509_certificate.cc
index adfe9e55ccba5397601cf5f8f5eba3bea39c197f..494956432fb0ec8cdf5e5f7d2c68b37d768491a6 100644
--- a/net/cert/x509_certificate.cc
+++ b/net/cert/x509_certificate.cc
@@ -83,7 +83,7 @@ class X509CertificateCache {
   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 {
@@ -97,7 +97,7 @@ class X509CertificateCache {
     // 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() {}
@@ -120,8 +120,8 @@ base::LazyInstance<X509CertificateCache>::Leaky
 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;
   {
@@ -139,7 +139,7 @@ void X509CertificateCache::InsertOrUpdate(
       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;
@@ -168,8 +168,8 @@ void X509CertificateCache::InsertOrUpdate(
 }
 
 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);
@@ -228,17 +228,32 @@ void SplitOnChar(const base::StringPiece& src,
 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;
-
-  rv = memcmp(lhs->ca_fingerprint_.data, rhs->ca_fingerprint_.data,
-              sizeof(lhs->ca_fingerprint_.data));
-  return rv < 0;
+  std::string lhs_der;
+  X509Certificate::GetDEREncoded(lhs->os_cert_handle(), &lhs_der);
+
+  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,
@@ -250,8 +265,6 @@ X509Certificate::X509Certificate(const std::string& subject,
       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
@@ -710,26 +723,6 @@ bool X509Certificate::GetPEMEncodedChain(
 }
 
 // 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) {