From bae103126c5bdaf1361bcff4750eb5ebe10020ee Mon Sep 17 00:00:00 2001 From: Zhi Huang Date: Fri, 8 Jun 2018 14:55:27 -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. 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). Bug: chromium:835958 Change-Id: Ifb4b99f68dc272507728ab59c07627f0d1b9c605 Reviewed-on: https://webrtc-review.googlesource.com/81642 Commit-Queue: Zhi Huang Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#23570} --- api/peerconnectioninterface.h | 7 +++++ pc/dtlssrtptransport.cc | 5 +++- pc/dtlssrtptransport.h | 8 ++++++ pc/dtlssrtptransport_unittest.cc | 48 ++++++++++++++++++++++++++++++++ pc/jseptransportcontroller.cc | 2 ++ pc/jseptransportcontroller.h | 1 + pc/peerconnection.cc | 5 +++- 7 files changed, 74 insertions(+), 2 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/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc index d05339e1a1..7d456f9601 100644 --- a/pc/jseptransportcontroller.cc +++ b/pc/jseptransportcontroller.cc @@ -478,6 +478,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 ebe105b510..6a2e7d2b6d 100644 --- a/pc/jseptransportcontroller.h +++ b/pc/jseptransportcontroller.h @@ -77,6 +77,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; }; // The ICE related events are signaled on the |signaling_thread|. diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 0a984a703f..1f792c3e71 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!=( @@ -937,6 +939,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(