Stop caching DER-encoded certificates unnecessarily

net/socket/ssl_client_socket_impl.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/socket/ssl_client_socket_impl.h"
 
 #include <errno.h>
 #include <openssl/bio.h>
 #include <openssl/bytestring.h>
 #include <openssl/err.h>
@@ skipping 431 matching lines @@
   openssl_chain_.reset(X509_chain_up_ref(other.openssl_chain_.get()));
   return *this;
 }
 
 void SSLClientSocketImpl::PeerCertificateChain::Reset(STACK_OF(X509) * chain) {
   openssl_chain_.reset(chain ? X509_chain_up_ref(chain) : NULL);
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketImpl::PeerCertificateChain::AsOSChain() const {
-#if defined(USE_OPENSSL_CERTS)

[davidben, 2016/08/31 19:20:49]

Hrm. This does mean that the SSLClientSocketImpl will no longer share an X509*
with the X509Certificate version, so we lose one deduplication in the good dedup
case but avoid caching things with GetDER in the bad dedup case (where we got an
X509* over the wire into SSLClientSocketImpl that we already had).

Or am I misunderstanding the situation? Seems what we'd really want is for
InsertOrUpdate to not cache stuff on the discarded OSCertHandle. Or maybe it
caches and throws it away?

[Ryan Sleevi, 2016/08/31 20:33:13]

We were sharing (de-duping) the first X509* (X509*[1]) from a socket, since
InsertOrUpdate() would add it to the X509CertificateCache. However, because each
SSL* creates a distinct X509*, the second (and subsequent) sockets would end up
calling InsertOrUpdate(), which would:
A) Call GetDER on X509*[2], which caches the DER on X509*[2]
B) See that X509*[2] is the same as X509*[1]
C) Replace X509Certificate's version with X509*[1] and free Chromium's copy of
X509*[2]

However, SSL*[2] still holds X509*[2], so it ended up keeping X509*[2] (and the
DER cache) from A alive until the socket was disconnected. That was the dupe.

So the end result is we lose 1 de-dup if we only had a single socket to the host
with the cert, but if we had >1 socket, we win. It seems safe to assume that, in
aggregate, we'll win here (as reflected in Dimitry's tests), although yes, we
lose out for single-socket H2 connections.

-  // When OSCertHandle is typedef'ed to X509, this implementation does a short
-  // cut to avoid converting back and forth between DER and the X509 struct.
-  X509Certificate::OSCertHandles intermediates;
-  for (size_t i = 1; i < sk_X509_num(openssl_chain_.get()); ++i) {
-    intermediates.push_back(sk_X509_value(openssl_chain_.get(), i));
+  // DER-encode the chain and convert to a platform certificate handle.
+  std::vector<std::string> chain;
+  std::vector<base::StringPiece> stringpiece_chain;
+  for (size_t i = 0; i < sk_X509_num(openssl_chain_.get()); ++i) {
+    X509* x = sk_X509_value(openssl_chain_.get(), i);
+    // Note: This intentionally avoids using x509_util::GetDER(), which may
+    // cache the encoded DER on |x|, as |x| is shared with the underlying
+    // socket (SSL*) this chain belongs to. As the DER will only be used
+    // once in //net, within this code, this avoids needlessly caching
+    // additional data. See https://crbug.com/642082
+    int len = i2d_X509(x, nullptr);
+    if (len < 0)
+      return nullptr;
+    std::string cert;
+    uint8_t* ptr = reinterpret_cast<uint8_t*>(base::WriteInto(&cert, len + 1));
+    if (i2d_X509(x, &ptr) < 0) {
+      NOTREACHED();
+      return nullptr;
+    }
+    chain.emplace_back(cert);
+    stringpiece_chain.push_back(chain.back());
   }
 
-  return X509Certificate::CreateFromHandle(
-      sk_X509_value(openssl_chain_.get(), 0), intermediates);
-#else
-  // DER-encode the chain and convert to a platform certificate handle.
-  std::vector<base::StringPiece> der_chain;
-  for (size_t i = 0; i < sk_X509_num(openssl_chain_.get()); ++i) {
-    X509* x = sk_X509_value(openssl_chain_.get(), i);
-    base::StringPiece der;
-    if (!x509_util::GetDER(x, &der))
-      return NULL;
-    der_chain.push_back(der);
-  }
-
-  return X509Certificate::CreateFromDERCertChain(der_chain);
-#endif
+  return X509Certificate::CreateFromDERCertChain(stringpiece_chain);
 }
 
 // static
 void SSLClientSocket::ClearSessionCache() {
   SSLClientSocketImpl::SSLContext* context =
       SSLClientSocketImpl::SSLContext::GetInstance();
   context->session_cache()->Flush();
 }
 
 SSLClientSocketImpl::SSLClientSocketImpl(
@@ skipping 801 matching lines @@
 
 int SSLClientSocketImpl::DoVerifyCert(int result) {
   DCHECK(!server_cert_chain_->empty());
   DCHECK(start_cert_verification_time_.is_null());
 
   next_handshake_state_ = STATE_VERIFY_CERT_COMPLETE;
 
   // OpenSSL decoded the certificate, but the platform certificate
   // implementation could not. This is treated as a fatal SSL-level protocol
   // error rather than a certificate error. See https://crbug.com/91341.
-  if (!server_cert_.get())
+  if (!server_cert_)
     return ERR_SSL_SERVER_CERT_BAD_FORMAT;
 
   // If the certificate is bad and has been previously accepted, use
   // the previous status and bypass the error.
-  base::StringPiece der_cert;
-  if (!x509_util::GetDER(server_cert_chain_->Get(0), &der_cert)) {
-    NOTREACHED();
-    return ERR_CERT_INVALID;
-  }
   CertStatus cert_status;
-  if (ssl_config_.IsAllowedBadCert(der_cert, &cert_status)) {
+  if (ssl_config_.IsAllowedBadCert(server_cert_.get(), &cert_status)) {
     server_cert_verify_result_.Reset();
     server_cert_verify_result_.cert_status = cert_status;
     server_cert_verify_result_.verified_cert = server_cert_;
     return OK;
   }
 
   start_cert_verification_time_ = base::TimeTicks::Now();
 
   return cert_verifier_->Verify(
       CertVerifier::RequestParams(server_cert_, host_and_port_.host(),
@@ skipping 1064 matching lines @@
   }
   UMA_HISTOGRAM_ENUMERATION("DomainBoundCerts.Support", supported,
                             CHANNEL_ID_USAGE_MAX);
 }
 
 bool SSLClientSocketImpl::IsChannelIDEnabled() const {
   return ssl_config_.channel_id_enabled && channel_id_service_;
 }
 
 }  // namespace net