From b57e169f3c63204af8929835bd9e708726b49296 Mon Sep 17 00:00:00 2001 From: Zhi Huang Date: Tue, 12 Jun 2018 11:41:11 -0700 Subject: [PATCH] Add a flag to actively reset the SRTP parameters Add a new flag to RtcConfiguration. By setting that flag to true, the SRTP parameters will be reset whenever the DTLS transports are reset after every offer/answer negotiation. The flag is added to Android and Objc wrapper as well. This should only be used as a workaround for the linked bug, if the application knows that the other party is affected (for instance, using a version number). TBR=sakal@webrtc.org, denicija@webrtc.org Bug: chromium:835958 Change-Id: I6db025e1c69bf83e1b1908f7df4627430db9920c Reviewed-on: https://webrtc-review.googlesource.com/83101 Commit-Queue: Zhi Huang Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#23587} --- api/peerconnectioninterface.h | 7 +++ pc/dtlssrtptransport.cc | 5 +- pc/dtlssrtptransport.h | 8 ++++ pc/dtlssrtptransport_unittest.cc | 48 +++++++++++++++++++ pc/jseptransport.cc | 9 ++++ pc/jseptransport.h | 2 + pc/jseptransportcontroller.cc | 20 ++++++++ pc/jseptransportcontroller.h | 3 ++ pc/peerconnection.cc | 14 +++++- .../api/org/webrtc/PeerConnection.java | 10 ++++ sdk/android/src/jni/pc/peerconnection.cc | 2 + .../PeerConnection/RTCConfiguration.mm | 8 +++- .../Headers/WebRTC/RTCConfiguration.h | 6 +++ .../UnitTests/RTCPeerConnectionTest.mm | 2 + 14 files changed, 139 insertions(+), 5 deletions(-) diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index ab9f458e01..757811ece4 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -577,6 +577,13 @@ class PeerConnectionInterface : public rtc::RefCountInterface { // For all other users, specify kUnifiedPlan. SdpSemantics sdp_semantics = SdpSemantics::kPlanB; + // Actively reset the SRTP parameters whenever the DTLS transports + // underneath are reset for every offer/answer negotiation. + // This is only intended to be a workaround for crbug.com/835958 + // WARNING: This would cause RTP/RTCP packets decryption failure if not used + // correctly. This flag will be deprecated soon. Do not rely on it. + bool active_reset_srtp_params = false; + // // Don't forget to update operator== if adding something. // diff --git a/pc/dtlssrtptransport.cc b/pc/dtlssrtptransport.cc index a452d970d5..e31b2f5872 100644 --- a/pc/dtlssrtptransport.cc +++ b/pc/dtlssrtptransport.cc @@ -39,7 +39,10 @@ void DtlsSrtpTransport::SetDtlsTransports( // When using DTLS-SRTP, we must reset the SrtpTransport every time the // DtlsTransport changes and wait until the DTLS handshake is complete to set // the newly negotiated parameters. - if (IsSrtpActive() && rtp_dtls_transport != rtp_dtls_transport_) { + // If |active_reset_srtp_params_| is true, intentionally reset the SRTP + // parameter even though the DtlsTransport may not change. + if (IsSrtpActive() && (rtp_dtls_transport != rtp_dtls_transport_ || + active_reset_srtp_params_)) { ResetParams(); } diff --git a/pc/dtlssrtptransport.h b/pc/dtlssrtptransport.h index c24034a146..a2d7aadf67 100644 --- a/pc/dtlssrtptransport.h +++ b/pc/dtlssrtptransport.h @@ -53,6 +53,12 @@ class DtlsSrtpTransport : public SrtpTransport { "Set SRTP keys for DTLS-SRTP is not supported."); } + // If |active_reset_srtp_params_| is set to be true, the SRTP parameters will + // be reset whenever the DtlsTransports are reset. + void SetActiveResetSrtpParams(bool active_reset_srtp_params) { + active_reset_srtp_params_ = active_reset_srtp_params; + } + private: bool IsDtlsActive(); bool IsDtlsConnected(); @@ -84,6 +90,8 @@ class DtlsSrtpTransport : public SrtpTransport { // The encrypted header extension IDs. rtc::Optional> send_extension_ids_; rtc::Optional> recv_extension_ids_; + + bool active_reset_srtp_params_ = false; }; } // namespace webrtc diff --git a/pc/dtlssrtptransport_unittest.cc b/pc/dtlssrtptransport_unittest.cc index e1da4628ad..cfe817f3f8 100644 --- a/pc/dtlssrtptransport_unittest.cc +++ b/pc/dtlssrtptransport_unittest.cc @@ -510,3 +510,51 @@ TEST_F(DtlsSrtpTransportTest, SrtpSessionNotResetWhenRtcpTransportRemoved) { // errors when a packet with a duplicate SRTCP index is received. SendRecvRtcpPackets(); } + +// Tests that RTCP packets can be sent and received if both sides actively reset +// the SRTP parameters with the |active_reset_srtp_params_| flag. +TEST_F(DtlsSrtpTransportTest, ActivelyResetSrtpParams) { + auto rtp_dtls1 = rtc::MakeUnique( + "audio", cricket::ICE_CANDIDATE_COMPONENT_RTP); + auto rtcp_dtls1 = rtc::MakeUnique( + "audio", cricket::ICE_CANDIDATE_COMPONENT_RTCP); + auto rtp_dtls2 = rtc::MakeUnique( + "audio", cricket::ICE_CANDIDATE_COMPONENT_RTP); + auto rtcp_dtls2 = rtc::MakeUnique( + "audio", cricket::ICE_CANDIDATE_COMPONENT_RTCP); + + MakeDtlsSrtpTransports(rtp_dtls1.get(), rtcp_dtls1.get(), rtp_dtls2.get(), + rtcp_dtls2.get(), /*rtcp_mux_enabled=*/true); + CompleteDtlsHandshake(rtp_dtls1.get(), rtp_dtls2.get()); + CompleteDtlsHandshake(rtcp_dtls1.get(), rtcp_dtls2.get()); + + // Send some RTCP packets, causing the SRTCP index to be incremented. + SendRecvRtcpPackets(); + + // Only set the |active_reset_srtp_params_| flag to be true one side. + dtls_srtp_transport1_->SetActiveResetSrtpParams(true); + // Set RTCP transport to null to trigger the SRTP parameters update. + dtls_srtp_transport1_->SetDtlsTransports(rtp_dtls1.get(), nullptr); + dtls_srtp_transport2_->SetDtlsTransports(rtp_dtls2.get(), nullptr); + + // Sending some RTCP packets. + size_t rtcp_len = sizeof(kRtcpReport); + size_t packet_size = rtcp_len + 4 + kRtpAuthTagLen; + rtc::Buffer rtcp_packet_buffer(packet_size); + rtc::CopyOnWriteBuffer rtcp_packet(kRtcpReport, rtcp_len, packet_size); + int prev_received_packets = transport_observer2_.rtcp_count(); + ASSERT_TRUE(dtls_srtp_transport1_->SendRtcpPacket( + &rtcp_packet, rtc::PacketOptions(), cricket::PF_SRTP_BYPASS)); + // The RTCP packet is not exepected to be received because the SRTP parameters + // are only reset on one side and the SRTCP index is out of sync. + EXPECT_EQ(prev_received_packets, transport_observer2_.rtcp_count()); + + // Set the flag to be true on the other side. + dtls_srtp_transport2_->SetActiveResetSrtpParams(true); + // Set RTCP transport to null to trigger the SRTP parameters update. + dtls_srtp_transport1_->SetDtlsTransports(rtp_dtls1.get(), nullptr); + dtls_srtp_transport2_->SetDtlsTransports(rtp_dtls2.get(), nullptr); + + // RTCP packets flow is expected to work just fine. + SendRecvRtcpPackets(); +} diff --git a/pc/jseptransport.cc b/pc/jseptransport.cc index ab785affb1..adce4af14b 100644 --- a/pc/jseptransport.cc +++ b/pc/jseptransport.cc @@ -330,6 +330,15 @@ webrtc::RTCError JsepTransport::VerifyCertificateFingerprint( std::string(desc.str())); } +void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) { + if (dtls_srtp_transport_) { + RTC_LOG(INFO) + << "Setting active_reset_srtp_params of DtlsSrtpTransport to: " + << active_reset_srtp_params; + dtls_srtp_transport_->SetActiveResetSrtpParams(active_reset_srtp_params); + } +} + void JsepTransport::SetLocalIceParameters(IceTransportInternal* ice_transport) { RTC_DCHECK(ice_transport); RTC_DCHECK(local_description_); diff --git a/pc/jseptransport.h b/pc/jseptransport.h index 290ac4a2db..392f861f25 100644 --- a/pc/jseptransport.h +++ b/pc/jseptransport.h @@ -173,6 +173,8 @@ class JsepTransport : public sigslot::has_slots<> { const rtc::RTCCertificate* certificate, const rtc::SSLFingerprint* fingerprint) const; + void SetActiveResetSrtpParams(bool active_reset_srtp_params); + private: bool SetRtcpMux(bool enable, webrtc::SdpType type, ContentSource source); diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc index 2d3eeb9b70..52519c0443 100644 --- a/pc/jseptransportcontroller.cc +++ b/pc/jseptransportcontroller.cc @@ -379,6 +379,24 @@ void JsepTransportController::SetMetricsObserver( } } +void JsepTransportController::SetActiveResetSrtpParams( + bool active_reset_srtp_params) { + if (!network_thread_->IsCurrent()) { + network_thread_->Invoke(RTC_FROM_HERE, [=] { + SetActiveResetSrtpParams(active_reset_srtp_params); + }); + return; + } + + RTC_LOG(INFO) + << "Updating the active_reset_srtp_params for JsepTransportController: " + << active_reset_srtp_params; + config_.active_reset_srtp_params = active_reset_srtp_params; + for (auto& kv : jsep_transports_by_name_) { + kv.second->SetActiveResetSrtpParams(active_reset_srtp_params); + } +} + std::unique_ptr JsepTransportController::CreateDtlsTransport(const std::string& transport_name, bool rtcp) { @@ -478,6 +496,8 @@ JsepTransportController::CreateDtlsSrtpTransport( dtls_srtp_transport->SetDtlsTransports(rtp_dtls_transport, rtcp_dtls_transport); + dtls_srtp_transport->SetActiveResetSrtpParams( + config_.active_reset_srtp_params); return dtls_srtp_transport; } diff --git a/pc/jseptransportcontroller.h b/pc/jseptransportcontroller.h index dee05faee8..b678a09048 100644 --- a/pc/jseptransportcontroller.h +++ b/pc/jseptransportcontroller.h @@ -78,6 +78,7 @@ class JsepTransportController : public sigslot::has_slots<>, // Used to inject the ICE/DTLS transports created externally. cricket::TransportFactoryInterface* external_transport_factory = nullptr; Observer* transport_observer = nullptr; + bool active_reset_srtp_params = false; RtcEventLog* event_log = nullptr; }; @@ -155,6 +156,8 @@ class JsepTransportController : public sigslot::has_slots<>, bool initial_offerer() const { return initial_offerer_ && *initial_offerer_; } + void SetActiveResetSrtpParams(bool active_reset_srtp_params); + // All of these signals are fired on the signaling thread. // If any transport failed => failed, diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 093e72d77b..cf56c350f6 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -693,6 +693,7 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( webrtc::TurnCustomizer* turn_customizer; SdpSemantics sdp_semantics; rtc::Optional network_preference; + bool active_reset_srtp_params; }; static_assert(sizeof(stuff_being_tested_for_equality) == sizeof(*this), "Did you add something to RTCConfiguration and forget to " @@ -739,7 +740,8 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( ice_regather_interval_range == o.ice_regather_interval_range && turn_customizer == o.turn_customizer && sdp_semantics == o.sdp_semantics && - network_preference == o.network_preference; + network_preference == o.network_preference && + active_reset_srtp_params == o.active_reset_srtp_params; } bool PeerConnectionInterface::RTCConfiguration::operator!=( @@ -938,6 +940,7 @@ bool PeerConnection::Initialize( #if defined(ENABLE_EXTERNAL_AUTH) config.enable_external_auth = true; #endif + config.active_reset_srtp_params = configuration.active_reset_srtp_params; transport_controller_.reset(new JsepTransportController( signaling_thread(), network_thread(), port_allocator_.get(), config)); transport_controller_->SignalIceConnectionState.connect( @@ -2841,7 +2844,6 @@ PeerConnectionInterface::RTCConfiguration PeerConnection::GetConfiguration() { bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration, RTCError* error) { TRACE_EVENT0("webrtc", "PeerConnection::SetConfiguration"); - if (IsClosed()) { RTC_LOG(LS_ERROR) << "SetConfiguration: PeerConnection is closed."; return SafeSetError(RTCErrorType::INVALID_STATE, error); @@ -2879,6 +2881,8 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration, configuration.stun_candidate_keepalive_interval; modified_config.turn_customizer = configuration.turn_customizer; modified_config.network_preference = configuration.network_preference; + modified_config.active_reset_srtp_params = + configuration.active_reset_srtp_params; if (configuration != modified_config) { RTC_LOG(LS_ERROR) << "Modifying the configuration in an unsupported way."; return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error); @@ -2937,6 +2941,12 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration, transport_controller_->SetIceConfig(ParseIceConfig(modified_config)); + if (configuration_.active_reset_srtp_params != + modified_config.active_reset_srtp_params) { + transport_controller_->SetActiveResetSrtpParams( + modified_config.active_reset_srtp_params); + } + configuration_ = modified_config; return SafeSetError(RTCErrorType::NONE, error); } diff --git a/sdk/android/api/org/webrtc/PeerConnection.java b/sdk/android/api/org/webrtc/PeerConnection.java index b9e31e1e6b..2f9adcf962 100644 --- a/sdk/android/api/org/webrtc/PeerConnection.java +++ b/sdk/android/api/org/webrtc/PeerConnection.java @@ -457,6 +457,10 @@ public class PeerConnection { // This is an optional wrapper for the C++ webrtc::TurnCustomizer. @Nullable public TurnCustomizer turnCustomizer; + // Actively reset the SRTP parameters whenever the DTLS transports underneath are reset for + // every offer/answer negotiation.This is only intended to be a workaround for crbug.com/835958 + public boolean activeResetSrtpParams; + // TODO(deadbeef): Instead of duplicating the defaults here, we should do // something to pick up the defaults from C++. The Objective-C equivalent // of RTCConfiguration does that. @@ -495,6 +499,7 @@ public class PeerConnection { enableDtlsSrtp = null; networkPreference = AdapterType.UNKNOWN; sdpSemantics = SdpSemantics.PLAN_B; + activeResetSrtpParams = false; } @CalledByNative("RTCConfiguration") @@ -682,6 +687,11 @@ public class PeerConnection { SdpSemantics getSdpSemantics() { return sdpSemantics; } + + @CalledByNative("RTCConfiguration") + boolean getActiveResetSrtpParams() { + return activeResetSrtpParams; + } }; private final List localStreams = new ArrayList<>(); diff --git a/sdk/android/src/jni/pc/peerconnection.cc b/sdk/android/src/jni/pc/peerconnection.cc index 225443f9ec..722a785ea1 100644 --- a/sdk/android/src/jni/pc/peerconnection.cc +++ b/sdk/android/src/jni/pc/peerconnection.cc @@ -232,6 +232,8 @@ void JavaToNativeRTCConfiguration( rtc_config->network_preference = JavaToNativeNetworkPreference(jni, j_network_preference); rtc_config->sdp_semantics = JavaToNativeSdpSemantics(jni, j_sdp_semantics); + rtc_config->active_reset_srtp_params = + Java_RTCConfiguration_getActiveResetSrtpParams(jni, j_rtc_config); } rtc::KeyType GetRtcConfigKeyType(JNIEnv* env, diff --git a/sdk/objc/Framework/Classes/PeerConnection/RTCConfiguration.mm b/sdk/objc/Framework/Classes/PeerConnection/RTCConfiguration.mm index e318a9f977..4bd0450f69 100644 --- a/sdk/objc/Framework/Classes/PeerConnection/RTCConfiguration.mm +++ b/sdk/objc/Framework/Classes/PeerConnection/RTCConfiguration.mm @@ -45,6 +45,7 @@ @synthesize iceRegatherIntervalRange = _iceRegatherIntervalRange; @synthesize sdpSemantics = _sdpSemantics; @synthesize turnCustomizer = _turnCustomizer; +@synthesize activeResetSrtpParams = _activeResetSrtpParams; - (instancetype)init { // Copy defaults. @@ -99,6 +100,7 @@ } _sdpSemantics = [[self class] sdpSemanticsForNativeSdpSemantics:config.sdp_semantics]; _turnCustomizer = config.turn_customizer; + _activeResetSrtpParams = config.active_reset_srtp_params; } return self; } @@ -106,7 +108,7 @@ - (NSString *)description { static NSString *formatString = @"RTCConfiguration: " - @"{\n%@\n%@\n%@\n%@\n%@\n%@\n%@\n%@\n%d\n%d\n%d\n%d\n%d\n%d\n%d\n%@\n%@\n%d\n%d\n}\n"; + @"{\n%@\n%@\n%@\n%@\n%@\n%@\n%@\n%@\n%d\n%d\n%d\n%d\n%d\n%d\n%d\n%@\n%@\n%d\n%d\n%d\n}\n"; return [NSString stringWithFormat:formatString, @@ -128,7 +130,8 @@ _iceCheckMinInterval, _iceRegatherIntervalRange, _disableLinkLocalNetworks, - _maxIPv6Networks]; + _maxIPv6Networks, + _activeResetSrtpParams]; } #pragma mark - Private @@ -194,6 +197,7 @@ if (_turnCustomizer) { nativeConfig->turn_customizer = _turnCustomizer; } + nativeConfig->active_reset_srtp_params = _activeResetSrtpParams ? true : false; return nativeConfig.release(); } diff --git a/sdk/objc/Framework/Headers/WebRTC/RTCConfiguration.h b/sdk/objc/Framework/Headers/WebRTC/RTCConfiguration.h index dd8f364cbd..b203b28156 100644 --- a/sdk/objc/Framework/Headers/WebRTC/RTCConfiguration.h +++ b/sdk/objc/Framework/Headers/WebRTC/RTCConfiguration.h @@ -162,6 +162,12 @@ RTC_EXPORT */ @property(nonatomic, assign) RTCSdpSemantics sdpSemantics; +/** Actively reset the SRTP parameters when the DTLS transports underneath are + * changed after offer/answer negotiation. This is only intended to be a + * workaround for crbug.com/835958 + */ +@property(nonatomic, assign) BOOL activeResetSrtpParams; + - (instancetype)init; @end diff --git a/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm b/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm index 686bf2d37d..63617cbe5a 100644 --- a/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm +++ b/sdk/objc/Framework/UnitTests/RTCPeerConnectionTest.mm @@ -49,6 +49,7 @@ config.continualGatheringPolicy = RTCContinualGatheringPolicyGatherContinually; config.shouldPruneTurnPorts = YES; + config.activeResetSrtpParams = YES; RTCMediaConstraints *contraints = [[RTCMediaConstraints alloc] initWithMandatoryConstraints:@{} optionalConstraints:nil]; @@ -87,6 +88,7 @@ newConfig.iceBackupCandidatePairPingInterval); EXPECT_EQ(config.continualGatheringPolicy, newConfig.continualGatheringPolicy); EXPECT_EQ(config.shouldPruneTurnPorts, newConfig.shouldPruneTurnPorts); + EXPECT_EQ(config.activeResetSrtpParams, newConfig.activeResetSrtpParams); } @end