Only use the platform cert in verification in SSLClientSocketOpenSSL.

net/socket/ssl_client_socket_openssl.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.
 
 // OpenSSL binding for SSLClientSocket. The class layout and general principle
 // of operation is derived from SSLClientSocketNSS.
 
 #include "net/socket/ssl_client_socket_openssl.h"
 
 #include <errno.h>
 #include <openssl/bio.h>
 #include <openssl/err.h>
 #include <openssl/ssl.h>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/environment.h"
 #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"
 #include "crypto/scoped_openssl_types.h"
 #include "net/base/net_errors.h"
 #include "net/cert/cert_verifier.h"
 #include "net/cert/ct_verifier.h"
 #include "net/cert/single_request_cert_verifier.h"
 #include "net/cert/x509_certificate_net_log_param.h"
 #include "net/http/transport_security_state.h"
@@ skipping 96 matching lines @@
   ScopedX509Stack stack(sk_X509_new_null());
   for (size_t i = 0; i < os_handles.size(); i++) {
     ScopedX509 x509 = OSCertHandleToOpenSSL(os_handles[i]);
     if (!x509)
       return ScopedX509Stack();
     sk_X509_push(stack.get(), x509.release());
   }
   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;
 }
 
 }  // namespace
 
 class SSLClientSocketOpenSSL::SSLContext {
  public:
   static SSLContext* GetInstance() { return Singleton<SSLContext>::get(); }
@@ skipping 97 matching lines @@
   ~PeerCertificateChain() {}
   PeerCertificateChain& operator=(const PeerCertificateChain& other);
 
   // Resets the PeerCertificateChain to the set of certificates in|chain|,
   // which may be NULL, indicating to empty the store certificates.
   // Note: If an error occurs, such as being unable to parse the certificates,
   // this will behave as if Reset(NULL) was called.
   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())
       return 0;
     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&
 SSLClientSocketOpenSSL::PeerCertificateChain::operator=(
     const PeerCertificateChain& other) {
   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;
+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));
+  }
 
-  openssl_chain_.reset(X509_chain_up_ref(chain));
-
+  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));
+    if (!GetDEREncodedX509(x, &storage[i]))
+      return NULL;
+    der_chain.push_back(base::StringPiece(storage[i]));
   }
 
-  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 (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
     SSLClientSocketOpenSSL::SSLContext::kDefaultSessionCacheConfig = {
         &GetSessionCacheKey,  // key_func
         1024,                 // max_entries
         256,                  // expiration_check_count
         60 * 60,              // timeout_seconds
 };
 
@@ skipping 252 matching lines @@
 bool SSLClientSocketOpenSSL::UsingTCPFastOpen() const {
   if (transport_.get() && transport_->socket())
     return transport_->socket()->UsingTCPFastOpen();
 
   NOTREACHED();
   return false;
 }
 
 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;
   ssl_info->cert_status = server_cert_verify_result_.cert_status;
   ssl_info->is_issued_by_known_root =
       server_cert_verify_result_.is_issued_by_known_root;
   ssl_info->public_key_hashes =
     server_cert_verify_result_.public_key_hashes;
   ssl_info->client_cert_sent =
       ssl_config_.send_client_cert && ssl_config_.client_cert.get();
@@ skipping 327 matching lines @@
     size_t ocsp_response_len;
     SSL_get0_ocsp_response(ssl_, &ocsp_response, &ocsp_response_len);
     set_stapled_ocsp_response_received(ocsp_response_len != 0);
 
     uint8_t* sct_list;
     size_t sct_list_len;
     SSL_get0_signed_cert_timestamp_list(ssl_, &sct_list, &sct_list_len);
     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);
 
     if (ssl_error == SSL_ERROR_WANT_CHANNEL_ID_LOOKUP) {
       // The server supports channel ID. Stop to look one up before returning to
       // the handshake.
       channel_id_xtn_negotiated_ = true;
       GotoState(STATE_CHANNEL_ID_LOOKUP);
       return OK;
@@ skipping 62 matching lines @@
     return MapOpenSSLError(err, err_tracer);
   }
 
   // Return to the handshake.
   set_channel_id_sent(true);
   GotoState(STATE_HANDSHAKE);
   return OK;
 }
 
 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;
     server_cert_verify_result_.verified_cert = server_cert_;
     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;
   if (ssl_config_.rev_checking_enabled)
     flags |= CertVerifier::VERIFY_REV_CHECKING_ENABLED;
   if (ssl_config_.verify_ev_cert)
     flags |= CertVerifier::VERIFY_EV_CERT;
   if (ssl_config_.cert_io_enabled)
     flags |= CertVerifier::VERIFY_CERT_IO_ENABLED;
   if (ssl_config_.rev_checking_required_local_anchors)
@@ skipping 65 matching lines @@
 void SSLClientSocketOpenSSL::DoConnectCallback(int rv) {
   if (rv < OK)
     OnHandshakeCompletion();
   if (!user_connect_callback_.is_null()) {
     CompletionCallback c = user_connect_callback_;
     user_connect_callback_.Reset();
     c.Run(rv > OK ? OK : 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() {
   if (!cert_transparency_verifier_)
     return;
 
   uint8_t* ocsp_response_raw;
   size_t ocsp_response_len;
   SSL_get0_ocsp_response(ssl_, &ocsp_response_raw, &ocsp_response_len);
   std::string ocsp_response;
@@ skipping 444 matching lines @@
   return 1;
 }
 
 int SSLClientSocketOpenSSL::CertVerifyCallback(X509_STORE_CTX* store_ctx) {
   if (!completed_connect_) {
     // If the first handshake hasn't completed then we accept any certificates
     // because we verify after the handshake.
     return 1;
   }
 
-  CHECK(server_cert_.get());
+  // 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";
+    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;
+  }
 
-  PeerCertificateChain chain(store_ctx->untrusted);
-  if (chain.IsValid() && server_cert_->Equals(chain.AsOSChain().get()))
-    return 1;
-
-  if (!chain.IsValid())
-    LOG(ERROR) << "Received invalid certificate chain between handshakes";
-  else
-    LOG(ERROR) << "Server certificate changed between handshakes";
-  return 0;
+  return 1;
 }
 
 // SelectNextProtoCallback is called by OpenSSL during the handshake. If the
 // server supports NPN, selects a protocol from the list that the server
 // provides. According to third_party/openssl/openssl/ssl/ssl_lib.c, the
 // callback can assume that |in| is syntactically valid.
 int SSLClientSocketOpenSSL::SelectNextProtoCallback(unsigned char** out,
                                                     unsigned char* outlen,
                                                     const unsigned char* in,
                                                     unsigned int inlen) {
@@ skipping 130 matching lines @@
                                             ct::SCT_STATUS_LOG_UNKNOWN));
   }
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net