From bd917a13fb4704cfc7d3f8c6879b877d7e839dc2 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Tue, 14 Sep 2021 15:22:41 -0700 Subject: [PATCH] Don't invoke custom certificate verifier unless there is an error. The custom callback is intended to override errors, so there's no point in calling it if the status is ok. Calling it during an otherwise successful verification was an unintentional change from: https://webrtc-review.googlesource.com/c/src/+/196941 This is misleading as the return value isn't even used. Bug: chromium:1247577 Change-Id: Id74411f7364537a3225021e7631bc9ab962889ee Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/231881 Commit-Queue: Taylor Brandstetter Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#34994} --- rtc_base/openssl_adapter.cc | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/rtc_base/openssl_adapter.cc b/rtc_base/openssl_adapter.cc index 93e578057d..7489bc992d 100644 --- a/rtc_base/openssl_adapter.cc +++ b/rtc_base/openssl_adapter.cc @@ -868,11 +868,11 @@ int OpenSSLAdapter::SSLVerifyCallback(int status, X509_STORE_CTX* store) { return status; } -int OpenSSLAdapter::SSLVerifyInternal(int status_on_failure, +int OpenSSLAdapter::SSLVerifyInternal(int previous_status, SSL* ssl, X509_STORE_CTX* store) { #if !defined(NDEBUG) - if (!status_on_failure) { + if (!previous_status) { char data[256]; X509* cert = X509_STORE_CTX_get_current_cert(store); int depth = X509_STORE_CTX_get_error_depth(store); @@ -887,8 +887,10 @@ int OpenSSLAdapter::SSLVerifyInternal(int status_on_failure, << X509_verify_cert_error_string(err); } #endif - if (ssl_cert_verifier_ == nullptr) { - return status_on_failure; + // `ssl_cert_verifier_` is used to override errors; if there is no error + // there is no reason to call it. + if (previous_status || ssl_cert_verifier_ == nullptr) { + return previous_status; } RTC_LOG(LS_INFO) << "Invoking SSL Verify Callback."; @@ -898,14 +900,14 @@ int OpenSSLAdapter::SSLVerifyInternal(int status_on_failure, int length = i2d_X509(X509_STORE_CTX_get_current_cert(store), &data); if (length < 0) { RTC_LOG(LS_ERROR) << "Failed to encode X509."; - return status_on_failure; + return previous_status; } bssl::UniquePtr owned_data(data); bssl::UniquePtr crypto_buffer( CRYPTO_BUFFER_new(data, length, openssl::GetBufferPool())); if (!crypto_buffer) { RTC_LOG(LS_ERROR) << "Failed to allocate CRYPTO_BUFFER."; - return status_on_failure; + return previous_status; } const BoringSSLCertificate cert(std::move(crypto_buffer)); #else @@ -913,7 +915,7 @@ int OpenSSLAdapter::SSLVerifyInternal(int status_on_failure, #endif if (!ssl_cert_verifier_->Verify(cert)) { RTC_LOG(LS_INFO) << "Failed to verify certificate using custom callback"; - return status_on_failure; + return previous_status; } custom_cert_verifier_status_ = true;