OpenSSL: don't allow the server certificate to change during renegotiation.

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 b253dfeae50884859e2dceb3657363d2a6834b29..a76775d15eb3be25adfd5d7c037d396c06b6898d 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -183,6 +183,10 @@ int MapOpenSSLErrorSSL() {
     case SSL_R_TLSV1_ALERT_RECORD_OVERFLOW:
     case SSL_R_TLSV1_ALERT_USER_CANCELLED:
       return ERR_SSL_PROTOCOL_ERROR;
+    case SSL_R_CERTIFICATE_VERIFY_FAILED:
+      // The only way that the certificate verify callback can fail is if
+      // the leaf certificate changed during a renegotiation.
+      return ERR_SSL_SERVER_CERT_CHANGED;
     default:
       LOG(WARNING) << "Unmapped error reason: " << ERR_GET_REASON(error_code);
       return ERR_FAILED;
@@ -212,13 +216,6 @@ int MapOpenSSLError(int err, const crypto::OpenSSLErrStackTracer& tracer) {
   }
 }
 
-// We do certificate verification after handshake, so we disable the default
-// by registering a no-op verify function.
-int NoOpVerifyCallback(X509_STORE_CTX*, void *) {
-  DVLOG(3) << "skipping cert verify";
-  return 1;
-}
-
 // Utility to construct the appropriate set & clear masks for use the OpenSSL
 // options and mode configuration functions. (SSL_set_options etc)
 struct SslSetClearMask {
@@ -270,9 +267,10 @@ class SSLClientSocketOpenSSL::SSLContext {
     DCHECK_NE(ssl_socket_data_index_, -1);
     ssl_ctx_.reset(SSL_CTX_new(SSLv23_client_method()));
     session_cache_.Reset(ssl_ctx_.get(), kDefaultSessionCacheConfig);
-    SSL_CTX_set_cert_verify_callback(ssl_ctx_.get(), NoOpVerifyCallback, NULL);
+    SSL_CTX_set_cert_verify_callback(ssl_ctx_.get(), CertificateCallback, NULL);
     SSL_CTX_set_client_cert_cb(ssl_ctx_.get(), ClientCertCallback);
     SSL_CTX_set_channel_id_cb(ssl_ctx_.get(), ChannelIDCallback);
+    SSL_CTX_set_verify(ssl_ctx_.get(), SSL_VERIFY_PEER, NULL);
 #if defined(OPENSSL_NPN_NEGOTIATED)
     // TODO(kristianm): Only select this if ssl_config_.next_proto is not empty.
     // It would be better if the callback were not a global setting,
@@ -302,6 +300,15 @@ class SSLClientSocketOpenSSL::SSLContext {
     socket->ChannelIDRequestCallback(ssl, pkey);
   }
 
+  static int CertificateCallback(X509_STORE_CTX *store_ctx, void *arg) {

[wtc, 2014/02/27 22:12:07]

Nit: also rename this function "CertVerifyCallback".

[agl, 2014/02/27 22:31:23]

Done.

+    SSL* ssl = reinterpret_cast<SSL*>(X509_STORE_CTX_get_ex_data(
+        store_ctx, SSL_get_ex_data_X509_STORE_CTX_idx()));
+    SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
+    CHECK(socket);
+
+    return socket->CertificateCallback(store_ctx);
+  }
+
   static int SelectNextProtoCallback(SSL* ssl,
                                      unsigned char** out, unsigned char* outlen,
                                      const unsigned char* in,
@@ -1372,6 +1379,44 @@ void SSLClientSocketOpenSSL::ChannelIDRequestCallback(SSL* ssl,
   *pkey = EVP_PKEY_dup(ec_private_key->key());
 }
 
+int SSLClientSocketOpenSSL::CertificateCallback(X509_STORE_CTX* store_ctx) {
+  if (!completed_handshake_) {
+    // If the first handshake hasn't completed then we accept any certificates
+    // because we verify after the handshake.
+    return 1;
+  }
+
+  std::string der_current_cert;
+  if (!X509Certificate::GetDEREncoded(server_cert_->os_cert_handle(),
+                                      &der_current_cert)) {
+    LOG(ERROR) << "Failed to get current certificate in DER form";
+    return 0;
+  }
+
+  X509* leaf_cert = sk_X509_value(store_ctx->chain, 0);
+  int len = i2d_X509(leaf_cert, NULL);
+  if (len < 0) {
+    LOG(ERROR) << "Failed to marshal certificate from renegotiation";
+    return 0;
+  }
+
+  if (static_cast<size_t>(len) == der_current_cert.size()) {
+    scoped_ptr<uint8[]> der_leaf_cert(new uint8[len]);
+    uint8 *outp = der_leaf_cert.get();
+    i2d_X509(leaf_cert, &outp);

[wtc, 2014/02/27 22:12:07]

Nit: should we also check the return value of the second i2d_X509 call, as
defensive programming? (I think it is unlikely to return different value
though.)

One way to do this is (starting from line 1397:

  int len = i2d_X509(leaf_cert, NULL);
  if (len < 0) {
    ...
  }
  scoped_ptr<uint8[]> der_leaf_cert(new uint8[len]);
  uint8 *outp = der_leaf_cert.get();
  len = i2d_X509(leaf_cert, &outp);
  if (static_cast<size_t>(len) == der_current_cert.size() &&
      memcmp(der_leaf_cert.get(),
             ...
             der_current_cert.size()) == 0) {
    // The certificates match so the renegotiation can continue.
    return 1;
  }

  LOG(ERROR) << "Server certificate changed between handshakes";
  return 0;
  

[agl, 2014/02/27 22:31:23]

That's a fair point. I was trying to avoid running i2d_X509 twice, but since
it's only happening on renegotiations it's not worth worrying about.

Done.

+
+    if (memcmp(der_leaf_cert.get(),
+               der_current_cert.data(),
+               der_current_cert.size()) == 0) {
+      // The certificates match so the renegotiation can continue.
+      return 1;
+    }
+  }
+
+  LOG(ERROR) << "Server certificate changed between handshakes";
+  return 0;
+}
+
 // 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