Only use the platform cert in verification in SSLClientSocketOpenSSL.

Align with SSLClientSocketNSS's behavior around the PeerCertificate object.
Within the sandbox for Chromoting, the platform certificate may be unavailable
while the SSL-layer certificate is. Match NSS in using the SSL-layer
certificate for many operations. Also check appropriately for the platform
certificate being NULL in DoVerifyCert rather than simply crashing.

BUG=414315
TEST=Install Chromoting app. Connecting to another computer works.

Review URL: https://codereview.chromium.org/576233002

Cr-Commit-Position: refs/heads/master@{#295764}

net/cert/x509_certificate_openssl.cc

@@ -17,6 +17,7 @@
 #include "base/pickle.h"
 #include "base/sha1.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "crypto/openssl_util.h"
 #include "crypto/scoped_openssl_types.h"
@@ -135,21 +136,6 @@ void ParseSubjectAltName(X509Certificate::OSCertHandle cert,
   }
 }
 
-struct DERCache {
-  unsigned char* data;
-  int data_length;
-};
-
-void DERCache_free(void* parent, void* ptr, CRYPTO_EX_DATA* ad, int idx,
-                   long argl, void* argp) {
-  DERCache* der_cache = static_cast<DERCache*>(ptr);
-  if (!der_cache)
-      return;
-  if (der_cache->data)
-      OPENSSL_free(der_cache->data);
-  OPENSSL_free(der_cache);
-}
-
 class X509InitSingleton {
  public:
   static X509InitSingleton* GetInstance() {
@@ -159,7 +145,6 @@ class X509InitSingleton {
     return Singleton<X509InitSingleton,
                      LeakySingletonTraits<X509InitSingleton> >::get();
   }
-  int der_cache_ex_index() const { return der_cache_ex_index_; }
   X509_STORE* store() const { return store_.get(); }
 
   void ResetCertStore() {
@@ -173,63 +158,14 @@ class X509InitSingleton {
   friend struct DefaultSingletonTraits<X509InitSingleton>;
   X509InitSingleton() {
     crypto::EnsureOpenSSLInit();
-    der_cache_ex_index_ = X509_get_ex_new_index(0, 0, 0, 0, DERCache_free);
-    DCHECK_NE(der_cache_ex_index_, -1);
     ResetCertStore();
   }
 
-  int der_cache_ex_index_;
   crypto::ScopedOpenSSL<X509_STORE, X509_STORE_free>::Type store_;
 
   DISALLOW_COPY_AND_ASSIGN(X509InitSingleton);
 };
 
-// Takes ownership of |data| (which must have been allocated by OpenSSL).
-DERCache* SetDERCache(X509Certificate::OSCertHandle cert,
-                      int x509_der_cache_index,
-                      unsigned char* data,
-                      int data_length) {
-  DERCache* internal_cache = static_cast<DERCache*>(
-      OPENSSL_malloc(sizeof(*internal_cache)));
-  if (!internal_cache) {
-    // We took ownership of |data|, so we must free if we can't add it to
-    // |cert|.
-    OPENSSL_free(data);
-    return NULL;
-  }
-
-  internal_cache->data = data;
-  internal_cache->data_length = data_length;
-  X509_set_ex_data(cert, x509_der_cache_index, internal_cache);
-  return internal_cache;
-}
-
-// Returns true if |der_cache| points to valid data, false otherwise.
-// (note: the DER-encoded data in |der_cache| is owned by |cert|, callers should
-// not free it).
-bool GetDERAndCacheIfNeeded(X509Certificate::OSCertHandle cert,
-                            DERCache* der_cache) {
-  int x509_der_cache_index =
-      X509InitSingleton::GetInstance()->der_cache_ex_index();
-
-  // Re-encoding the DER data via i2d_X509 is an expensive operation, but it's
-  // necessary for comparing two certificates. We re-encode at most once per
-  // certificate and cache the data within the X509 cert using X509_set_ex_data.
-  DERCache* internal_cache = static_cast<DERCache*>(
-      X509_get_ex_data(cert, x509_der_cache_index));
-  if (!internal_cache) {
-    unsigned char* data = NULL;
-    int data_length = i2d_X509(cert, &data);
-    if (data_length <= 0 || !data)
-      return false;
-    internal_cache = SetDERCache(cert, x509_der_cache_index, data, data_length);
-    if (!internal_cache)
-      return false;
-  }
-  *der_cache = *internal_cache;
-  return true;
-}
-
 // Used to free a list of X509_NAMEs and the objects it points to.
 void sk_X509_NAME_free_all(STACK_OF(X509_NAME)* sk) {
   sk_X509_NAME_pop_free(sk, X509_NAME_free);
@@ -301,11 +237,11 @@ SHA1HashValue X509Certificate::CalculateCAFingerprint(
 
   SHA_CTX sha1_ctx;
   SHA1_Init(&sha1_ctx);
-  DERCache der_cache;
+  base::StringPiece der;
   for (size_t i = 0; i < intermediates.size(); ++i) {
-    if (!GetDERAndCacheIfNeeded(intermediates[i], &der_cache))
+    if (!x509_util::GetDER(intermediates[i], &der))
       return sha1;
-    SHA1_Update(&sha1_ctx, der_cache.data, der_cache.data_length);
+    SHA1_Update(&sha1_ctx, der.data(), der.length());
   }
   SHA1_Final(sha1.data, &sha1_ctx);
 
@@ -320,8 +256,8 @@ X509Certificate::OSCertHandle X509Certificate::CreateOSCertHandleFromBytes(
   crypto::EnsureOpenSSLInit();
   const unsigned char* d2i_data =
       reinterpret_cast<const unsigned char*>(data);
-  // Don't cache this data via SetDERCache as this wire format may be not be
-  // identical from the i2d_X509 roundtrip.
+  // Don't cache this data for x509_util::GetDER as this wire format
+  // may be not be identical from the i2d_X509 roundtrip.
   X509* cert = d2i_X509(NULL, &d2i_data, length);
   return cert;
 }
@@ -372,11 +308,10 @@ X509_STORE* X509Certificate::cert_store() {
 // static
 bool X509Certificate::GetDEREncoded(X509Certificate::OSCertHandle cert_handle,
                                     std::string* encoded) {
-  DERCache der_cache;
-  if (!GetDERAndCacheIfNeeded(cert_handle, &der_cache))
+  base::StringPiece der;
+  if (!x509_util::GetDER(cert_handle, &der))
     return false;
-  encoded->assign(reinterpret_cast<const char*>(der_cache.data),
-                  der_cache.data_length);
+  encoded->assign(der.data(), der.length());
   return true;
 }
 
@@ -390,12 +325,11 @@ bool X509Certificate::IsSameOSCert(X509Certificate::OSCertHandle a,
   // X509_cmp only checks the fingerprint, but we want to compare the whole
   // DER data. Encoding it from OSCertHandle is an expensive operation, so we
   // cache the DER (if not already cached via X509_set_ex_data).
-  DERCache der_cache_a, der_cache_b;
+  base::StringPiece der_a, der_b;
 
-  return GetDERAndCacheIfNeeded(a, &der_cache_a) &&
-      GetDERAndCacheIfNeeded(b, &der_cache_b) &&
-      der_cache_a.data_length == der_cache_b.data_length &&
-      memcmp(der_cache_a.data, der_cache_b.data, der_cache_a.data_length) == 0;
+  return x509_util::GetDER(a, &der_a) &&
+      x509_util::GetDER(b, &der_b) &&
+      der_a == der_b;
 }
 
 // static
@@ -412,13 +346,11 @@ X509Certificate::ReadOSCertHandleFromPickle(PickleIterator* pickle_iter) {
 // static
 bool X509Certificate::WriteOSCertHandleToPickle(OSCertHandle cert_handle,
                                                 Pickle* pickle) {
-  DERCache der_cache;
-  if (!GetDERAndCacheIfNeeded(cert_handle, &der_cache))
+  base::StringPiece der;
+  if (!x509_util::GetDER(cert_handle, &der))
     return false;
 
-  return pickle->WriteData(
-      reinterpret_cast<const char*>(der_cache.data),
-      der_cache.data_length);
+  return pickle->WriteData(der.data(), der.length());
 }
 
 // static

net/cert/x509_util_openssl.cc

@@ -10,6 +10,7 @@
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/strings/string_piece.h"
+#include "base/strings/string_util.h"
 #include "crypto/ec_private_key.h"
 #include "crypto/openssl_util.h"
 #include "crypto/rsa_private_key.h"
@@ -127,6 +128,22 @@ X509* CreateCertificate(EVP_PKEY* key,
   return cert.release();
 }
 
+// DER-encodes |x509|. On success, returns true and writes the
+// encoding to |*out_der|.
+bool DerEncodeCert(X509* x509, std::string* out_der) {
+  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;
+}
+
 bool SignAndDerEncodeCert(X509* cert,
                           EVP_PKEY* key,
                           DigestAlgorithm alg,
@@ -145,17 +162,7 @@ bool SignAndDerEncodeCert(X509* cert,
   }
 
   // Convert it into a DER-encoded string copied to |der_encoded|.
-  int der_data_length = i2d_X509(cert, NULL);
-  if (der_data_length < 0)
-    return false;
-
-  der_encoded->resize(der_data_length);
-  unsigned char* der_data =
-      reinterpret_cast<unsigned char*>(&(*der_encoded)[0]);
-  if (i2d_X509(cert, &der_data) < 0)
-    return false;
-
-  return true;
+  return DerEncodeCert(cert, der_encoded);
 }
 
 // There is no OpenSSL NID for the 'originBoundCertificate' extension OID yet,
@@ -188,6 +195,36 @@ ASN1_OBJECT* GetDomainBoundOid() {
   return s_lazy.Get().obj();
 }
 
+
+struct DERCache {
+  std::string data;
+};
+
+void DERCache_free(void* parent, void* ptr, CRYPTO_EX_DATA* ad, int idx,
+                   long argl, void* argp) {
+  DERCache* der_cache = static_cast<DERCache*>(ptr);
+  delete der_cache;
+}
+
+class DERCacheInitSingleton {
+ public:
+  DERCacheInitSingleton() {
+    crypto::EnsureOpenSSLInit();
+    der_cache_ex_index_ = X509_get_ex_new_index(0, 0, 0, 0, DERCache_free);
+    DCHECK_NE(-1, der_cache_ex_index_);
+  }
+
+  int der_cache_ex_index() const { return der_cache_ex_index_; }
+
+ private:
+  int der_cache_ex_index_;
+
+  DISALLOW_COPY_AND_ASSIGN(DERCacheInitSingleton);
+};
+
+base::LazyInstance<DERCacheInitSingleton>::Leaky g_der_cache_singleton =
+    LAZY_INSTANCE_INITIALIZER;
+
 }  // namespace
 
 bool IsSupportedValidityRange(base::Time not_valid_before,
@@ -356,6 +393,30 @@ bool ParseDate(ASN1_TIME* x509_time, base::Time* time) {
   return ParseCertificateDate(str_date, format, time);
 }
 
+// Returns true if |der_cache| points to valid data, false otherwise.
+// (note: the DER-encoded data in |der_cache| is owned by |cert|, callers should
+// not free it).
+bool GetDER(X509* x509, base::StringPiece* der_cache) {
+  int x509_der_cache_index =
+      g_der_cache_singleton.Get().der_cache_ex_index();
+
+  // Re-encoding the DER data via i2d_X509 is an expensive operation,
+  // but it's necessary for comparing two certificates. Re-encode at
+  // most once per certificate and cache the data within the X509 cert
+  // using X509_set_ex_data.
+  DERCache* internal_cache = static_cast<DERCache*>(
+      X509_get_ex_data(x509, x509_der_cache_index));
+  if (!internal_cache) {
+    scoped_ptr<DERCache> new_cache(new DERCache);
+    if (!DerEncodeCert(x509, &new_cache->data))
+      return false;
+    internal_cache = new_cache.get();
+    X509_set_ex_data(x509, x509_der_cache_index, new_cache.release());
+  }
+  *der_cache = base::StringPiece(internal_cache->data);
+  return true;
+}
+
 }  // namespace x509_util
 
 }  // namespace net

net/cert/x509_util_openssl.h

@@ -11,6 +11,7 @@
 #include <string>
 #include <vector>
 
+#include "base/strings/string_piece.h"
 #include "net/base/net_export.h"
 
 namespace base {
@@ -42,6 +43,15 @@ bool NET_EXPORT ParsePrincipalValueByNID(X509_NAME* name,
 
 bool NET_EXPORT ParseDate(ASN1_TIME* x509_time, base::Time* time);
 
+// DER-encodes |x509|, caching the encoding in a structure owned by
+// the X509. On success, returns true, and sets |*out_der| to point to
+// the encoding. The StringPiece is valid as long as |x509| is not
+// freed.
+//
+// Note: this caches the encoding, so |x509| must not be modified
+// after the first call to this function.
+bool NET_EXPORT GetDER(X509* x509, base::StringPiece* out_der);
+
 } // namespace x509_util
 
 } // namespace net

net/socket/ssl_client_socket_openssl.cc

@@ -27,6 +27,7 @@
 #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/cert/x509_util_openssl.h"
 #include "net/http/transport_security_state.h"
 #include "net/socket/ssl_session_cache_openssl.h"
 #include "net/ssl/openssl_ssl_util.h"
@@ -251,7 +252,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 +260,17 @@ class SSLClientSocketOpenSSL::PeerCertificateChain {
     return sk_X509_num(openssl_chain_.get());
   }
 
-  X509* operator[](size_t index) const {
+  bool empty() const {
+    return size() == 0;
+  }
+
+  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 +279,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) {
-  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));
+  openssl_chain_.reset(chain ? X509_chain_up_ref(chain) : NULL);
 }
-#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;
   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()));
+    base::StringPiece der;
+    if (!x509_util::GetDER(x, &der))
+      return NULL;
+    der_chain.push_back(der);
   }
 
-  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 +586,7 @@ bool SSLClientSocketOpenSSL::UsingTCPFastOpen() const {
 
 bool SSLClientSocketOpenSSL::GetSSLInfo(SSLInfo* ssl_info) {
   ssl_info->Reset();
-  if (!server_cert_.get())
+  if (server_cert_chain_->empty())
     return false;
 
   ssl_info->cert = server_cert_verify_result_.verified_cert;
@@ -961,12 +934,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 +1017,20 @@ int SSLClientSocketOpenSSL::DoChannelIDLookupComplete(int result) {
 }
 
 int SSLClientSocketOpenSSL::DoVerifyCert(int result) {
-  DCHECK(server_cert_.get());
+  DCHECK(!server_cert_chain_->empty());
   DCHECK(start_cert_verification_time_.is_null());
+
   GotoState(STATE_VERIFY_CERT_COMPLETE);
 
+  // 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(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 +1038,15 @@ int SSLClientSocketOpenSSL::DoVerifyCert(int result) {
     return OK;
   }
 
+  // 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.
+  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 +1132,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 +1606,24 @@ 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_->empty()) {
     LOG(ERROR) << "Received invalid certificate chain between handshakes";
-  else
+    return 0;
+  }
+  base::StringPiece old_der, new_der;
+  if (store_ctx->cert == NULL ||
+      !x509_util::GetDER(server_cert_chain_->Get(0), &old_der) ||
+      !x509_util::GetDER(store_ctx->cert, &new_der)) {
+    LOG(ERROR) << "Failed to encode certificates";
+    return 0;
+  }
+  if (old_der != new_der) {
     LOG(ERROR) << "Server certificate changed between handshakes";
-  return 0;
+    return 0;
+  }
+
+  return 1;
 }
 
 // SelectNextProtoCallback is called by OpenSSL during the handshake. If the

net/socket/ssl_client_socket_openssl.h

@@ -122,7 +122,7 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   int DoVerifyCert(int result);
   int DoVerifyCertComplete(int result);
   void DoConnectCallback(int result);
-  X509Certificate* UpdateServerCert();
+  void UpdateServerCert();
   void VerifyCT();
 
   void OnHandshakeIOComplete(int result);