From 9ff254eaf2cd799a604178ada2a89500794e13a0 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Wed, 18 Dec 2024 10:01:44 -0800 Subject: [PATCH] srtp: stop using private libsrtp function to determine packet index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit instead use the standard API to get the rollover counter and determine the extended sequence number which is the basis for the packet index. See https://github.com/cisco/libsrtp/issues/738 and https://github.com/cisco/libsrtp/issues/721 BUG=webrtc:357776213 Change-Id: I90c5a4a538f56132158aa48db8700187fcdb47d2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/371960 Commit-Queue: Philipp Hancke Reviewed-by: Harald Alvestrand Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/main@{#43802} --- pc/srtp_session.cc | 17 +++++---- pc/srtp_session_unittest.cc | 73 +++++++++++++++++++++++++++++-------- 2 files changed, 68 insertions(+), 22 deletions(-) diff --git a/pc/srtp_session.cc b/pc/srtp_session.cc index 9b98080886..bf6ec99477 100644 --- a/pc/srtp_session.cc +++ b/pc/srtp_session.cc @@ -507,16 +507,19 @@ bool SrtpSession::RemoveSsrcFromSession(uint32_t ssrc) { bool SrtpSession::GetSendStreamPacketIndex(rtc::CopyOnWriteBuffer& buffer, int64_t* index) { RTC_DCHECK(thread_checker_.IsCurrent()); - // libSRTP expects the SSRC to be in network byte order. - srtp_stream_ctx_t* stream = - srtp_get_stream(session_, htonl(webrtc::ParseRtpSsrc(buffer))); - if (!stream) { + const srtp_hdr_t* hdr = reinterpret_cast(buffer.data()); + + uint32_t roc; + if (srtp_get_stream_roc(session_, htonl(hdr->ssrc), &roc) != + srtp_err_status_ok) { return false; } + // Calculate the extended sequence number. + uint16_t seq_num = webrtc::ParseRtpSequenceNumber(buffer); + int64_t extended_seq_num = (roc << 16) + seq_num; - // Shift packet index, put into network byte order - *index = static_cast(rtc::NetworkToHost64( - srtp_rdbx_get_packet_index(&stream->rtp_rdbx) << 16)); + // Shift extended sequence number, put into network byte order + *index = static_cast(rtc::NetworkToHost64(extended_seq_num << 16)); return true; } diff --git a/pc/srtp_session_unittest.cc b/pc/srtp_session_unittest.cc index 35d4108afc..2ae3a8a473 100644 --- a/pc/srtp_session_unittest.cc +++ b/pc/srtp_session_unittest.cc @@ -285,8 +285,6 @@ TEST_F(SrtpSessionTest, RemoveSsrc) { EXPECT_TRUE(s2_.RemoveSsrcFromSession(1)); } -#pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wdeprecated-declarations" TEST_F(SrtpSessionTest, ProtectUnprotectWrapAroundRocMismatch) { // This unit tests demonstrates why you should be careful when // choosing the initial RTP sequence number as there can be decryption @@ -308,6 +306,8 @@ TEST_F(SrtpSessionTest, ProtectUnprotectWrapAroundRocMismatch) { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, // clang-format on }; + rtc::CopyOnWriteBuffer packet1(kFrame1, sizeof(kFrame1) - 10, + sizeof(kFrame1)); unsigned char kFrame2[] = { // clang-format off // PT=0, SN=1, TS=0, SSRC=1 @@ -317,34 +317,77 @@ TEST_F(SrtpSessionTest, ProtectUnprotectWrapAroundRocMismatch) { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, // clang-format on }; + rtc::CopyOnWriteBuffer packet2(kFrame2, sizeof(kFrame2) - 10, + sizeof(kFrame1)); const unsigned char kPayload[] = {0xBE, 0xEF}; - int out_len; // Encrypt the frames in-order. There is a sequence number rollover from // 65535 to 1 (skipping 0) and the second packet gets encrypted with a // roll-over counter (ROC) of 1. See // https://datatracker.ietf.org/doc/html/rfc3711#section-3.3.1 - EXPECT_TRUE( - s1_.ProtectRtp(kFrame1, sizeof(kFrame1) - 10, sizeof(kFrame1), &out_len)); - EXPECT_EQ(out_len, 24); - EXPECT_TRUE( - s1_.ProtectRtp(kFrame2, sizeof(kFrame2) - 10, sizeof(kFrame2), &out_len)); - EXPECT_EQ(out_len, 24); + EXPECT_TRUE(s1_.ProtectRtp(packet1)); + EXPECT_EQ(packet1.size(), 24u); + EXPECT_TRUE(s1_.ProtectRtp(packet2)); + EXPECT_EQ(packet2.size(), 24u); // If we decrypt frame 2 first it will have a ROC of 1 but the receiver // does not know this is a rollover so will attempt with a ROC of 0. // Note: If libsrtp is modified to attempt to decrypt with ROC=1 for this // case, this test will fail and needs to be modified accordingly to unblock // the roll. See https://issues.webrtc.org/353565743 for details. - EXPECT_FALSE(s2_.UnprotectRtp(kFrame2, sizeof(kFrame2), &out_len)); + EXPECT_FALSE(s2_.UnprotectRtp(packet2)); // Decrypt frame 1. - EXPECT_TRUE(s2_.UnprotectRtp(kFrame1, sizeof(kFrame1), &out_len)); - EXPECT_EQ(0, std::memcmp(kFrame1 + 12, kPayload, sizeof(kPayload))); + EXPECT_TRUE(s2_.UnprotectRtp(packet1)); + ASSERT_EQ(packet1.size(), 14u); + EXPECT_EQ(0, std::memcmp(packet1.data() + 12, kPayload, sizeof(kPayload))); // Now decrypt frame 2 again. A rollover is detected which increases // the ROC to 1 so this succeeds. - EXPECT_TRUE(s2_.UnprotectRtp(kFrame2, sizeof(kFrame2), &out_len)); - EXPECT_EQ(0, std::memcmp(kFrame2 + 12, kPayload, sizeof(kPayload))); + EXPECT_TRUE(s2_.UnprotectRtp(packet2)); + ASSERT_EQ(packet2.size(), 14u); + EXPECT_EQ(0, std::memcmp(packet2.data() + 12, kPayload, sizeof(kPayload))); +} + +TEST_F(SrtpSessionTest, ProtectGetPacketIndex) { + EXPECT_TRUE(s1_.SetSend(kSrtpAes128CmSha1_80, kTestKey1, + kEncryptedHeaderExtensionIds)); + EXPECT_TRUE(s2_.SetReceive(kSrtpAes128CmSha1_80, kTestKey1, + kEncryptedHeaderExtensionIds)); + // Buffers include enough room for the 10 byte SRTP auth tag so we can + // encrypt in place. + unsigned char kFrame1[] = { + // clang-format off + // PT=0, SN=65535, TS=0, SSRC=1 + 0x80, 0x00, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, + 0xBE, 0xEF, // data bytes + // Space for the SRTP auth tag + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + // clang-format on + }; + rtc::CopyOnWriteBuffer packet1(kFrame1, sizeof(kFrame1) - 10, + sizeof(kFrame1)); + unsigned char kFrame2[] = { + // clang-format off + // PT=0, SN=1, TS=0, SSRC=1 + 0x80, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, + 0xBE, 0xEF, // data bytes + // Space for the SRTP auth tag + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + // clang-format on + }; + rtc::CopyOnWriteBuffer packet2(kFrame2, sizeof(kFrame2) - 10, + sizeof(kFrame1)); + + // Encrypt the frames in-order. There is a sequence number rollover from + // 65535 to 1 (skipping 0) and the second packet gets encrypted with a + // roll-over counter (ROC) of 1. See + // https://datatracker.ietf.org/doc/html/rfc3711#section-3.3.1 + int64_t index; + EXPECT_TRUE(s1_.ProtectRtp(packet1, &index)); + EXPECT_EQ(packet1.size(), 24u); + EXPECT_EQ(index, 0xffff00000000); // ntohl(65535 << 16) + EXPECT_TRUE(s1_.ProtectRtp(packet2, &index)); + EXPECT_EQ(packet2.size(), 24u); + EXPECT_EQ(index, 0x10001000000); // ntohl(65537 << 16) } -#pragma clang diagnostic pop } // namespace rtc