diff --git a/pc/dtlssrtptransport.cc b/pc/dtlssrtptransport.cc index 4ccec73e6c..a452d970d5 100644 --- a/pc/dtlssrtptransport.cc +++ b/pc/dtlssrtptransport.cc @@ -39,18 +39,20 @@ 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()) { + if (IsSrtpActive() && rtp_dtls_transport != rtp_dtls_transport_) { ResetParams(); } const std::string transport_name = rtp_dtls_transport ? rtp_dtls_transport->transport_name() : "null"; - // This would only be possible if using BUNDLE but not rtcp-mux, which isn't - // allowed according to the BUNDLE spec. - RTC_CHECK(!(IsSrtpActive())) - << "Setting RTCP for DTLS/SRTP after the DTLS is active " - "should never happen."; + if (rtcp_dtls_transport && rtcp_dtls_transport != rtcp_dtls_transport_) { + // This would only be possible if using BUNDLE but not rtcp-mux, which isn't + // allowed according to the BUNDLE spec. + RTC_CHECK(!(IsSrtpActive())) + << "Setting RTCP for DTLS/SRTP after the DTLS is active " + "should never happen."; + } RTC_LOG(LS_INFO) << "Setting RTCP Transport on " << transport_name << " transport " << rtcp_dtls_transport; diff --git a/pc/dtlssrtptransport_unittest.cc b/pc/dtlssrtptransport_unittest.cc index 37d517b236..e1da4628ad 100644 --- a/pc/dtlssrtptransport_unittest.cc +++ b/pc/dtlssrtptransport_unittest.cc @@ -123,16 +123,21 @@ class DtlsSrtpTransportTest : public testing::Test, rtc::PacketOptions options; // Send a packet from |srtp_transport1_| to |srtp_transport2_| and verify // that the packet can be successfully received and decrypted. + int prev_received_packets = transport_observer2_.rtp_count(); ASSERT_TRUE(dtls_srtp_transport1_->SendRtpPacket(&rtp_packet1to2, options, cricket::PF_SRTP_BYPASS)); ASSERT_TRUE(transport_observer2_.last_recv_rtp_packet().data()); EXPECT_EQ(0, memcmp(transport_observer2_.last_recv_rtp_packet().data(), kPcmuFrame, rtp_len)); + EXPECT_EQ(prev_received_packets + 1, transport_observer2_.rtp_count()); + + prev_received_packets = transport_observer1_.rtp_count(); ASSERT_TRUE(dtls_srtp_transport2_->SendRtpPacket(&rtp_packet2to1, options, cricket::PF_SRTP_BYPASS)); ASSERT_TRUE(transport_observer1_.last_recv_rtp_packet().data()); EXPECT_EQ(0, memcmp(transport_observer1_.last_recv_rtp_packet().data(), kPcmuFrame, rtp_len)); + EXPECT_EQ(prev_received_packets + 1, transport_observer1_.rtp_count()); } void SendRecvRtcpPackets() { @@ -148,18 +153,22 @@ class DtlsSrtpTransportTest : public testing::Test, rtc::PacketOptions options; // Send a packet from |srtp_transport1_| to |srtp_transport2_| and verify // that the packet can be successfully received and decrypted. + int prev_received_packets = transport_observer2_.rtcp_count(); ASSERT_TRUE(dtls_srtp_transport1_->SendRtcpPacket(&rtcp_packet1to2, options, cricket::PF_SRTP_BYPASS)); ASSERT_TRUE(transport_observer2_.last_recv_rtcp_packet().data()); EXPECT_EQ(0, memcmp(transport_observer2_.last_recv_rtcp_packet().data(), kRtcpReport, rtcp_len)); + EXPECT_EQ(prev_received_packets + 1, transport_observer2_.rtcp_count()); // Do the same thing in the opposite direction; + prev_received_packets = transport_observer1_.rtcp_count(); ASSERT_TRUE(dtls_srtp_transport2_->SendRtcpPacket(&rtcp_packet2to1, options, cricket::PF_SRTP_BYPASS)); ASSERT_TRUE(transport_observer1_.last_recv_rtcp_packet().data()); EXPECT_EQ(0, memcmp(transport_observer1_.last_recv_rtcp_packet().data(), kRtcpReport, rtcp_len)); + EXPECT_EQ(prev_received_packets + 1, transport_observer1_.rtcp_count()); } void SendRecvRtpPacketsWithHeaderExtension( @@ -469,3 +478,35 @@ TEST_F(DtlsSrtpTransportTest, SignalReadyToSendFiredWithoutRtcpMux) { EXPECT_TRUE(transport_observer1_.ready_to_send()); EXPECT_TRUE(transport_observer2_.ready_to_send()); } + +// Test that if an endpoint "fully" enables RTCP mux, setting the RTCP +// transport to null, it *doesn't* reset its SRTP context. That would cause the +// ROC and SRTCP index to be reset, causing replay detection and other errors +// when attempting to unprotect packets. +// Regression test for bugs.webrtc.org/8996 +TEST_F(DtlsSrtpTransportTest, SrtpSessionNotResetWhenRtcpTransportRemoved) { + 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(); + + // Set RTCP transport to null, which previously would trigger this problem. + dtls_srtp_transport1_->SetDtlsTransports(rtp_dtls1.get(), nullptr); + + // Attempt to send more RTCP packets. If the issue occurred, one side would + // reset its context while the other would not, causing replay detection + // errors when a packet with a duplicate SRTCP index is received. + SendRecvRtcpPackets(); +}