From 2e04a1597112e490415a44fe7cb4fc6fb977f59e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20B=C3=BChler?= Date: Wed, 2 Sep 2026 07:50:54 +0200 Subject: [PATCH] srtp: detect SSRC collisions on RCC unprotect Deferring index estimation for ROC-carrying packets also skipped the sender-direction check, so an RCC sender stream would accept its own packets instead of reporting a collision. --- srtp/srtp.c | 28 ++++++++++--------- test/rcc_test.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 12 deletions(-) diff --git a/srtp/srtp.c b/srtp/srtp.c index 90545f963..d7e9f66e5 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -3001,6 +3001,22 @@ srtp_err_status_t srtp_unprotect(srtp_t ctx, rcc_carry = stream->rcc_mode != srtp_rcc_mode_none && (ntohs(hdr->seq) % stream->roc_tx_rate) == 0; + /* + * Verify that stream is for received traffic - this check will + * detect SSRC collisions, since a stream that appears in both + * srtp_protect() and srtp_unprotect() will fail this test in one of + * those functions. + * + * Skip it for the provisional template stream, which is not yet a + * real sender or receiver stream. RCC packets still have to run + * this check: deferring index estimation for ROC-carrying (and + * mode 3) packets must not also skip collision detection. + */ + if (!from_template && stream->direction == dir_srtp_sender) { + srtp_handle_event(ctx, stream, event_ssrc_collision); + return srtp_err_status_direction_mismatch; + } + if (from_template || rcc_carry || stream->rcc_mode == srtp_rcc_mode_3) { /* * set estimated packet index to sequence number from header, @@ -3009,18 +3025,6 @@ srtp_err_status_t srtp_unprotect(srtp_t ctx, est = (srtp_xtd_seq_num_t)ntohs(hdr->seq); delta = (int)est; } else { - /* - * Verify that stream is for received traffic - this check will - * detect SSRC collisions, since a stream that appears in both - * srtp_protect() and srtp_unprotect() will fail this test in one of - * those functions. - * - */ - if (stream->direction == dir_srtp_sender) { - srtp_handle_event(ctx, stream, event_ssrc_collision); - return srtp_err_status_direction_mismatch; - } - status = srtp_get_est_pkt_index(hdr, stream, &est, &delta); if (status && (status != srtp_err_status_pkt_idx_adv)) { diff --git a/test/rcc_test.c b/test/rcc_test.c index b68370206..4daca6239 100644 --- a/test/rcc_test.c +++ b/test/rcc_test.c @@ -165,6 +165,27 @@ static void rcc_roundtrip(srtp_t snd, srtp_t rcv, uint16_t seq, const char *msg) CHECK_BUFFER_EQUAL(dec, pkt, len); } +/* + * Protect a packet on policy, then try to unprotect it on the same session. + * After srtp_protect() the stream is a sender, so unprotect must report an + * SSRC collision rather than decrypting the packet. + */ +static void rcc_sender_must_reject_unprotect(srtp_policy_t policy, uint16_t seq) +{ + srtp_t sess; + uint8_t pkt[256], enc[256], dec[256]; + size_t len, enc_len, dec_len; + + CHECK_OK(srtp_create(&sess, policy)); + len = make_rtp(pkt, seq, "sender unprotect"); + enc_len = sizeof(enc); + CHECK_OK(srtp_protect(sess, pkt, len, enc, &enc_len, 0)); + dec_len = sizeof(dec); + CHECK_RETURN(srtp_unprotect(sess, enc, enc_len, dec, &dec_len), + srtp_err_status_direction_mismatch); + CHECK_OK(srtp_dealloc(sess)); +} + /* advance the sender's ROC to 1 by walking the sequence number past a wrap */ static void advance_sender_roc(srtp_t snd) { @@ -660,6 +681,36 @@ static void rcc_mode2_carry_out_of_order_keeps_window(void) CHECK_OK(srtp_shutdown()); } +/* + * An RCC-enabled sender stream must still reject unprotect as an SSRC + * collision. ROC-carrying packets (and every mode-3 packet) used to skip + * that check because their index is estimated later. + */ +static void rcc_mode2_sender_rejects_unprotect(void) +{ + srtp_policy_t p; + + CHECK_OK(srtp_init()); + create_cm_rcc_policy(&p, srtp_rcc_mode_2, 1); + /* R == 1: every packet carries the ROC */ + rcc_sender_must_reject_unprotect(p, 4); + srtp_policy_destroy(p); + CHECK_OK(srtp_shutdown()); +} + +static void rcc_mode1_sender_rejects_unprotect(void) +{ + srtp_policy_t p; + + CHECK_OK(srtp_init()); + create_cm_rcc_policy(&p, srtp_rcc_mode_1, 4); + /* seq 0 is ROC-carrying; seq 1 is untagged. Both must collide. */ + rcc_sender_must_reject_unprotect(p, 0); + rcc_sender_must_reject_unprotect(p, 1); + srtp_policy_destroy(p); + CHECK_OK(srtp_shutdown()); +} + #ifdef GCM /* * AES-GCM round trips (mode 3, RFC 7714 layout) @@ -977,6 +1028,20 @@ static void rcc_gcm_mode3_carry_replay_rejected(void) srtp_policy_destroy(rp); CHECK_OK(srtp_shutdown()); } + +static void rcc_gcm_mode3_sender_rejects_unprotect(void) +{ + srtp_policy_t p; + + CHECK_OK(srtp_init()); + create_gcm_rcc_policy(&p, srtp_rcc_mode_3, 4); + /* seq 0 carries the ROC; seq 1 does not. Mode 3 skipped the check on + * both. */ + rcc_sender_must_reject_unprotect(p, 0); + rcc_sender_must_reject_unprotect(p, 1); + srtp_policy_destroy(p); + CHECK_OK(srtp_shutdown()); +} #endif /* GCM */ TEST_LIST = { @@ -1000,6 +1065,10 @@ TEST_LIST = { { "rcc_mode2_carry_replay_rejected()", rcc_mode2_carry_replay_rejected }, { "rcc_mode2_carry_out_of_order_keeps_window()", rcc_mode2_carry_out_of_order_keeps_window }, + { "rcc_mode2_sender_rejects_unprotect()", + rcc_mode2_sender_rejects_unprotect }, + { "rcc_mode1_sender_rejects_unprotect()", + rcc_mode1_sender_rejects_unprotect }, #ifdef GCM { "rcc_gcm_mode2_rejected_at_create()", rcc_gcm_mode2_rejected_at_create }, { "rcc_gcm_mode3_basic_roundtrip()", rcc_gcm_mode3_basic_roundtrip }, @@ -1014,6 +1083,8 @@ TEST_LIST = { rcc_gcm_mode3_wildcard_inbound_late_join }, { "rcc_gcm_mode3_carry_replay_rejected()", rcc_gcm_mode3_carry_replay_rejected }, + { "rcc_gcm_mode3_sender_rejects_unprotect()", + rcc_gcm_mode3_sender_rejects_unprotect }, #endif { 0 } };