Only use the platform cert in verification in SSLClientSocketOpenSSL.

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index 2e011d97997d551dc780eded34b235bd7440278f..4db7ef2527c1dfbdc8e1c4f9cecf458b685049c9 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -18,6 +18,7 @@
 #include "base/memory/singleton.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_piece.h"
+#include "base/strings/string_util.h"
 #include "base/synchronization/lock.h"
 #include "crypto/ec_private_key.h"
 #include "crypto/openssl_util.h"
@@ -134,6 +135,21 @@ ScopedX509Stack OSCertHandlesToOpenSSL(
   return stack.Pass();
 }
 
+// DER-encodes an OpenSSL X509 structure. Returns true on success.
+bool GetDEREncodedX509(X509* x509, std::string* out_der) {

[Ryan Sleevi, 2014/09/18 01:34:39]

This seems to defeat the purpose of
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_cert...

I'm wondering whether that layer should move into the common x509_util_openssl
from x509_certificate_openssl. Thoughts?

[davidben, 2014/09/18 19:40:06]

Hrm, that's a thought. It does bother me that we have a lot of handy logic for
X509s that are only available in USE_OPENSSL_CERTS ports. Done.

+  int len = i2d_X509(x509, NULL);
+  if (len < 0)
+    return false;
+
+  uint8_t* ptr = reinterpret_cast<uint8_t*>(WriteInto(out_der, len + 1));
+  if (i2d_X509(x509, &ptr) < 0) {
+    NOTREACHED();
+    out_der->clear();
+    return false;
+  }
+  return true;
+}
+
 int LogErrorCallback(const char* str, size_t len, void* context) {
   LOG(ERROR) << base::StringPiece(str, len);
   return 1;
@@ -251,7 +267,7 @@ class SSLClientSocketOpenSSL::PeerCertificateChain {
   void Reset(STACK_OF(X509)* chain);
 
   // Note that when USE_OPENSSL is defined, OSCertHandle is X509*
-  const scoped_refptr<X509Certificate>& AsOSChain() const { return os_chain_; }
+  scoped_refptr<X509Certificate> AsOSChain() const;
 
   size_t size() const {
     if (!openssl_chain_.get())
@@ -259,17 +275,13 @@ class SSLClientSocketOpenSSL::PeerCertificateChain {
     return sk_X509_num(openssl_chain_.get());
   }
 
-  X509* operator[](size_t index) const {
+  X509* Get(size_t index) const {
     DCHECK_LT(index, size());
     return sk_X509_value(openssl_chain_.get(), index);
   }
 
-  bool IsValid() { return os_chain_.get() && openssl_chain_.get(); }
-
  private:
   ScopedX509Stack openssl_chain_;
-
-  scoped_refptr<X509Certificate> os_chain_;
 };
 
 SSLClientSocketOpenSSL::PeerCertificateChain&
@@ -278,69 +290,41 @@ SSLClientSocketOpenSSL::PeerCertificateChain::operator=(
   if (this == &other)
     return *this;
 
-  // os_chain_ is reference counted by scoped_refptr;
-  os_chain_ = other.os_chain_;
-
   openssl_chain_.reset(X509_chain_up_ref(other.openssl_chain_.get()));
-
   return *this;
 }
 
-#if defined(USE_OPENSSL_CERTS)
-// When OSCertHandle is typedef'ed to X509, this implementation does a short cut
-// to avoid converting back and forth between der and X509 struct.
 void SSLClientSocketOpenSSL::PeerCertificateChain::Reset(
     STACK_OF(X509)* chain) {

[Ryan Sleevi, 2014/09/18 01:34:39]

You (accidentally?) nuked a NULL check & ability to Reset(NULL).

[davidben, 2014/09/18 19:40:06]

I checked and X509_chain_up_ref will handle a NULL input properly. But it's
probably better to be explicit here, so I added a check.

-  openssl_chain_.reset(NULL);
-  os_chain_ = NULL;
-
-  if (!chain)
-    return;
-
-  X509Certificate::OSCertHandles intermediates;
-  for (size_t i = 1; i < sk_X509_num(chain); ++i)
-    intermediates.push_back(sk_X509_value(chain, i));
-
-  os_chain_ =
-      X509Certificate::CreateFromHandle(sk_X509_value(chain, 0), intermediates);
-
   openssl_chain_.reset(X509_chain_up_ref(chain));
 }
-#else  // !defined(USE_OPENSSL_CERTS)
-void SSLClientSocketOpenSSL::PeerCertificateChain::Reset(
-    STACK_OF(X509)* chain) {
-  openssl_chain_.reset(NULL);
-  os_chain_ = NULL;
 
-  if (!chain)
-    return;
-
-  openssl_chain_.reset(X509_chain_up_ref(chain));
+scoped_refptr<X509Certificate>
+SSLClientSocketOpenSSL::PeerCertificateChain::AsOSChain() const {
+#if defined(USE_OPENSSL_CERTS)
+  // 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));
+  }
 
+  return make_scoped_refptr(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;
+  std::vector<std::string> storage(sk_X509_num(openssl_chain_.get()));
   for (size_t i = 0; i < sk_X509_num(openssl_chain_.get()); ++i) {
     X509* x = sk_X509_value(openssl_chain_.get(), i);
-
-    unsigned char* cert_data = NULL;
-    int cert_data_length = i2d_X509(x, &cert_data);
-    if (cert_data_length && cert_data)
-      der_chain.push_back(base::StringPiece(reinterpret_cast<char*>(cert_data),
-                                            cert_data_length));
-  }
-
-  os_chain_ = X509Certificate::CreateFromDERCertChain(der_chain);
-
-  for (size_t i = 0; i < der_chain.size(); ++i) {
-    OPENSSL_free(const_cast<char*>(der_chain[i].data()));
+    if (!GetDEREncodedX509(x, &storage[i]))
+      return NULL;
+    der_chain.push_back(base::StringPiece(storage[i]));
   }
 
-  if (der_chain.size() !=
-      static_cast<size_t>(sk_X509_num(openssl_chain_.get()))) {
-    openssl_chain_.reset(NULL);
-    os_chain_ = NULL;
-  }
+  return make_scoped_refptr(X509Certificate::CreateFromDERCertChain(der_chain));
+#endif
 }
-#endif  // defined(USE_OPENSSL_CERTS)
 
 // static
 SSLSessionCacheOpenSSL::Config
@@ -613,7 +597,7 @@ bool SSLClientSocketOpenSSL::UsingTCPFastOpen() const {
 
 bool SSLClientSocketOpenSSL::GetSSLInfo(SSLInfo* ssl_info) {
   ssl_info->Reset();
-  if (!server_cert_.get())
+  if (server_cert_chain_->size() == 0)
     return false;
 
   ssl_info->cert = server_cert_verify_result_.verified_cert;
@@ -961,12 +945,7 @@ int SSLClientSocketOpenSSL::DoHandshake() {
     set_signed_cert_timestamps_received(sct_list_len != 0);
 
     // Verify the certificate.
-    const bool got_cert = !!UpdateServerCert();
-    DCHECK(got_cert);
-    net_log_.AddEvent(
-        NetLog::TYPE_SSL_CERTIFICATES_RECEIVED,
-        base::Bind(&NetLogX509CertificateCallback,
-                   base::Unretained(server_cert_.get())));
+    UpdateServerCert();
     GotoState(STATE_VERIFY_CERT);
   } else {
     int ssl_error = SSL_get_error(ssl_, rv);
@@ -1049,12 +1028,20 @@ int SSLClientSocketOpenSSL::DoChannelIDLookupComplete(int result) {
 }
 
 int SSLClientSocketOpenSSL::DoVerifyCert(int result) {
-  DCHECK(server_cert_.get());
+  DCHECK_NE(0u, server_cert_chain_->size());
   DCHECK(start_cert_verification_time_.is_null());
+
   GotoState(STATE_VERIFY_CERT_COMPLETE);
 
+  // If the certificate is expected to be bad we can use the expectation as
+  // the cert status.

[Ryan Sleevi, 2014/09/18 01:34:39]

Don't use we in comments

// If the certificate is bad and has been previously accepted, use the previous
status and bypass
// the error.

[davidben, 2014/09/18 19:40:06]

Done. (I just pulled this from SSLClientSocketNSS.)

+  std::string der_cert;
+  if (!GetDEREncodedX509(server_cert_chain_->Get(0), &der_cert)) {
+    NOTREACHED();
+    return ERR_CERT_INVALID;
+  }
   CertStatus cert_status;
-  if (ssl_config_.IsAllowedBadCert(server_cert_.get(), &cert_status)) {
+  if (ssl_config_.IsAllowedBadCert(der_cert, &cert_status)) {
     VLOG(1) << "Received an expected bad cert with status: " << cert_status;
     server_cert_verify_result_.Reset();
     server_cert_verify_result_.cert_status = cert_status;
@@ -1062,6 +1049,14 @@ int SSLClientSocketOpenSSL::DoVerifyCert(int result) {
     return OK;
   }
 
+  // We may have failed to create X509Certificate object if we are
+  // running inside sandbox.

[Ryan Sleevi, 2014/09/18 01:34:39]

// When running in a sandbox, it may not be possible to create an
X509Certificate*, as that may
// depend on OS functionality blocked in the sandbox.

[davidben, 2014/09/18 19:40:06]

Done. (Also just pulled from NSS.)

+  if (!server_cert_.get()) {
+    server_cert_verify_result_.Reset();
+    server_cert_verify_result_.cert_status = CERT_STATUS_INVALID;
+    return ERR_CERT_INVALID;
+  }
+
   start_cert_verification_time_ = base::TimeTicks::Now();
 
   int flags = 0;
@@ -1147,14 +1142,16 @@ void SSLClientSocketOpenSSL::DoConnectCallback(int rv) {
   }
 }
 
-X509Certificate* SSLClientSocketOpenSSL::UpdateServerCert() {
+void SSLClientSocketOpenSSL::UpdateServerCert() {
   server_cert_chain_->Reset(SSL_get_peer_cert_chain(ssl_));
   server_cert_ = server_cert_chain_->AsOSChain();
 
-  if (!server_cert_chain_->IsValid())
-    DVLOG(1) << "UpdateServerCert received invalid certificate chain from peer";
-
-  return server_cert_.get();
+  if (server_cert_.get()) {
+    net_log_.AddEvent(
+        NetLog::TYPE_SSL_CERTIFICATES_RECEIVED,
+        base::Bind(&NetLogX509CertificateCallback,
+                   base::Unretained(server_cert_.get())));
+  }
 }
 
 void SSLClientSocketOpenSSL::VerifyCT() {
@@ -1619,17 +1616,18 @@ int SSLClientSocketOpenSSL::CertVerifyCallback(X509_STORE_CTX* store_ctx) {
     return 1;
   }
 
-  CHECK(server_cert_.get());
-
-  PeerCertificateChain chain(store_ctx->untrusted);
-  if (chain.IsValid() && server_cert_->Equals(chain.AsOSChain().get()))
-    return 1;
-
-  if (!chain.IsValid())
+  // Disallow the server certificate to change in a renegotiation.
+  if (server_cert_chain_->size() < 1) {

[Ryan Sleevi, 2014/09/18 01:34:39]

.empty() optimization?

[davidben, 2014/09/18 19:40:06]

Added a empty() method (matches NSS too) if that's what you meant.

     LOG(ERROR) << "Received invalid certificate chain between handshakes";
-  else
+    return 0;
+  }
+  if (store_ctx->cert == NULL ||
+      X509_cmp(server_cert_chain_->Get(0), store_ctx->cert) != 0) {
     LOG(ERROR) << "Server certificate changed between handshakes";
-  return 0;
+    return 0;
+  }
+
+  return 1;
 }
 
 // SelectNextProtoCallback is called by OpenSSL during the handshake. If the