From 8662f940230bb0f8989ddc993488cde3a9c5b76a Mon Sep 17 00:00:00 2001 From: deadbeef Date: Fri, 20 Jan 2017 21:20:51 -0800 Subject: [PATCH] Only set certificate on DTLS transport if fingerprint is found in SDP. This is used for fallback from DTLS to SDES encryption, which we probably still want to support. Setting a certificate puts the DTLS transport in a "DTLS enabled" mode, so it should be delayed until SDP with "a=fingerprint" is set. BUG=webrtc:6972 Review-Url: https://codereview.webrtc.org/2641633002 Cr-Commit-Position: refs/heads/master@{#16199} --- .../api/peerconnectioninterface_unittest.cc | 49 ++++++++++++++++++- webrtc/api/test/fakertccertificategenerator.h | 4 ++ webrtc/base/sslfingerprint.cc | 17 +++++++ webrtc/base/sslfingerprint.h | 5 ++ webrtc/p2p/base/faketransportcontroller.h | 12 ++++- webrtc/p2p/base/jseptransport.cc | 7 ++- webrtc/p2p/base/transportcontroller.cc | 15 ++---- .../p2p/base/transportdescriptionfactory.cc | 7 ++- 8 files changed, 100 insertions(+), 16 deletions(-) diff --git a/webrtc/api/peerconnectioninterface_unittest.cc b/webrtc/api/peerconnectioninterface_unittest.cc index 2da3755fcb..be3825ce0f 100644 --- a/webrtc/api/peerconnectioninterface_unittest.cc +++ b/webrtc/api/peerconnectioninterface_unittest.cc @@ -293,6 +293,31 @@ static const char kSdpStringMs1Video1[] = "a=ssrc:4 cname:stream1\r\n" "a=ssrc:4 msid:stream1 videotrack1\r\n"; +static const char kDtlsSdesFallbackSdp[] = + "v=0\r\n" + "o=xxxxxx 7 2 IN IP4 0.0.0.0\r\n" + "s=-\r\n" + "c=IN IP4 0.0.0.0\r\n" + "t=0 0\r\n" + "a=group:BUNDLE audio\r\n" + "a=msid-semantic: WMS\r\n" + "m=audio 1 RTP/SAVPF 0\r\n" + "a=sendrecv\r\n" + "a=rtcp-mux\r\n" + "a=mid:audio\r\n" + "a=ssrc:1 cname:stream1\r\n" + "a=ssrc:1 mslabel:stream1\r\n" + "a=ssrc:1 label:audiotrack0\r\n" + "a=ice-ufrag:e5785931\r\n" + "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" + "a=rtpmap:0 pcmu/8000\r\n" + "a=fingerprint:sha-1 " + "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n" + "a=setup:actpass\r\n" + "a=crypto:1 AES_CM_128_HMAC_SHA1_32 " + "inline:NzB4d1BINUAvLEw6UzF3WSJ+PSdFcGdUJShpX1Zj|2^20|1:32 " + "dummy_session_params\r\n"; + #define MAYBE_SKIP_TEST(feature) \ if (!(feature())) { \ LOG(LS_INFO) << "Feature disabled... skipping"; \ @@ -741,7 +766,8 @@ class PeerConnectionInterfaceTest : public testing::Test { webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, &dtls, nullptr) && dtls) { - cert_generator.reset(new FakeRTCCertificateGenerator()); + fake_certificate_generator_ = new FakeRTCCertificateGenerator(); + cert_generator.reset(fake_certificate_generator_); } pc_ = pc_factory_->CreatePeerConnection( config, constraints, std::move(port_allocator), @@ -1135,6 +1161,7 @@ class PeerConnectionInterfaceTest : public testing::Test { } cricket::FakePortAllocator* port_allocator_ = nullptr; + FakeRTCCertificateGenerator* fake_certificate_generator_ = nullptr; rtc::scoped_refptr pc_factory_; rtc::scoped_refptr pc_factory_for_test_; rtc::scoped_refptr pc_; @@ -2073,6 +2100,26 @@ TEST_F(PeerConnectionInterfaceTest, ReceiveFireFoxOffer) { #endif } +// Test that an offer can be received which offers DTLS with SDES fallback. +// Regression test for issue: +// https://bugs.chromium.org/p/webrtc/issues/detail?id=6972 +TEST_F(PeerConnectionInterfaceTest, ReceiveDtlsSdesFallbackOffer) { + FakeConstraints constraints; + constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, + true); + CreatePeerConnection(&constraints); + // Wait for fake certificate to be generated. Previously, this is what caused + // the "a=crypto" lines to be rejected. + AddAudioVideoStream(kStreamLabel1, "audio_label", "video_label"); + ASSERT_NE(nullptr, fake_certificate_generator_); + EXPECT_EQ_WAIT(1, fake_certificate_generator_->generated_certificates(), + kTimeout); + SessionDescriptionInterface* desc = webrtc::CreateSessionDescription( + SessionDescriptionInterface::kOffer, kDtlsSdesFallbackSdp, nullptr); + EXPECT_TRUE(DoSetSessionDescription(desc, false)); + CreateAnswerAsLocalDescription(); +} + // Test that we can create an audio only offer and receive an answer with a // limited set of audio codecs and receive an updated offer with more audio // codecs, where the added codecs are not supported. diff --git a/webrtc/api/test/fakertccertificategenerator.h b/webrtc/api/test/fakertccertificategenerator.h index 34fc1c53a9..cd1e4bf5b8 100644 --- a/webrtc/api/test/fakertccertificategenerator.h +++ b/webrtc/api/test/fakertccertificategenerator.h @@ -134,6 +134,8 @@ class FakeRTCCertificateGenerator void use_original_key() { key_index_ = 0; } void use_alternate_key() { key_index_ = 1; } + int generated_certificates() { return generated_certificates_; } + void GenerateCertificateAsync( const rtc::KeyParams& key_params, const rtc::Optional& expires_ms, @@ -210,6 +212,7 @@ class FakeRTCCertificateGenerator msg->message_id == MSG_SUCCESS_RSA ? rtc::KT_RSA : rtc::KT_ECDSA; certificate = rtc::RTCCertificate::FromPEM(get_pem(key_type)); RTC_DCHECK(certificate); + ++generated_certificates_; callback->OnSuccess(certificate); break; } @@ -222,6 +225,7 @@ class FakeRTCCertificateGenerator bool should_fail_; int key_index_ = 0; + int generated_certificates_ = 0; }; #endif // WEBRTC_API_TEST_FAKERTCCERTIFICATEGENERATOR_H_ diff --git a/webrtc/base/sslfingerprint.cc b/webrtc/base/sslfingerprint.cc index 2c3e1e974b..e172a2c304 100644 --- a/webrtc/base/sslfingerprint.cc +++ b/webrtc/base/sslfingerprint.cc @@ -14,6 +14,7 @@ #include #include "webrtc/base/helpers.h" +#include "webrtc/base/logging.h" #include "webrtc/base/messagedigest.h" #include "webrtc/base/stringencode.h" @@ -62,6 +63,22 @@ SSLFingerprint* SSLFingerprint::CreateFromRfc4572( value_len); } +SSLFingerprint* SSLFingerprint::CreateFromCertificate( + const RTCCertificate* cert) { + std::string digest_alg; + if (!cert->ssl_certificate().GetSignatureDigestAlgorithm(&digest_alg)) { + LOG(LS_ERROR) << "Failed to retrieve the certificate's digest algorithm"; + return nullptr; + } + + SSLFingerprint* fingerprint = Create(digest_alg, cert->identity()); + if (!fingerprint) { + LOG(LS_ERROR) << "Failed to create identity fingerprint, alg=" + << digest_alg; + } + return fingerprint; +} + SSLFingerprint::SSLFingerprint(const std::string& algorithm, const uint8_t* digest_in, size_t digest_len) diff --git a/webrtc/base/sslfingerprint.h b/webrtc/base/sslfingerprint.h index 4ffb2b0524..62b4bc812f 100644 --- a/webrtc/base/sslfingerprint.h +++ b/webrtc/base/sslfingerprint.h @@ -15,6 +15,7 @@ #include "webrtc/base/basictypes.h" #include "webrtc/base/copyonwritebuffer.h" +#include "webrtc/base/rtccertificate.h" #include "webrtc/base/sslidentity.h" namespace rtc { @@ -31,6 +32,10 @@ struct SSLFingerprint { static SSLFingerprint* CreateFromRfc4572(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); + SSLFingerprint(const std::string& algorithm, const uint8_t* digest_in, size_t digest_len); diff --git a/webrtc/p2p/base/faketransportcontroller.h b/webrtc/p2p/base/faketransportcontroller.h index fd476829c4..d97b4b6188 100644 --- a/webrtc/p2p/base/faketransportcontroller.h +++ b/webrtc/p2p/base/faketransportcontroller.h @@ -617,12 +617,20 @@ class FakeTransportController : public TransportController { std::vector(), rtc::CreateRandomString(cricket::ICE_UFRAG_LENGTH), rtc::CreateRandomString(cricket::ICE_PWD_LENGTH), - cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_NONE, nullptr); + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_NONE, + certificate_for_testing() + ? rtc::SSLFingerprint::CreateFromCertificate( + certificate_for_testing()) + : nullptr); TransportDescription remote_desc( std::vector(), rtc::CreateRandomString(cricket::ICE_UFRAG_LENGTH), rtc::CreateRandomString(cricket::ICE_PWD_LENGTH), - cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_NONE, nullptr); + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_NONE, + dest->certificate_for_testing() + ? rtc::SSLFingerprint::CreateFromCertificate( + dest->certificate_for_testing()) + : nullptr); std::string err; SetLocalTransportDescription(transport_name, local_desc, cricket::CA_OFFER, &err); diff --git a/webrtc/p2p/base/jseptransport.cc b/webrtc/p2p/base/jseptransport.cc index 159c238a79..9018a643d8 100644 --- a/webrtc/p2p/base/jseptransport.cc +++ b/webrtc/p2p/base/jseptransport.cc @@ -329,7 +329,12 @@ bool JsepTransport::ApplyLocalTransportDescription( std::string* error_desc) { dtls_transport->ice_transport()->SetIceParameters( local_description_->GetIceParameters()); - return true; + bool ret = true; + if (certificate_) { + ret = dtls_transport->SetLocalCertificate(certificate_); + RTC_DCHECK(ret); + } + return ret; } bool JsepTransport::ApplyRemoteTransportDescription( diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc index 820645b42b..1dc21180ad 100644 --- a/webrtc/p2p/base/transportcontroller.cc +++ b/webrtc/p2p/base/transportcontroller.cc @@ -262,10 +262,6 @@ DtlsTransportInternal* TransportController::CreateDtlsTransport_n( dtls->ice_transport()->SetIceRole(ice_role_); dtls->ice_transport()->SetIceTiebreaker(ice_tiebreaker_); dtls->ice_transport()->SetIceConfig(ice_config_); - if (certificate_) { - bool set_cert_success = dtls->SetLocalCertificate(certificate_); - RTC_DCHECK(set_cert_success); - } // Connect to signals offered by the channels. Currently, the DTLS channel // forwards signals from the ICE channel, so we only need to connect to the @@ -539,16 +535,13 @@ bool TransportController::SetLocalCertificate_n( } certificate_ = certificate; - // Set certificate both for Transport, which verifies it matches the - // fingerprint in SDP... + // Set certificate for JsepTransport, which verifies it matches the + // fingerprint in SDP, and only applies it to the DTLS transport if a + // fingerprint attribute is present in SDP. This is used for fallback from + // DTLS to SDES. for (auto& kv : transports_) { kv.second->SetLocalCertificate(certificate_); } - // ... and for the DTLS channel, which needs it for the DTLS handshake. - for (auto& channel : channels_) { - bool set_cert_success = channel->dtls()->SetLocalCertificate(certificate); - RTC_DCHECK(set_cert_success); - } return true; } diff --git a/webrtc/p2p/base/transportdescriptionfactory.cc b/webrtc/p2p/base/transportdescriptionfactory.cc index 61e2a9f346..238e1b66a1 100644 --- a/webrtc/p2p/base/transportdescriptionfactory.cc +++ b/webrtc/p2p/base/transportdescriptionfactory.cc @@ -110,7 +110,12 @@ bool TransportDescriptionFactory::SetSecurityInfo( // 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. + // the certificate's signature, which is what CreateFromCertificate does. + desc->identity_fingerprint.reset( + rtc::SSLFingerprint::CreateFromCertificate(certificate_)); + if (!desc->identity_fingerprint) { + return false; + } std::string digest_alg; if (!certificate_->ssl_certificate().GetSignatureDigestAlgorithm( &digest_alg)) {