From 609f5798341a65e161b51a848a737689036103f7 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Fri, 27 Sep 2024 15:26:23 -0700 Subject: [PATCH 1/2] =?UTF-8?q?add=20test=20for=20ROC=20m=D1=96smatch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit illustrating the reason why one should carefully pick the initial sequence number low enough to avoid a rollover. See https://webrtc-review.googlesource.com/c/src/+/358360 --- format.sh | 2 +- test/srtp_driver.c | 108 +++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/format.sh b/format.sh index b67828c93..cc8bb90ff 100755 --- a/format.sh +++ b/format.sh @@ -20,7 +20,7 @@ done m=`git ls-files -m` if [ -n "$m" ]; then v=`$CLANG_FORMAT -version` - echo "Fromatting required when checking with $v" + echo "Formatting required when checking with $v" echo echo "The following files required formatting:" for f in $m; do diff --git a/test/srtp_driver.c b/test/srtp_driver.c index 04f8ca964..519c1e3fb 100644 --- a/test/srtp_driver.c +++ b/test/srtp_driver.c @@ -104,6 +104,8 @@ srtp_err_status_t srtp_test_get_roc(void); srtp_err_status_t srtp_test_set_receiver_roc(void); +srtp_err_status_t srtp_test_roc_mismatch(void); + srtp_err_status_t srtp_test_set_sender_roc(void); double srtp_bits_per_second(size_t msg_len_octets, const srtp_policy_t *policy); @@ -841,6 +843,14 @@ int main(int argc, char *argv[]) exit(1); } + printf("testing srtp_test_roc_mismatch()..."); + if (srtp_test_roc_mismatch() == srtp_err_status_ok) { + printf("passed\n"); + } else { + printf("failed\n"); + exit(1); + } + printf("testing srtp_test_set_sender_roc()..."); if (srtp_test_set_sender_roc() == srtp_err_status_ok) { printf("passed\n"); @@ -4021,7 +4031,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) uint32_t i; uint32_t stream_roc; - /* Create sender */ + /* Create the sender */ memset(&sender_policy, 0, sizeof(sender_policy)); #ifdef GCM srtp_crypto_policy_set_aes_gcm_128_16_auth(&sender_policy.rtp); @@ -4311,7 +4321,7 @@ static srtp_err_status_t test_set_receiver_roc(uint32_t packets, size_t protected_msg_len_octets_1; size_t protected_msg_len_octets_2; - /* Create sender */ + /* Create the sender */ memset(&sender_policy, 0, sizeof(sender_policy)); #ifdef GCM srtp_crypto_policy_set_aes_gcm_128_16_auth(&sender_policy.rtp); @@ -4472,7 +4482,7 @@ static srtp_err_status_t test_set_sender_roc(uint16_t seq, uint32_t roc_to_set) size_t msg_len_octets = 32; size_t protected_msg_len_octets; - /* Create sender */ + /* Create the sender */ memset(&sender_policy, 0, sizeof(sender_policy)); #ifdef GCM srtp_crypto_policy_set_aes_gcm_128_16_auth(&sender_policy.rtp); @@ -4670,6 +4680,98 @@ srtp_err_status_t srtp_test_set_sender_roc(void) return srtp_err_status_ok; } +/* This test illustrates how the ROC can be mismatched between + * sender and receiver when a packets are lost before the initial + * sequence number wraparound. In a nutshell: + * - Sender encrypts sequence numbers 65535, 0, 1, ... + * - Receiver only receives 1 initially. + * In that state the receiver will assume the sender used ROC=0 to + * encrypt the packet with sequence number 0. + * This is a long-standing problem that is best avoided by a choice + * of initial sequence number in the lower half of the sequence number + * space. + */ +srtp_err_status_t srtp_test_roc_mismatch(void) +{ + srtp_policy_t sender_policy; + srtp_t sender_session; + + srtp_policy_t receiver_policy; + srtp_t receiver_session; + + const uint32_t num_pkts = 3; + uint8_t *pkts[3]; + size_t pkt_len_octets[3]; + + const uint16_t seq = 0xffff; + uint32_t i; + + /* Create the sender */ + memset(&sender_policy, 0, sizeof(sender_policy)); +#ifdef GCM + srtp_crypto_policy_set_aes_gcm_128_16_auth(&sender_policy.rtp); + srtp_crypto_policy_set_aes_gcm_128_16_auth(&sender_policy.rtcp); + sender_policy.key = test_key_gcm; +#else + srtp_crypto_policy_set_rtp_default(&sender_policy.rtp); + srtp_crypto_policy_set_rtcp_default(&sender_policy.rtcp); + sender_policy.key = test_key; +#endif + sender_policy.ssrc.type = ssrc_specific; + sender_policy.ssrc.value = 0xcafebabe; + sender_policy.window_size = 128; + + CHECK_OK(srtp_create(&sender_session, &sender_policy)); + + /* Create the receiver */ + memset(&receiver_policy, 0, sizeof(receiver_policy)); +#ifdef GCM + srtp_crypto_policy_set_aes_gcm_128_16_auth(&receiver_policy.rtp); + srtp_crypto_policy_set_aes_gcm_128_16_auth(&receiver_policy.rtcp); + receiver_policy.key = test_key_gcm; +#else + srtp_crypto_policy_set_rtp_default(&receiver_policy.rtp); + srtp_crypto_policy_set_rtcp_default(&receiver_policy.rtcp); + receiver_policy.key = test_key; +#endif + receiver_policy.ssrc.type = ssrc_specific; + receiver_policy.ssrc.value = sender_policy.ssrc.value; + receiver_policy.window_size = 128; + + CHECK_OK(srtp_create(&receiver_session, &receiver_policy)); + + /* Create and protect packets to get to get ROC == 1 */ + for (i = 0; i < num_pkts; i++) { + pkts[i] = create_rtp_test_packet(64, sender_policy.ssrc.value, + (uint16_t)(seq + i), 0, false, + &pkt_len_octets[i], NULL); + CHECK_OK( + call_srtp_protect(sender_session, pkts[i], &pkt_len_octets[i], 0)); + } + + /* Decrypt in reverse order (1, 0, 65535) */ + CHECK_RETURN( + call_srtp_unprotect(receiver_session, pkts[2], &pkt_len_octets[2]), + srtp_err_status_auth_fail); + CHECK_RETURN( + call_srtp_unprotect(receiver_session, pkts[1], &pkt_len_octets[1]), + srtp_err_status_auth_fail); + CHECK_OK( + call_srtp_unprotect(receiver_session, pkts[0], &pkt_len_octets[0])); + /* After decryption of the previous ROC rollover will work as expected */ + CHECK_OK( + call_srtp_unprotect(receiver_session, pkts[1], &pkt_len_octets[1])); + CHECK_OK( + call_srtp_unprotect(receiver_session, pkts[2], &pkt_len_octets[2])); + + for (i = 0; i < num_pkts; i++) { + free(pkts[i]); + } + CHECK_OK(srtp_dealloc(sender_session)); + CHECK_OK(srtp_dealloc(receiver_session)); + return srtp_err_status_ok; +} + /* * srtp policy definitions - these definitions are used above */ From ba6a14f409de8d23edb1571f80d409a6c46b8b7b Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Sun, 29 Sep 2024 19:49:02 -0700 Subject: [PATCH 2/2] tweak test to avoid failure --- test/srtp_driver.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/srtp_driver.c b/test/srtp_driver.c index 519c1e3fb..8012c0bf7 100644 --- a/test/srtp_driver.c +++ b/test/srtp_driver.c @@ -4749,20 +4749,17 @@ srtp_err_status_t srtp_test_roc_mismatch(void) call_srtp_protect(sender_session, pkts[i], &pkt_len_octets[i], 0)); } - /* Decrypt in reverse order (1, 0, 65535) */ + /* Decrypt in reverse order (1, 65535) */ CHECK_RETURN( call_srtp_unprotect(receiver_session, pkts[2], &pkt_len_octets[2]), srtp_err_status_auth_fail); - CHECK_RETURN( - call_srtp_unprotect(receiver_session, pkts[1], &pkt_len_octets[1]), - srtp_err_status_auth_fail); CHECK_OK( call_srtp_unprotect(receiver_session, pkts[0], &pkt_len_octets[0])); /* After decryption of the previous ROC rollover will work as expected */ + /* Only pkts[1] is checked since that is not modified by the attempt to + * decryption. */ CHECK_OK( call_srtp_unprotect(receiver_session, pkts[1], &pkt_len_octets[1])); - CHECK_OK( - call_srtp_unprotect(receiver_session, pkts[2], &pkt_len_octets[2])); for (i = 0; i < num_pkts; i++) { free(pkts[i]);