From 13b327b05fa3788b4daa9c3463e13282824cb320 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Sat, 17 Aug 2024 07:42:21 -0700 Subject: [PATCH] srtp: demonstrate wraparound with loss decryption failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit by encryption a packet with sequence number 65535 followed by a packet with sequence number 1. The second packet is encrypted with a SRTP ROC of 1 as described in https://datatracker.ietf.org/doc/html/rfc3711#section-3.3.1 The packets are (received and) decrypted in a different order, the packet with sequence number 1 (and ROC=1) is decrypted first. Since the ROC is maintained locally the decrypting session assumes it to be 0. Why is that a problem? The RFC recommends estimating the ROC with +-1 which, as demonstrated by the test, libSRTP does not. But this is a rare problem that requires a random in a high range combined with packet loss/reordering which turns into no-a-problem if you choose carefully as done by packet_sequencer.cc which restricts the initial sequence number in the range 0..32767 which means you do not run into this issue in production. See also Q6 in libsrtp's historical documentation at https://srtp.sourceforge.net/historical/faq.html BUG=webrtc:353565743 Change-Id: I9bd72b198c946937aeb25c229005a0c682447f53 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/358360 Reviewed-by: Erik Språng Reviewed-by: Harald Alvestrand Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/main@{#42798} --- modules/rtp_rtcp/source/packet_sequencer.cc | 7 +-- pc/srtp_session_unittest.cc | 56 +++++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/modules/rtp_rtcp/source/packet_sequencer.cc b/modules/rtp_rtcp/source/packet_sequencer.cc index 5f2f69f830..f640351362 100644 --- a/modules/rtp_rtcp/source/packet_sequencer.cc +++ b/modules/rtp_rtcp/source/packet_sequencer.cc @@ -38,9 +38,10 @@ PacketSequencer::PacketSequencer(uint32_t media_ssrc, last_packet_marker_bit_(false) { Random random(clock_->TimeInMicroseconds()); // Random start, 16 bits. Upper half of range is avoided in order to prevent - // wraparound issues during startup. Sequence number 0 is avoided for - // historical reasons, presumably to avoid debugability or test usage - // conflicts. + // SRTP wraparound issues during startup. See this unit test for details: + // SrtpSessionTest.ProtectUnprotectWrapAroundRocMismatch + // Sequence number 0 is avoided for historical reasons, presumably to avoid + // debugability or test usage conflicts. constexpr uint16_t kMaxInitRtpSeqNumber = 0x7fff; // 2^15 - 1. media_sequence_number_ = random.Rand(1, kMaxInitRtpSeqNumber); rtx_sequence_number_ = random.Rand(1, kMaxInitRtpSeqNumber); diff --git a/pc/srtp_session_unittest.cc b/pc/srtp_session_unittest.cc index 613a872619..eb6562392b 100644 --- a/pc/srtp_session_unittest.cc +++ b/pc/srtp_session_unittest.cc @@ -284,4 +284,60 @@ TEST_F(SrtpSessionTest, RemoveSsrc) { EXPECT_TRUE(s2_.RemoveSsrcFromSession(1)); } +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 + // failures when it wraps around with packet loss. Pick your starting + // sequence number in the lower half of the range for robustness reasons, + // see packet_sequencer.cc for the code doing so. + EXPECT_TRUE(s1_.SetSend(kSrtpAes128CmSha1_80, kTestKey1, kTestKeyLen, + kEncryptedHeaderExtensionIds)); + EXPECT_TRUE(s2_.SetRecv(kSrtpAes128CmSha1_80, kTestKey1, kTestKeyLen, + 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 + }; + 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 + }; + + 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); + + // 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)); + // Decrypt frame 1. + EXPECT_TRUE(s2_.UnprotectRtp(kFrame1, sizeof(kFrame1), &out_len)); + // 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)); +} + } // namespace rtc