From 4060745995b8d054584969c7f55a6ef57da0a54d Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Tue, 26 Nov 2024 10:50:26 -0800 Subject: [PATCH] spanify SSLStreamAdapter::SetPeerCertificateDigest BUG=webrtc:357776213 Change-Id: Ie6189ac21b9f76f7ce5ddb3e4208c08793df73ff Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/368220 Reviewed-by: Harald Alvestrand Commit-Queue: Philipp Hancke Reviewed-by: Florent Castelli Cr-Commit-Position: refs/heads/main@{#43462} --- p2p/BUILD.gn | 6 +++ p2p/base/dtls_transport.cc | 31 +++++++----- rtc_base/openssl_stream_adapter.cc | 34 ++++--------- rtc_base/openssl_stream_adapter.h | 6 +-- rtc_base/ssl_stream_adapter.cc | 16 +++++- rtc_base/ssl_stream_adapter.h | 19 ++++--- rtc_base/ssl_stream_adapter_unittest.cc | 66 ++++++++++++++++--------- 7 files changed, 107 insertions(+), 71 deletions(-) diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index bddae54a82..f62331e3de 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -398,15 +398,20 @@ rtc_library("dtls_transport") { ":packet_transport_internal", "../api:array_view", "../api:dtls_transport_interface", + "../api:rtc_error", + "../api:scoped_refptr", "../api:sequence_checker", "../api/crypto:options", "../api/rtc_event_log", + "../api/units:timestamp", "../logging:ice_log", "../rtc_base:buffer", "../rtc_base:buffer_queue", "../rtc_base:checks", "../rtc_base:dscp", "../rtc_base:logging", + "../rtc_base:network_route", + "../rtc_base:socket", "../rtc_base:socket_address", "../rtc_base:ssl", "../rtc_base:ssl_adapter", @@ -414,6 +419,7 @@ rtc_library("dtls_transport") { "../rtc_base:stringutils", "../rtc_base:threading", "../rtc_base:timeutils", + "../rtc_base/network:ecn_marking", "../rtc_base/network:received_packet", "../rtc_base/system:no_unique_address", "//third_party/abseil-cpp/absl/memory", diff --git a/p2p/base/dtls_transport.cc b/p2p/base/dtls_transport.cc index cd44f45c51..832acaf2be 100644 --- a/p2p/base/dtls_transport.cc +++ b/p2p/base/dtls_transport.cc @@ -11,28 +11,38 @@ #include "p2p/base/dtls_transport.h" #include +#include #include #include +#include +#include #include -#include "absl/memory/memory.h" #include "absl/strings/string_view.h" #include "api/array_view.h" +#include "api/crypto/crypto_options.h" #include "api/dtls_transport_interface.h" +#include "api/rtc_error.h" #include "api/rtc_event_log/rtc_event_log.h" +#include "api/scoped_refptr.h" +#include "api/sequence_checker.h" +#include "api/units/timestamp.h" #include "logging/rtc_event_log/events/rtc_event_dtls_transport_state.h" #include "logging/rtc_event_log/events/rtc_event_dtls_writable_state.h" +#include "p2p/base/dtls_transport_internal.h" +#include "p2p/base/ice_transport_internal.h" #include "p2p/base/packet_transport_internal.h" #include "rtc_base/buffer.h" #include "rtc_base/checks.h" -#include "rtc_base/dscp.h" #include "rtc_base/logging.h" +#include "rtc_base/network/ecn_marking.h" #include "rtc_base/network/received_packet.h" +#include "rtc_base/network_route.h" #include "rtc_base/rtc_certificate.h" +#include "rtc_base/socket.h" #include "rtc_base/socket_address.h" #include "rtc_base/ssl_stream_adapter.h" #include "rtc_base/stream.h" -#include "rtc_base/thread.h" #include "rtc_base/time_utils.h" namespace cricket { @@ -312,11 +322,9 @@ bool DtlsTransport::SetRemoteFingerprint(absl::string_view digest_alg, // This can occur if DTLS is set up before a remote fingerprint is // received. For instance, if we set up DTLS due to receiving an early // ClientHello. - rtc::SSLPeerCertificateDigestError err; - if (!dtls_->SetPeerCertificateDigest( - remote_fingerprint_algorithm_, - reinterpret_cast(remote_fingerprint_value_.data()), - remote_fingerprint_value_.size(), &err)) { + rtc::SSLPeerCertificateDigestError err = dtls_->SetPeerCertificateDigest( + remote_fingerprint_algorithm_, remote_fingerprint_value_); + if (err != rtc::SSLPeerCertificateDigestError::NONE) { RTC_LOG(LS_ERROR) << ToString() << ": Couldn't set DTLS certificate digest."; set_dtls_state(webrtc::DtlsTransportState::kFailed); @@ -381,10 +389,9 @@ bool DtlsTransport::SetupDtls() { dtls_->SetEventCallback( [this](int events, int err) { OnDtlsEvent(events, err); }); if (remote_fingerprint_value_.size() && - !dtls_->SetPeerCertificateDigest( - remote_fingerprint_algorithm_, - reinterpret_cast(remote_fingerprint_value_.data()), - remote_fingerprint_value_.size())) { + dtls_->SetPeerCertificateDigest(remote_fingerprint_algorithm_, + remote_fingerprint_value_) != + rtc::SSLPeerCertificateDigestError::NONE) { RTC_LOG(LS_ERROR) << ToString() << ": Couldn't set DTLS certificate digest."; return false; diff --git a/rtc_base/openssl_stream_adapter.cc b/rtc_base/openssl_stream_adapter.cc index 0593f68f2e..88e09166b0 100644 --- a/rtc_base/openssl_stream_adapter.cc +++ b/rtc_base/openssl_stream_adapter.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -269,47 +270,33 @@ void OpenSSLStreamAdapter::SetServerRole(SSLRole role) { role_ = role; } -bool OpenSSLStreamAdapter::SetPeerCertificateDigest( +SSLPeerCertificateDigestError OpenSSLStreamAdapter::SetPeerCertificateDigest( absl::string_view digest_alg, - const unsigned char* digest_val, - size_t digest_len, - SSLPeerCertificateDigestError* error) { + rtc::ArrayView digest_val) { RTC_DCHECK(!peer_certificate_verified_); RTC_DCHECK(!HasPeerCertificateDigest()); size_t expected_len; - if (error) { - *error = SSLPeerCertificateDigestError::NONE; - } if (!OpenSSLDigest::GetDigestSize(digest_alg, &expected_len)) { RTC_LOG(LS_WARNING) << "Unknown digest algorithm: " << digest_alg; - if (error) { - *error = SSLPeerCertificateDigestError::UNKNOWN_ALGORITHM; - } - return false; + return SSLPeerCertificateDigestError::UNKNOWN_ALGORITHM; } - if (expected_len != digest_len) { - if (error) { - *error = SSLPeerCertificateDigestError::INVALID_LENGTH; - } - return false; + if (expected_len != digest_val.size()) { + return SSLPeerCertificateDigestError::INVALID_LENGTH; } - peer_certificate_digest_value_.SetData(digest_val, digest_len); + peer_certificate_digest_value_.SetData(digest_val); peer_certificate_digest_algorithm_ = std::string(digest_alg); if (!peer_cert_chain_) { // Normal case, where the digest is set before we obtain the certificate // from the handshake. - return true; + return SSLPeerCertificateDigestError::NONE; } if (!VerifyPeerCertificate()) { Error("SetPeerCertificateDigest", -1, SSL_AD_BAD_CERTIFICATE, false); - if (error) { - *error = SSLPeerCertificateDigestError::VERIFICATION_FAILED; - } - return false; + return SSLPeerCertificateDigestError::VERIFICATION_FAILED; } if (state_ == SSL_CONNECTED) { @@ -318,8 +305,7 @@ bool OpenSSLStreamAdapter::SetPeerCertificateDigest( // events and may not be prepared for reentrancy. PostEvent(SE_OPEN | SE_READ | SE_WRITE, 0); } - - return true; + return SSLPeerCertificateDigestError::NONE; } std::optional OpenSSLStreamAdapter::GetTlsCipherSuiteName() diff --git a/rtc_base/openssl_stream_adapter.h b/rtc_base/openssl_stream_adapter.h index 0cf79719cf..2ae7c0dd4e 100644 --- a/rtc_base/openssl_stream_adapter.h +++ b/rtc_base/openssl_stream_adapter.h @@ -78,11 +78,9 @@ class OpenSSLStreamAdapter final : public SSLStreamAdapter { // Default argument is for compatibility void SetServerRole(SSLRole role = SSL_SERVER) override; - bool SetPeerCertificateDigest( + SSLPeerCertificateDigestError SetPeerCertificateDigest( absl::string_view digest_alg, - const unsigned char* digest_val, - size_t digest_len, - SSLPeerCertificateDigestError* error = nullptr) override; + rtc::ArrayView digest_val) override; std::unique_ptr GetPeerSSLCertChain() const override; diff --git a/rtc_base/ssl_stream_adapter.cc b/rtc_base/ssl_stream_adapter.cc index ce26e4f201..dc72d46f12 100644 --- a/rtc_base/ssl_stream_adapter.cc +++ b/rtc_base/ssl_stream_adapter.cc @@ -15,10 +15,10 @@ #include #include #include -#include #include "absl/functional/any_invocable.h" #include "absl/strings/string_view.h" +#include "api/array_view.h" #include "rtc_base/openssl_stream_adapter.h" #include "rtc_base/ssl_identity.h" #include "rtc_base/stream.h" @@ -98,6 +98,20 @@ bool SSLStreamAdapter::IsAcceptableCipher(absl::string_view cipher, return OpenSSLStreamAdapter::IsAcceptableCipher(cipher, key_type); } +// Default shim for backward compat. +bool SSLStreamAdapter::SetPeerCertificateDigest( + absl::string_view digest_alg, + const unsigned char* digest_val, + size_t digest_len, + SSLPeerCertificateDigestError* error) { + unsigned char* nonconst_val = const_cast(digest_val); + SSLPeerCertificateDigestError ret = SetPeerCertificateDigest( + digest_alg, rtc::ArrayView(nonconst_val, digest_len)); + if (error) + *error = ret; + return ret == SSLPeerCertificateDigestError::NONE; +} + /////////////////////////////////////////////////////////////////////////////// // Test only settings /////////////////////////////////////////////////////////////////////////////// diff --git a/rtc_base/ssl_stream_adapter.h b/rtc_base/ssl_stream_adapter.h index ece5b44ac5..49094b8965 100644 --- a/rtc_base/ssl_stream_adapter.h +++ b/rtc_base/ssl_stream_adapter.h @@ -15,12 +15,14 @@ #include #include +#include #include #include #include "absl/functional/any_invocable.h" -#include "absl/memory/memory.h" #include "absl/strings/string_view.h" +#include "api/array_view.h" +#include "rtc_base/buffer.h" #include "rtc_base/ssl_certificate.h" #include "rtc_base/ssl_identity.h" #include "rtc_base/stream.h" @@ -170,13 +172,16 @@ class SSLStreamAdapter : public StreamInterface { // channel (such as the signaling channel). This must specify the terminal // certificate, not just a CA. SSLStream makes a copy of the digest value. // - // Returns true if successful. - // `error` is optional and provides more information about the failure. - virtual bool SetPeerCertificateDigest( + // Returns SSLPeerCertificateDigestError::NONE if successful. + virtual SSLPeerCertificateDigestError SetPeerCertificateDigest( absl::string_view digest_alg, - const unsigned char* digest_val, - size_t digest_len, - SSLPeerCertificateDigestError* error = nullptr) = 0; + rtc::ArrayView digest_val) = 0; + [[deprecated( + "Use SetPeerCertificateDigest with ArrayView instead")]] virtual bool + SetPeerCertificateDigest(absl::string_view digest_alg, + const unsigned char* digest_val, + size_t digest_len, + SSLPeerCertificateDigestError* error = nullptr); // Retrieves the peer's certificate chain including leaf certificate, if a // connection has been established. diff --git a/rtc_base/ssl_stream_adapter_unittest.cc b/rtc_base/ssl_stream_adapter_unittest.cc index 10299578c0..f474b9c074 100644 --- a/rtc_base/ssl_stream_adapter_unittest.cc +++ b/rtc_base/ssl_stream_adapter_unittest.cc @@ -10,7 +10,11 @@ #include "rtc_base/ssl_stream_adapter.h" +#ifdef OPENSSL_IS_BORINGSSL +#include +#else #include +#endif #include #include @@ -20,6 +24,7 @@ #include #include #include +#include #include #include @@ -29,6 +34,7 @@ #include "api/sequence_checker.h" #include "api/task_queue/pending_task_safety_flag.h" #include "api/units/time_delta.h" +#include "rtc_base/buffer.h" #include "rtc_base/buffer_queue.h" #include "rtc_base/callback_list.h" #include "rtc_base/checks.h" @@ -36,10 +42,7 @@ #include "rtc_base/fake_clock.h" #include "rtc_base/gunit.h" #include "rtc_base/logging.h" -#include "rtc_base/memory/fifo_buffer.h" -#include "rtc_base/memory_stream.h" #include "rtc_base/message_digest.h" -#include "rtc_base/openssl_stream_adapter.h" #include "rtc_base/ssl_identity.h" #include "rtc_base/stream.h" #include "rtc_base/third_party/sigslot/sigslot.h" @@ -508,9 +511,9 @@ class SSLStreamAdapterTestBase : public ::testing::Test, } void SetPeerIdentitiesByDigest(bool correct, bool expect_success) { - unsigned char server_digest[EVP_MAX_MD_SIZE]; + rtc::Buffer server_digest(0, EVP_MAX_MD_SIZE); size_t server_digest_len; - unsigned char client_digest[EVP_MAX_MD_SIZE]; + rtc::Buffer client_digest(0, EVP_MAX_MD_SIZE); size_t client_digest_len; bool rv; rtc::SSLPeerCertificateDigestError err; @@ -524,29 +527,31 @@ class SSLStreamAdapterTestBase : public ::testing::Test, RTC_DCHECK(client_identity()); rv = server_identity()->certificate().ComputeDigest( - digest_algorithm_, server_digest, digest_length_, &server_digest_len); + digest_algorithm_, server_digest.data(), digest_length_, + &server_digest_len); ASSERT_TRUE(rv); + server_digest.SetSize(server_digest_len); rv = client_identity()->certificate().ComputeDigest( - digest_algorithm_, client_digest, digest_length_, &client_digest_len); + digest_algorithm_, client_digest.data(), digest_length_, + &client_digest_len); ASSERT_TRUE(rv); + client_digest.SetSize(client_digest_len); if (!correct) { RTC_LOG(LS_INFO) << "Setting bogus digest for server cert"; server_digest[0]++; } - rv = client_ssl_->SetPeerCertificateDigest(digest_algorithm_, server_digest, - server_digest_len, &err); + err = + client_ssl_->SetPeerCertificateDigest(digest_algorithm_, server_digest); EXPECT_EQ(expected_err, err); - EXPECT_EQ(expect_success, rv); if (!correct) { RTC_LOG(LS_INFO) << "Setting bogus digest for client cert"; client_digest[0]++; } - rv = server_ssl_->SetPeerCertificateDigest(digest_algorithm_, client_digest, - client_digest_len, &err); + err = + server_ssl_->SetPeerCertificateDigest(digest_algorithm_, client_digest); EXPECT_EQ(expected_err, err); - EXPECT_EQ(expect_success, rv); identities_set_ = true; } @@ -668,21 +673,25 @@ class SSLStreamAdapterTestBase : public ::testing::Test, // Collect both of the certificate digests; needs to be done before calling // SetPeerCertificateDigest as that may reset the identity. - unsigned char server_digest[EVP_MAX_MD_SIZE]; + rtc::Buffer server_digest(0, EVP_MAX_MD_SIZE); size_t server_digest_len; - unsigned char client_digest[EVP_MAX_MD_SIZE]; + rtc::Buffer client_digest(0, EVP_MAX_MD_SIZE); size_t client_digest_len; bool rv; ASSERT_THAT(server_identity(), NotNull()); rv = server_identity()->certificate().ComputeDigest( - digest_algorithm_, server_digest, digest_length_, &server_digest_len); + digest_algorithm_, server_digest.data(), digest_length_, + &server_digest_len); ASSERT_TRUE(rv); + server_digest.SetSize(server_digest_len); ASSERT_THAT(client_identity(), NotNull()); rv = client_identity()->certificate().ComputeDigest( - digest_algorithm_, client_digest, digest_length_, &client_digest_len); + digest_algorithm_, client_digest.data(), digest_length_, + &client_digest_len); ASSERT_TRUE(rv); + client_digest.SetSize(client_digest_len); if (!valid_identity) { RTC_LOG(LS_INFO) << "Setting bogus digest for client/server certs"; @@ -696,10 +705,9 @@ class SSLStreamAdapterTestBase : public ::testing::Test, valid_identity ? rtc::SSLPeerCertificateDigestError::NONE : rtc::SSLPeerCertificateDigestError::VERIFICATION_FAILED; - rv = client_ssl_->SetPeerCertificateDigest(digest_algorithm_, server_digest, - server_digest_len, &err); + err = + client_ssl_->SetPeerCertificateDigest(digest_algorithm_, server_digest); EXPECT_EQ(expected_err, err); - EXPECT_EQ(valid_identity, rv); // State should then transition to SS_OPEN or SS_CLOSED based on validation // of the identity. if (valid_identity) { @@ -715,10 +723,9 @@ class SSLStreamAdapterTestBase : public ::testing::Test, } // Set the peer certificate digest for the server. - rv = server_ssl_->SetPeerCertificateDigest(digest_algorithm_, client_digest, - client_digest_len, &err); + err = + server_ssl_->SetPeerCertificateDigest(digest_algorithm_, client_digest); EXPECT_EQ(expected_err, err); - EXPECT_EQ(valid_identity, rv); if (valid_identity) { EXPECT_EQ(rtc::SS_OPEN, server_ssl_->GetState()); } else { @@ -1466,6 +1473,19 @@ TEST_F(SSLStreamAdapterTestDTLSFromPEMStrings, TestDTLSGetPeerCertificate) { ASSERT_EQ(kCERT_PEM, server_peer_cert->ToPEMString()); } +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdeprecated-declarations" +TEST_F(SSLStreamAdapterTestDTLSFromPEMStrings, + DeprecatedSetPeerCertificateDigest) { + rtc::SSLPeerCertificateDigestError error; + // Pass in a wrong length to trigger an error. + bool ret = client_ssl_->SetPeerCertificateDigest(rtc::DIGEST_SHA_256, {}, + /*length=*/0, &error); + EXPECT_FALSE(ret); + EXPECT_EQ(error, rtc::SSLPeerCertificateDigestError::INVALID_LENGTH); +} +#pragma clang diagnostic pop + // Test getting the DTLS 1.2 version. TEST_F(SSLStreamAdapterTestDTLS, TestGetSslVersionBytes) { // https://datatracker.ietf.org/doc/html/rfc9147#section-5.3