commit bd917a13fb4704cfc7d3f8c6879b877d7e839dc2
author Taylor Brandstetter <deadbeef@webrtc.org> Tue Sep 14 22:22:41 2021
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Sep 15 00:04:27 2021
tree cc84d79148c0bd8b333ab2e5d2bf689a6baed369
parent bc503c95b5e5262576fed85c6986cad303a02287

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 <deadbeef@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#34994}

rtc_base/openssl_adapter.cc

diff --git a/rtc_base/openssl_adapter.cc b/rtc_base/openssl_adapter.cc
index 93e5780..7489bc9 100644
--- a/rtc_base/openssl_adapter.cc
+++ b/rtc_base/openssl_adapter.cc
@@ -868,11 +868,11 @@
   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 @@
                       << 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 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<uint8_t> owned_data(data);
   bssl::UniquePtr<CRYPTO_BUFFER> 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 @@
 #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;