Only verify the certificate once.

WebRTC is currently using the SSL_CTX_set_verify callback. This
configures a callback for use with X509_STORE_CTX_set_verify_cb. See
https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_set_verify_cb.html

This callback does not override certificate verification. Rather, it
allows EACH failure in OpenSSL's built-in certificate verification, as
well as the final success, to be overridden (that's why there's an ok
parameter). It still runs the usual OpenSSL certificate verification
(which will never succeed).

The upshot is that the callback is called multiple times and
OpenSSLStreamAdapter does a ton of redundant work and checks the hash at
least twice, or more for certificates with other errors.

Instead, use SSL_CTX_set_cert_verify_callback. This short-circuits the
OpenSSL behavior entirely and uses a caller-supplied one.
https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#SSL_CTX_set_cert_verify_callback
https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_cert_verify_callback(3)

(This also removes the SSL_CTX_set_verify_depth call which is ignored
with SSL_CTX_set_cert_verify_callback. It didn't do anything before
either---it tells OpenSSL to reject chains that are too short, but the
rejection was overwritten by the callback anyway.)

(Later on, we'll need to switch this to the BoringSSL-only
SSL_CTX_set_custom_verify and CRYPTO_BUFFER APIs to fix WebRTC's
contribution to Chrome's binary size, but I've left that alone for the
time being.)

Bug: none
Change-Id: I9320a367d0961935836df63dc6f0868b069f0af0
Reviewed-on: https://webrtc-review.googlesource.com/4581
Commit-Queue: David Benjamin <davidben@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20053}

rtc_base/opensslstreamadapter.cc

@@ -1050,8 +1050,13 @@ SSL_CTX* OpenSSLStreamAdapter::SetupSSLContext() {
     mode |= SSL_VERIFY_FAIL_IF_NO_PEER_CERT;
   }
 
-  SSL_CTX_set_verify(ctx, mode, SSLVerifyCallback);
+  // Configure a custom certificate verification callback to check the peer
-  SSL_CTX_set_verify_depth(ctx, 4);
+  // certificate digest. Note the second argument to SSL_CTX_set_verify is to
+  // override individual errors in the default verification logic, which is not
+  // what we want here.
+  SSL_CTX_set_verify(ctx, mode, nullptr);
+  SSL_CTX_set_cert_verify_callback(ctx, SSLVerifyCallback, nullptr);
+
   // Select list of available ciphers. Note that !SHA256 and !SHA384 only
   // remove HMAC-SHA256 and HMAC-SHA384 cipher suites, not GCM cipher suites
   // with SHA256 or SHA384 as the handshake hash.
@@ -1097,28 +1102,17 @@ bool OpenSSLStreamAdapter::VerifyPeerCertificate() {
   return true;
 }
 
-int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) {
+int OpenSSLStreamAdapter::SSLVerifyCallback(X509_STORE_CTX* store, void* arg) {
-  // Get our SSL structure from the store
+  // Get our SSL structure and OpenSSLStreamAdapter from the store.
   SSL* ssl = reinterpret_cast<SSL*>(
       X509_STORE_CTX_get_ex_data(store, SSL_get_ex_data_X509_STORE_CTX_idx()));
-  X509* cert = X509_STORE_CTX_get_current_cert(store);
-  int depth = X509_STORE_CTX_get_error_depth(store);
-
-  // For now we ignore the parent certificates and verify the leaf against
-  // the digest.
-  //
-  // TODO(jiayl): Verify the chain is a proper chain and report the chain to
-  // |stream->peer_certificate_|.
-  if (depth > 0) {
-    LOG(LS_INFO) << "Ignored chained certificate at depth " << depth;
-    return 1;
-  }
-
   OpenSSLStreamAdapter* stream =
       reinterpret_cast<OpenSSLStreamAdapter*>(SSL_get_app_data(ssl));
 
   // Record the peer's certificate.
+  X509* cert = SSL_get_peer_certificate(ssl);
   stream->peer_certificate_.reset(new OpenSSLCertificate(cert));
+  X509_free(cert);
 
   // If the peer certificate digest isn't known yet, we'll wait to verify
   // until it's known, and for now just return a success status.
@@ -1127,7 +1121,12 @@ int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) {
     return 1;
   }
 
-  return stream->VerifyPeerCertificate();
+  if (!stream->VerifyPeerCertificate()) {
+    X509_STORE_CTX_set_error(store, X509_V_ERR_CERT_REJECTED);
+    return 0;
+  }
+
+  return 1;
 }
 
 bool OpenSSLStreamAdapter::IsBoringSsl() {

rtc_base/opensslstreamadapter.h

@@ -169,11 +169,9 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter {
   SSL_CTX* SetupSSLContext();
   // Verify the peer certificate matches the signaled digest.
   bool VerifyPeerCertificate();
-  // SSL certification verification error handler, called back from
+  // SSL certificate verification callback. See
-  // the openssl library. Returns an int interpreted as a boolean in
+  // SSL_CTX_set_cert_verify_callback.
-  // the C style: zero means verification failure, non-zero means
+  static int SSLVerifyCallback(X509_STORE_CTX* store, void* arg);
-  // passed.
-  static int SSLVerifyCallback(int ok, X509_STORE_CTX* store);
 
   bool waiting_to_verify_peer_certificate() const {
     return client_auth_enabled() && !peer_certificate_verified_;