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}diff --git a/rtc_base/opensslstreamadapter.cc b/rtc_base/opensslstreamadapter.cc
index 4efdd6f..ec9f238 100644
--- a/rtc_base/opensslstreamadapter.cc
+++ b/rtc_base/opensslstreamadapter.cc
@@ -1050,8 +1050,13 @@
mode |= SSL_VERIFY_FAIL_IF_NO_PEER_CERT;
}
- SSL_CTX_set_verify(ctx, mode, SSLVerifyCallback);
- SSL_CTX_set_verify_depth(ctx, 4);
+ // Configure a custom certificate verification callback to check the peer
+ // 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 @@
return true;
}
-int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) {
- // Get our SSL structure from the store
+int OpenSSLStreamAdapter::SSLVerifyCallback(X509_STORE_CTX* store, void* arg) {
+ // 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 @@
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() {