From 4905edbd03ec2b6ec927a495301224f379c5f6fa Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Mon, 15 Oct 2018 19:27:44 -0700 Subject: [PATCH] Reland: Use unique_ptr and ArrayView in SSLFingerprint Bug: webrtc:9860 Change-Id: Ia6a0e82d6eff384fe3f618c77e8c78e45569eb97 Tbr: Benjamin Wright Tbr: Qingsi Wang Reviewed-on: https://webrtc-review.googlesource.com/c/106180 Reviewed-by: Steve Anton Commit-Queue: Steve Anton Cr-Commit-Position: refs/heads/master@{#25219} --- p2p/base/dtlstransport_unittest.cc | 5 +-- p2p/base/fakedtlstransport.h | 7 ++-- p2p/base/transportdescriptionfactory.cc | 4 +- pc/jseptransport.cc | 8 ++-- pc/jseptransport_unittest.cc | 7 ++-- pc/jseptransportcontroller_unittest.cc | 2 +- pc/peerconnection_crypto_unittest.cc | 4 +- pc/rtcstatscollector_unittest.cc | 2 +- pc/webrtcsdp.cc | 27 ++++++------ pc/webrtcsdp_unittest.cc | 3 +- rtc_base/sslcertificate.cc | 4 +- rtc_base/sslfingerprint.cc | 56 +++++++++++++++---------- rtc_base/sslfingerprint.h | 21 ++++++++-- rtc_base/sslidentity_unittest.cc | 2 +- 14 files changed, 92 insertions(+), 60 deletions(-) diff --git a/p2p/base/dtlstransport_unittest.cc b/p2p/base/dtlstransport_unittest.cc index 8dfbf6106c..2ccb64cbaf 100644 --- a/p2p/base/dtlstransport_unittest.cc +++ b/p2p/base/dtlstransport_unittest.cc @@ -47,8 +47,8 @@ void SetRemoteFingerprintFromCert( DtlsTransport* transport, const rtc::scoped_refptr& cert, bool modify_digest = false) { - rtc::SSLFingerprint* fingerprint = - rtc::SSLFingerprint::CreateFromCertificate(cert); + std::unique_ptr fingerprint = + rtc::SSLFingerprint::CreateFromCertificate(*cert); if (modify_digest) { ++fingerprint->digest[0]; } @@ -57,7 +57,6 @@ void SetRemoteFingerprintFromCert( fingerprint->algorithm, reinterpret_cast(fingerprint->digest.data()), fingerprint->digest.size())); - delete fingerprint; } class DtlsTestClient : public sigslot::has_slots<> { diff --git a/p2p/base/fakedtlstransport.h b/p2p/base/fakedtlstransport.h index c4f9d2c239..8b0619aa08 100644 --- a/p2p/base/fakedtlstransport.h +++ b/p2p/base/fakedtlstransport.h @@ -33,7 +33,7 @@ class FakeDtlsTransport : public DtlsTransportInternal { : ice_transport_(ice_transport), transport_name_(ice_transport->transport_name()), component_(ice_transport->component()), - dtls_fingerprint_("", nullptr, 0) { + dtls_fingerprint_("", nullptr) { RTC_DCHECK(ice_transport_); ice_transport_->SignalReadPacket.connect( this, &FakeDtlsTransport::OnIceTransportReadPacket); @@ -45,7 +45,7 @@ class FakeDtlsTransport : public DtlsTransportInternal { : owned_ice_transport_(std::move(ice)), transport_name_(owned_ice_transport_->transport_name()), component_(owned_ice_transport_->component()), - dtls_fingerprint_("", nullptr, 0) { + dtls_fingerprint_("", rtc::ArrayView()) { ice_transport_ = owned_ice_transport_.get(); ice_transport_->SignalReadPacket.connect( this, &FakeDtlsTransport::OnIceTransportReadPacket); @@ -133,7 +133,8 @@ class FakeDtlsTransport : public DtlsTransportInternal { bool SetRemoteFingerprint(const std::string& alg, const uint8_t* digest, size_t digest_len) override { - dtls_fingerprint_ = rtc::SSLFingerprint(alg, digest, digest_len); + dtls_fingerprint_ = + rtc::SSLFingerprint(alg, rtc::MakeArrayView(digest, digest_len)); return true; } bool SetSslMaxProtocolVersion(rtc::SSLProtocolVersion version) override { diff --git a/p2p/base/transportdescriptionfactory.cc b/p2p/base/transportdescriptionfactory.cc index 670950d5ec..689cd4f7a2 100644 --- a/p2p/base/transportdescriptionfactory.cc +++ b/p2p/base/transportdescriptionfactory.cc @@ -120,8 +120,8 @@ bool TransportDescriptionFactory::SetSecurityInfo(TransportDescription* desc, // This digest algorithm is used to produce the a=fingerprint lines in SDP. // RFC 4572 Section 5 requires that those lines use the same hash function as // the certificate's signature, which is what CreateFromCertificate does. - desc->identity_fingerprint.reset( - rtc::SSLFingerprint::CreateFromCertificate(certificate_)); + desc->identity_fingerprint = + rtc::SSLFingerprint::CreateFromCertificate(*certificate_); if (!desc->identity_fingerprint) { return false; } diff --git a/pc/jseptransport.cc b/pc/jseptransport.cc index 894dce122d..368b32f6f7 100644 --- a/pc/jseptransport.cc +++ b/pc/jseptransport.cc @@ -316,8 +316,9 @@ webrtc::RTCError JsepTransport::VerifyCertificateFingerprint( return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, "Fingerprint provided but no identity available."); } - std::unique_ptr fp_tmp(rtc::SSLFingerprint::Create( - fingerprint->algorithm, certificate->identity())); + std::unique_ptr fp_tmp = + rtc::SSLFingerprint::CreateUnique(fingerprint->algorithm, + *certificate->identity()); RTC_DCHECK(fp_tmp.get() != NULL); if (*fp_tmp == *fingerprint) { return webrtc::RTCError::OK(); @@ -506,7 +507,8 @@ webrtc::RTCError JsepTransport::NegotiateAndSetDtlsParameters( "Local fingerprint supplied when caller didn't offer DTLS."); } else { // We are not doing DTLS - remote_fingerprint = absl::make_unique("", nullptr, 0); + remote_fingerprint = absl::make_unique( + "", rtc::ArrayView()); } // Now that we have negotiated everything, push it downward. // Note that we cache the result so that if we have race conditions diff --git a/pc/jseptransport_unittest.cc b/pc/jseptransport_unittest.cc index 1b42578c73..748a4e078f 100644 --- a/pc/jseptransport_unittest.cc +++ b/pc/jseptransport_unittest.cc @@ -126,7 +126,7 @@ class JsepTransport2Test : public testing::Test, public sigslot::has_slots<> { std::unique_ptr fingerprint; if (cert) { - fingerprint.reset(rtc::SSLFingerprint::CreateFromCertificate(cert)); + fingerprint = rtc::SSLFingerprint::CreateFromCertificate(*cert); } jsep_description.transport_desc = TransportDescription(std::vector(), ufrag, pwd, @@ -386,8 +386,9 @@ TEST_P(JsepTransport2WithRtcpMux, VerifyCertificateFingerprint) { ASSERT_TRUE(certificate->ssl_certificate().GetSignatureDigestAlgorithm( &digest_algorithm)); ASSERT_FALSE(digest_algorithm.empty()); - std::unique_ptr good_fingerprint( - rtc::SSLFingerprint::Create(digest_algorithm, certificate->identity())); + std::unique_ptr good_fingerprint = + rtc::SSLFingerprint::CreateUnique(digest_algorithm, + *certificate->identity()); ASSERT_NE(nullptr, good_fingerprint); EXPECT_TRUE(jsep_transport_ diff --git a/pc/jseptransportcontroller_unittest.cc b/pc/jseptransportcontroller_unittest.cc index 26813e1c4f..d36b8f3ce1 100644 --- a/pc/jseptransportcontroller_unittest.cc +++ b/pc/jseptransportcontroller_unittest.cc @@ -170,7 +170,7 @@ class JsepTransportControllerTest : public JsepTransportController::Observer, rtc::scoped_refptr cert) { std::unique_ptr fingerprint; if (cert) { - fingerprint.reset(rtc::SSLFingerprint::CreateFromCertificate(cert)); + fingerprint = rtc::SSLFingerprint::CreateFromCertificate(*cert); } cricket::TransportDescription transport_desc(std::vector(), diff --git a/pc/peerconnection_crypto_unittest.cc b/pc/peerconnection_crypto_unittest.cc index 71e87636e2..1a53d860bf 100644 --- a/pc/peerconnection_crypto_unittest.cc +++ b/pc/peerconnection_crypto_unittest.cc @@ -703,8 +703,8 @@ TEST_P(PeerConnectionCryptoTest, SessionErrorIfFingerprintInvalid) { invalid_answer->description()->GetTransportInfoByName( audio_content->name); ASSERT_TRUE(audio_transport_info); - audio_transport_info->description.identity_fingerprint.reset( - rtc::SSLFingerprint::CreateFromCertificate(other_certificate)); + audio_transport_info->description.identity_fingerprint = + rtc::SSLFingerprint::CreateFromCertificate(*other_certificate); // Set the invalid answer and expect a fingerprint error. std::string error; diff --git a/pc/rtcstatscollector_unittest.cc b/pc/rtcstatscollector_unittest.cc index 7404d492ef..96ccfdf3ad 100644 --- a/pc/rtcstatscollector_unittest.cc +++ b/pc/rtcstatscollector_unittest.cc @@ -146,7 +146,7 @@ std::unique_ptr CreateFakeCertificateAndInfoFromDers( const rtc::SSLCertChain& chain = info->certificate->ssl_cert_chain(); std::unique_ptr fp; for (size_t i = 0; i < chain.GetSize(); i++) { - fp.reset(rtc::SSLFingerprint::Create("sha-1", &chain.Get(i))); + fp = rtc::SSLFingerprint::Create("sha-1", chain.Get(i)); EXPECT_TRUE(fp); info->fingerprints.push_back(fp->GetRfc4572Fingerprint()); } diff --git a/pc/webrtcsdp.cc b/pc/webrtcsdp.cc index 9995d5bb94..cf6b10dcd7 100644 --- a/pc/webrtcsdp.cc +++ b/pc/webrtcsdp.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include "api/mediatypes.h" @@ -335,9 +336,10 @@ static bool ParseIceOptions(const std::string& line, static bool ParseExtmap(const std::string& line, RtpExtension* extmap, SdpParseError* error); -static bool ParseFingerprintAttribute(const std::string& line, - rtc::SSLFingerprint** fingerprint, - SdpParseError* error); +static bool ParseFingerprintAttribute( + const std::string& line, + std::unique_ptr* fingerprint, + SdpParseError* error); static bool ParseDtlsSetup(const std::string& line, cricket::ConnectionRole* role, SdpParseError* error); @@ -2134,11 +2136,11 @@ bool ParseSessionDescription(const std::string& message, "Can't have multiple fingerprint attributes at the same level.", error); } - rtc::SSLFingerprint* fingerprint = NULL; + std::unique_ptr fingerprint; if (!ParseFingerprintAttribute(line, &fingerprint, error)) { return false; } - session_td->identity_fingerprint.reset(fingerprint); + session_td->identity_fingerprint = std::move(fingerprint); } else if (HasAttribute(line, kAttributeSetup)) { if (!ParseDtlsSetup(line, &(session_td->connection_role), error)) { return false; @@ -2185,9 +2187,10 @@ bool ParseGroupAttribute(const std::string& line, return true; } -static bool ParseFingerprintAttribute(const std::string& line, - rtc::SSLFingerprint** fingerprint, - SdpParseError* error) { +static bool ParseFingerprintAttribute( + const std::string& line, + std::unique_ptr* fingerprint, + SdpParseError* error) { if (!IsLineType(line, kLineTypeAttributes) || !HasAttribute(line, kAttributeFingerprint)) { return ParseFailedExpectLine(line, 0, kLineTypeAttributes, @@ -2213,7 +2216,8 @@ static bool ParseFingerprintAttribute(const std::string& line, ::tolower); // The second field is the digest value. De-hexify it. - *fingerprint = rtc::SSLFingerprint::CreateFromRfc4572(algorithm, fields[1]); + *fingerprint = + rtc::SSLFingerprint::CreateUniqueFromRfc4572(algorithm, fields[1]); if (!*fingerprint) { return ParseFailed(line, "Failed to create fingerprint from the digest.", error); @@ -2853,12 +2857,11 @@ bool ParseContent(const std::string& message, return false; } } else if (HasAttribute(line, kAttributeFingerprint)) { - rtc::SSLFingerprint* fingerprint = NULL; - + std::unique_ptr fingerprint; if (!ParseFingerprintAttribute(line, &fingerprint, error)) { return false; } - transport->identity_fingerprint.reset(fingerprint); + transport->identity_fingerprint = std::move(fingerprint); } else if (HasAttribute(line, kAttributeSetup)) { if (!ParseDtlsSetup(line, &(transport->connection_role), error)) { return false; diff --git a/pc/webrtcsdp_unittest.cc b/pc/webrtcsdp_unittest.cc index 59897af040..e3ccd06b89 100644 --- a/pc/webrtcsdp_unittest.cc +++ b/pc/webrtcsdp_unittest.cc @@ -1503,8 +1503,7 @@ class WebRtcSdpTest : public testing::Test { void AddFingerprint() { desc_.RemoveTransportInfoByName(kAudioContentName); desc_.RemoveTransportInfoByName(kVideoContentName); - rtc::SSLFingerprint fingerprint(rtc::DIGEST_SHA_1, kIdentityDigest, - sizeof(kIdentityDigest)); + rtc::SSLFingerprint fingerprint(rtc::DIGEST_SHA_1, kIdentityDigest); EXPECT_TRUE(desc_.AddTransportInfo(TransportInfo( kAudioContentName, TransportDescription(std::vector(), kUfragVoice, kPwdVoice, diff --git a/rtc_base/sslcertificate.cc b/rtc_base/sslcertificate.cc index e40feec219..53af0f5854 100644 --- a/rtc_base/sslcertificate.cc +++ b/rtc_base/sslcertificate.cc @@ -54,8 +54,8 @@ std::unique_ptr SSLCertificate::GetStats() const { // |SSLCertificate::GetSignatureDigestAlgorithm| is not supported by the // implementation of |SSLCertificate::ComputeDigest|. This currently happens // with MD5- and SHA-224-signed certificates when linked to libNSS. - std::unique_ptr ssl_fingerprint( - SSLFingerprint::Create(digest_algorithm, this)); + std::unique_ptr ssl_fingerprint = + SSLFingerprint::Create(digest_algorithm, *this); if (!ssl_fingerprint) return nullptr; std::string fingerprint = ssl_fingerprint->GetRfc4572Fingerprint(); diff --git a/rtc_base/sslfingerprint.cc b/rtc_base/sslfingerprint.cc index 4f1ae8f7a5..ef466545af 100644 --- a/rtc_base/sslfingerprint.cc +++ b/rtc_base/sslfingerprint.cc @@ -21,56 +21,66 @@ namespace rtc { SSLFingerprint* SSLFingerprint::Create(const std::string& algorithm, const rtc::SSLIdentity* identity) { - if (!identity) { - return nullptr; - } - - return Create(algorithm, &(identity->certificate())); + return CreateUnique(algorithm, *identity).release(); } -SSLFingerprint* SSLFingerprint::Create(const std::string& algorithm, - const rtc::SSLCertificate* cert) { +std::unique_ptr SSLFingerprint::CreateUnique( + const std::string& algorithm, + const rtc::SSLIdentity& identity) { + return Create(algorithm, identity.certificate()); +} + +std::unique_ptr SSLFingerprint::Create( + const std::string& algorithm, + const rtc::SSLCertificate& cert) { uint8_t digest_val[64]; size_t digest_len; - bool ret = cert->ComputeDigest(algorithm, digest_val, sizeof(digest_val), - &digest_len); + bool ret = cert.ComputeDigest(algorithm, digest_val, sizeof(digest_val), + &digest_len); if (!ret) { return nullptr; } - - return new SSLFingerprint(algorithm, digest_val, digest_len); + return absl::make_unique( + algorithm, ArrayView(digest_val, digest_len)); } SSLFingerprint* SSLFingerprint::CreateFromRfc4572( const std::string& algorithm, const std::string& fingerprint) { + return CreateUniqueFromRfc4572(algorithm, fingerprint).release(); +} + +std::unique_ptr SSLFingerprint::CreateUniqueFromRfc4572( + const std::string& algorithm, + const std::string& fingerprint) { if (algorithm.empty() || !rtc::IsFips180DigestAlgorithm(algorithm)) return nullptr; if (fingerprint.empty()) return nullptr; - size_t value_len; char value[rtc::MessageDigest::kMaxSize]; - value_len = rtc::hex_decode_with_delimiter( + size_t value_len = rtc::hex_decode_with_delimiter( value, sizeof(value), fingerprint.c_str(), fingerprint.length(), ':'); if (!value_len) return nullptr; - return new SSLFingerprint(algorithm, reinterpret_cast(value), - value_len); + return absl::make_unique( + algorithm, + ArrayView(reinterpret_cast(value), value_len)); } -SSLFingerprint* SSLFingerprint::CreateFromCertificate( - const RTCCertificate* cert) { +std::unique_ptr SSLFingerprint::CreateFromCertificate( + const RTCCertificate& cert) { std::string digest_alg; - if (!cert->ssl_certificate().GetSignatureDigestAlgorithm(&digest_alg)) { + if (!cert.ssl_certificate().GetSignatureDigestAlgorithm(&digest_alg)) { RTC_LOG(LS_ERROR) << "Failed to retrieve the certificate's digest algorithm"; return nullptr; } - SSLFingerprint* fingerprint = Create(digest_alg, cert->identity()); + std::unique_ptr fingerprint = + CreateUnique(digest_alg, *cert.identity()); if (!fingerprint) { RTC_LOG(LS_ERROR) << "Failed to create identity fingerprint, alg=" << digest_alg; @@ -78,12 +88,14 @@ SSLFingerprint* SSLFingerprint::CreateFromCertificate( return fingerprint; } +SSLFingerprint::SSLFingerprint(const std::string& algorithm, + ArrayView digest_view) + : algorithm(algorithm), digest(digest_view.data(), digest_view.size()) {} + SSLFingerprint::SSLFingerprint(const std::string& algorithm, const uint8_t* digest_in, size_t digest_len) - : algorithm(algorithm) { - digest.SetData(digest_in, digest_len); -} + : SSLFingerprint(algorithm, MakeArrayView(digest_in, digest_len)) {} SSLFingerprint::SSLFingerprint(const SSLFingerprint& from) : algorithm(from.algorithm), digest(from.digest) {} diff --git a/rtc_base/sslfingerprint.h b/rtc_base/sslfingerprint.h index b204bc77f3..b8109e88f3 100644 --- a/rtc_base/sslfingerprint.h +++ b/rtc_base/sslfingerprint.h @@ -22,19 +22,34 @@ namespace rtc { class SSLCertificate; struct SSLFingerprint { + // TODO(steveanton): Remove once downstream projects have moved off of this. static SSLFingerprint* Create(const std::string& algorithm, const rtc::SSLIdentity* identity); + // TODO(steveanton): Rename to Create once projects have migrated. + static std::unique_ptr CreateUnique( + const std::string& algorithm, + const rtc::SSLIdentity& identity); - static SSLFingerprint* Create(const std::string& algorithm, - const rtc::SSLCertificate* cert); + static std::unique_ptr Create( + const std::string& algorithm, + const rtc::SSLCertificate& cert); + // TODO(steveanton): Remove once downstream projects have moved off of this. static SSLFingerprint* CreateFromRfc4572(const std::string& algorithm, const std::string& fingerprint); + // TODO(steveanton): Rename to CreateFromRfc4572 once projects have migrated. + static std::unique_ptr CreateUniqueFromRfc4572( + const std::string& algorithm, + const std::string& fingerprint); // Creates a fingerprint from a certificate, using the same digest algorithm // as the certificate's signature. - static SSLFingerprint* CreateFromCertificate(const RTCCertificate* cert); + static std::unique_ptr CreateFromCertificate( + const RTCCertificate& cert); + SSLFingerprint(const std::string& algorithm, + ArrayView digest_view); + // TODO(steveanton): Remove once downstream projects have moved off of this. SSLFingerprint(const std::string& algorithm, const uint8_t* digest_in, size_t digest_len); diff --git a/rtc_base/sslidentity_unittest.cc b/rtc_base/sslidentity_unittest.cc index 68b582839f..81831848a9 100644 --- a/rtc_base/sslidentity_unittest.cc +++ b/rtc_base/sslidentity_unittest.cc @@ -180,7 +180,7 @@ IdentityAndInfo CreateFakeIdentityAndInfoFromDers( const rtc::SSLCertChain& chain = info.identity->cert_chain(); std::unique_ptr fp; for (size_t i = 0; i < chain.GetSize(); i++) { - fp.reset(rtc::SSLFingerprint::Create("sha-1", &chain.Get(i))); + fp = rtc::SSLFingerprint::Create("sha-1", chain.Get(i)); EXPECT_TRUE(fp); info.fingerprints.push_back(fp->GetRfc4572Fingerprint()); }