admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix bug that reset SRTP context on every applied answer.

Browse Source 
This causes the SRTCP index and SRTP ROC to be reset, which will cause replay
detection errors in decrypting SRTCP packets, and errors in decrypting SRTP
packets if the ROC was nonzero.

Bug: webrtc:8996
Change-Id: I3bf6c136d928f39b19de05616d5cd2833f42223c
Reviewed-on: https://webrtc-review.googlesource.com/71300
Reviewed-by: Zhi Huang <zhihuang@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22965}
This commit is contained in:
Taylor Brandstetter 2018-04-19 14:44:12 -07:00 committed by Commit Bot
parent d085936e6a
commit 53e43b3060
2 changed files with 49 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
pc/dtlssrtptransport.cc
View File

@ -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;

41
pc/dtlssrtptransport_unittest.cc
View File

@ -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<FakeDtlsTransport>(
"audio", cricket::ICE_CANDIDATE_COMPONENT_RTP);
auto rtcp_dtls1 = rtc::MakeUnique<FakeDtlsTransport>(
"audio", cricket::ICE_CANDIDATE_COMPONENT_RTCP);
auto rtp_dtls2 = rtc::MakeUnique<FakeDtlsTransport>(
"audio", cricket::ICE_CANDIDATE_COMPONENT_RTP);
auto rtcp_dtls2 = rtc::MakeUnique<FakeDtlsTransport>(
"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();
}