From 343c32897462828b43af76af7b242a89a9581dbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20B=C3=BChler?= Date: Wed, 27 Dec 2023 19:09:51 +0100 Subject: [PATCH 1/3] fix srtp_remove_stream to use SSRC in host byte order This fixes the inconsistency in SSRC byte order usage in the public api. Now all usage should be in host byte order. #220 --- include/srtp.h | 2 +- srtp/srtp.c | 2 +- test/srtp_driver.c | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/srtp.h b/include/srtp.h index 40b0c783f..9cf943f97 100644 --- a/include/srtp.h +++ b/include/srtp.h @@ -621,7 +621,7 @@ srtp_err_status_t srtp_add_stream(srtp_t session, const srtp_policy_t *policy); * will be removed. * * @param ssrc is the SSRC value of the stream to be removed - * in network byte order. + * in host byte order. * * @warning Wildcard SSRC values cannot be removed from a * session. diff --git a/srtp/srtp.c b/srtp/srtp.c index 82f4a8303..6510f6153 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -3050,7 +3050,7 @@ srtp_err_status_t srtp_remove_stream(srtp_t session, uint32_t ssrc) return srtp_err_status_bad_param; /* find and remove stream from the list */ - stream = srtp_stream_list_get(session->stream_list, ssrc); + stream = srtp_stream_list_get(session->stream_list, htonl(ssrc)); if (stream == NULL) { return srtp_err_status_no_ctx; } diff --git a/test/srtp_driver.c b/test/srtp_driver.c index 7307e1629..e4afa0ddf 100644 --- a/test/srtp_driver.c +++ b/test/srtp_driver.c @@ -2818,7 +2818,7 @@ srtp_err_status_t srtp_test_remove_stream(void) * check for false positives by trying to remove a stream that's not * in the session */ - status = srtp_remove_stream(session, htonl(0xaaaaaaaa)); + status = srtp_remove_stream(session, 0xaaaaaaaa); if (status != srtp_err_status_no_ctx) { return srtp_err_status_fail; } @@ -2827,7 +2827,7 @@ srtp_err_status_t srtp_test_remove_stream(void) * check for false negatives by removing stream 0x1, then * searching for streams 0x0 and 0x2 */ - status = srtp_remove_stream(session, htonl(0x1)); + status = srtp_remove_stream(session, 0x1); if (status != srtp_err_status_ok) { return srtp_err_status_fail; } @@ -2872,7 +2872,7 @@ srtp_err_status_t srtp_test_remove_stream(void) return status; } - status = srtp_remove_stream(session, htonl(0xcafebabe)); + status = srtp_remove_stream(session, 0xcafebabe); if (status != srtp_err_status_ok) { return status; } From 89011e4826058bb2236f350f2021fdcd5bc67fd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20B=C3=BChler?= Date: Thu, 28 Dec 2023 07:56:03 +0100 Subject: [PATCH 2/3] add attention to highlight param change --- include/srtp.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/include/srtp.h b/include/srtp.h index 9cf943f97..ab43e229d 100644 --- a/include/srtp.h +++ b/include/srtp.h @@ -618,13 +618,17 @@ srtp_err_status_t srtp_add_stream(srtp_t session, const srtp_policy_t *policy); * context given by the argument session. * * @param session is the SRTP session from which the stream - * will be removed. + * will be removed. * * @param ssrc is the SSRC value of the stream to be removed - * in host byte order. + * in host byte order. + * + * @attention In libSRTP version before 3.0.0 the SSRC param was in network + * byte order, this was changed in 3.0.0 to host byte order to be + * consistant with the rest of the api. * * @warning Wildcard SSRC values cannot be removed from a - * session. + * session. * * @return * - srtp_err_status_ok if the stream deallocation succeeded. From af78e623d8c76ac40ce11beb3ff1b0549beb6336 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pascal=20B=C3=BChler?= Date: Sat, 30 Dec 2023 09:35:27 +0100 Subject: [PATCH 3/3] rename stream api to be srtp_stream_xxx This is both part of #672 as well as trying to ensure that the parameter change for remove stream is noticed. --- fuzzer/fuzzer.c | 6 +++--- include/srtp.h | 30 +++++++++++++++--------------- srtp.def | 10 +++++----- srtp/srtp.c | 24 ++++++++++++------------ test/rtp_decoder.c | 2 +- test/srtp_driver.c | 40 ++++++++++++++++++++-------------------- 6 files changed, 56 insertions(+), 56 deletions(-) diff --git a/fuzzer/fuzzer.c b/fuzzer/fuzzer.c index 9c71ecfae..51187e3d5 100644 --- a/fuzzer/fuzzer.c +++ b/fuzzer/fuzzer.c @@ -849,7 +849,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) /* Keep removing streams until the set of SSRCs extracted from the * fuzzer input is exhausted */ if (i < num_remove_stream) { - if (srtp_remove_stream(srtp_ctx, remove_stream_ssrc[i]) != + if (srtp_stream_remove(srtp_ctx, remove_stream_ssrc[i]) != srtp_err_status_ok) { goto end; } @@ -860,11 +860,11 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) * extracted from the fuzzer input is exhausted */ if (j < num_set_roc * 2) { uint32_t roc; - if (srtp_set_stream_roc(srtp_ctx, set_roc[j], set_roc[j + 1]) != + if (srtp_stream_set_roc(srtp_ctx, set_roc[j], set_roc[j + 1]) != srtp_err_status_ok) { goto end; } - if (srtp_get_stream_roc(srtp_ctx, set_roc[j + 1], &roc) != + if (srtp_stream_get_roc(srtp_ctx, set_roc[j + 1], &roc) != srtp_err_status_ok) { goto end; } diff --git a/include/srtp.h b/include/srtp.h index ab43e229d..2d1d088ed 100644 --- a/include/srtp.h +++ b/include/srtp.h @@ -584,7 +584,7 @@ srtp_err_status_t srtp_unprotect_mki(srtp_t ctx, * for the session. The struct may be a single element, or it may be * the head of a list, in which case each element of the list is * processed. It may also be NULL, in which case streams should be added - * later using srtp_add_stream(). The final element of the list @b must + * later using srtp_stream_add(). The final element of the list @b must * have its `next' field set to NULL. * * @return @@ -595,10 +595,10 @@ srtp_err_status_t srtp_unprotect_mki(srtp_t ctx, srtp_err_status_t srtp_create(srtp_t *session, const srtp_policy_t *policy); /** - * @brief srtp_add_stream() allocates and initializes an SRTP stream + * @brief srtp_stream_add() allocates and initializes an SRTP stream * within a given SRTP session. * - * The function call srtp_add_stream(session, policy) allocates and + * The function call srtp_stream_add(session, policy) allocates and * initializes a new SRTP stream within a given, previously created * session, applying the policy given as the other argument to that * stream. @@ -608,12 +608,12 @@ srtp_err_status_t srtp_create(srtp_t *session, const srtp_policy_t *policy); * - srtp_err_status_alloc_fail if stream allocation failed * - srtp_err_status_init_fail if stream initialization failed. */ -srtp_err_status_t srtp_add_stream(srtp_t session, const srtp_policy_t *policy); +srtp_err_status_t srtp_stream_add(srtp_t session, const srtp_policy_t *policy); /** - * @brief srtp_remove_stream() deallocates an SRTP stream. + * @brief srtp_stream_remove() deallocates an SRTP stream. * - * The function call srtp_remove_stream(session, ssrc) removes + * The function call srtp_stream_remove(session, ssrc) removes * the SRTP stream with the SSRC value ssrc from the SRTP session * context given by the argument session. * @@ -635,7 +635,7 @@ srtp_err_status_t srtp_add_stream(srtp_t session, const srtp_policy_t *policy); * - [other] otherwise. * */ -srtp_err_status_t srtp_remove_stream(srtp_t session, unsigned int ssrc); +srtp_err_status_t srtp_stream_remove(srtp_t session, unsigned int ssrc); /** * @brief srtp_update() updates all streams in the session. @@ -664,9 +664,9 @@ srtp_err_status_t srtp_remove_stream(srtp_t session, unsigned int ssrc); srtp_err_status_t srtp_update(srtp_t session, const srtp_policy_t *policy); /** - * @brief srtp_update_stream() updates a SRTP stream. + * @brief srtp_stream_update() updates a SRTP stream. * - * The function call srtp_update_stream(session, policy) updates + * The function call srtp_stream_update(session, policy) updates * the stream(s) in the session that match applying the given * policy and key. The existing ROC value of all stream(s) will * be preserved. @@ -684,7 +684,7 @@ srtp_err_status_t srtp_update(srtp_t session, const srtp_policy_t *policy); * - [other] otherwise. * */ -srtp_err_status_t srtp_update_stream(srtp_t session, +srtp_err_status_t srtp_stream_update(srtp_t session, const srtp_policy_t *policy); /** @@ -1562,7 +1562,7 @@ void *srtp_get_user_data(srtp_t ctx); * reached, an SRTP stream will enter an `expired' state in which no * more packets can be protected or unprotected. When this happens, * it is likely that you will want to either deallocate the stream - * (using srtp_remove_stream()), and possibly allocate a new one. + * (using srtp_stream_remove()), and possibly allocate a new one. * * When an SRTP stream expires, the other streams in the same session * are unaffected, unless key sharing is used by that stream. In the @@ -1725,7 +1725,7 @@ srtp_err_status_t srtp_get_protect_rtcp_trailer_length(srtp_t session, uint32_t *length); /** - * @brief srtp_set_stream_roc(session, ssrc, roc) + * @brief srtp_stream_set_roc(session, ssrc, roc) * * Set the roll-over-counter on a session for a given SSRC * @@ -1733,12 +1733,12 @@ srtp_err_status_t srtp_get_protect_rtcp_trailer_length(srtp_t session, * stream found * */ -srtp_err_status_t srtp_set_stream_roc(srtp_t session, +srtp_err_status_t srtp_stream_set_roc(srtp_t session, uint32_t ssrc, uint32_t roc); /** - * @brief srtp_get_stream_roc(session, ssrc, roc) + * @brief srtp_stream_get_roc(session, ssrc, roc) * * Get the roll-over-counter on a session for a given SSRC * @@ -1746,7 +1746,7 @@ srtp_err_status_t srtp_set_stream_roc(srtp_t session, * stream found * */ -srtp_err_status_t srtp_get_stream_roc(srtp_t session, +srtp_err_status_t srtp_stream_get_roc(srtp_t session, uint32_t ssrc, uint32_t *roc); diff --git a/srtp.def b/srtp.def index c9e95c5b9..37953243c 100644 --- a/srtp.def +++ b/srtp.def @@ -6,10 +6,10 @@ srtp_protect_mki srtp_unprotect srtp_unprotect_mki srtp_create -srtp_add_stream -srtp_remove_stream +srtp_stream_add +srtp_stream_remove srtp_update -srtp_update_stream +srtp_stream_update srtp_get_stream srtp_crypto_policy_set_rtp_default srtp_crypto_policy_set_rtcp_default @@ -41,9 +41,9 @@ srtp_protect_rtcp srtp_protect_rtcp_mki srtp_unprotect_rtcp srtp_unprotect_rtcp_mki -srtp_set_stream_roc +srtp_stream_set_roc srtp_set_user_data -srtp_get_stream_roc +srtp_stream_get_roc srtp_get_user_data srtp_install_event_handler srtp_get_version_string diff --git a/srtp/srtp.c b/srtp/srtp.c index 6510f6153..16112195a 100644 --- a/srtp/srtp.c +++ b/srtp/srtp.c @@ -2917,7 +2917,7 @@ srtp_err_status_t srtp_dealloc(srtp_t session) return srtp_err_status_ok; } -srtp_err_status_t srtp_add_stream(srtp_t session, const srtp_policy_t *policy) +srtp_err_status_t srtp_stream_add(srtp_t session, const srtp_policy_t *policy) { srtp_err_status_t status; srtp_stream_t tmp; @@ -3025,7 +3025,7 @@ srtp_err_status_t srtp_create(srtp_t *session, /* handle for session */ * initializing a stream for each element */ while (policy != NULL) { - stat = srtp_add_stream(ctx, policy); + stat = srtp_stream_add(ctx, policy); if (stat) { /* clean up everything */ srtp_dealloc(*session); @@ -3040,7 +3040,7 @@ srtp_err_status_t srtp_create(srtp_t *session, /* handle for session */ return srtp_err_status_ok; } -srtp_err_status_t srtp_remove_stream(srtp_t session, uint32_t ssrc) +srtp_err_status_t srtp_stream_remove(srtp_t session, uint32_t ssrc) { srtp_stream_ctx_t *stream; srtp_err_status_t status; @@ -3081,7 +3081,7 @@ srtp_err_status_t srtp_update(srtp_t session, const srtp_policy_t *policy) } while (policy != NULL) { - stat = srtp_update_stream(session, policy); + stat = srtp_stream_update(session, policy); if (stat) { return stat; } @@ -3125,7 +3125,7 @@ static int update_template_stream_cb(srtp_stream_t stream, void *raw_data) old_rtcp_rdb = stream->rtcp_rdb; /* remove stream */ - data->status = srtp_remove_stream(session, ssrc); + data->status = srtp_stream_remove(session, ssrc); if (data->status) { return 1; } @@ -3212,7 +3212,7 @@ static srtp_err_status_t update_template_streams(srtp_t session, return srtp_err_status_ok; } -static srtp_err_status_t update_stream(srtp_t session, +static srtp_err_status_t stream_update(srtp_t session, const srtp_policy_t *policy) { srtp_err_status_t status; @@ -3234,12 +3234,12 @@ static srtp_err_status_t update_stream(srtp_t session, old_index = stream->rtp_rdbx.index; old_rtcp_rdb = stream->rtcp_rdb; - status = srtp_remove_stream(session, htonl(policy->ssrc.value)); + status = srtp_stream_remove(session, htonl(policy->ssrc.value)); if (status) { return status; } - status = srtp_add_stream(session, policy); + status = srtp_stream_add(session, policy); if (status) { return status; } @@ -3256,7 +3256,7 @@ static srtp_err_status_t update_stream(srtp_t session, return srtp_err_status_ok; } -srtp_err_status_t srtp_update_stream(srtp_t session, +srtp_err_status_t srtp_stream_update(srtp_t session, const srtp_policy_t *policy) { srtp_err_status_t status; @@ -3277,7 +3277,7 @@ srtp_err_status_t srtp_update_stream(srtp_t session, status = update_template_streams(session, policy); break; case (ssrc_specific): - status = update_stream(session, policy); + status = stream_update(session, policy); break; case (ssrc_undefined): default: @@ -4817,7 +4817,7 @@ srtp_err_status_t srtp_install_log_handler(srtp_log_handler_func_t func, return srtp_err_status_ok; } -srtp_err_status_t srtp_set_stream_roc(srtp_t session, +srtp_err_status_t srtp_stream_set_roc(srtp_t session, uint32_t ssrc, uint32_t roc) { @@ -4832,7 +4832,7 @@ srtp_err_status_t srtp_set_stream_roc(srtp_t session, return srtp_err_status_ok; } -srtp_err_status_t srtp_get_stream_roc(srtp_t session, +srtp_err_status_t srtp_stream_get_roc(srtp_t session, uint32_t ssrc, uint32_t *roc) { diff --git a/test/rtp_decoder.c b/test/rtp_decoder.c index c5d052b82..f4651393f 100644 --- a/test/rtp_decoder.c +++ b/test/rtp_decoder.c @@ -704,7 +704,7 @@ int rtp_decoder_init(rtp_decoder_t dcdr, } if (policy.ssrc.type == ssrc_specific && roc != 0) { - if (srtp_set_stream_roc(dcdr->srtp_ctx, policy.ssrc.value, roc)) { + if (srtp_stream_set_roc(dcdr->srtp_ctx, policy.ssrc.value, roc)) { return 1; } } diff --git a/test/srtp_driver.c b/test/srtp_driver.c index e4afa0ddf..4e601efc1 100644 --- a/test/srtp_driver.c +++ b/test/srtp_driver.c @@ -534,9 +534,9 @@ int main(int argc, char *argv[]) #endif /* - * test the function srtp_remove_stream() + * test the function srtp_stream_remove() */ - printf("testing srtp_remove_stream()..."); + printf("testing srtp_stream_remove()..."); if (srtp_test_remove_stream() == srtp_err_status_ok) { printf("passed\n"); } else { @@ -2818,7 +2818,7 @@ srtp_err_status_t srtp_test_remove_stream(void) * check for false positives by trying to remove a stream that's not * in the session */ - status = srtp_remove_stream(session, 0xaaaaaaaa); + status = srtp_stream_remove(session, 0xaaaaaaaa); if (status != srtp_err_status_no_ctx) { return srtp_err_status_fail; } @@ -2827,7 +2827,7 @@ srtp_err_status_t srtp_test_remove_stream(void) * check for false negatives by removing stream 0x1, then * searching for streams 0x0 and 0x2 */ - status = srtp_remove_stream(session, 0x1); + status = srtp_stream_remove(session, 0x1); if (status != srtp_err_status_ok) { return srtp_err_status_fail; } @@ -2867,12 +2867,12 @@ srtp_err_status_t srtp_test_remove_stream(void) return status; } - status = srtp_add_stream(session, &policy); + status = srtp_stream_add(session, &policy); if (status != srtp_err_status_ok) { return status; } - status = srtp_remove_stream(session, 0xcafebabe); + status = srtp_stream_remove(session, 0xcafebabe); if (status != srtp_err_status_ok) { return status; } @@ -3327,7 +3327,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) if (status) { return status; } - status = srtp_get_stream_roc(sender_session, sender_policy.ssrc.value, + status = srtp_stream_get_roc(sender_session, sender_policy.ssrc.value, &stream_roc); if (status) { return status; @@ -3342,7 +3342,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) if (status) { return status; } - status = srtp_get_stream_roc(sender_session, sender_policy.ssrc.value, + status = srtp_stream_get_roc(sender_session, sender_policy.ssrc.value, &stream_roc); if (status) { return status; @@ -3357,7 +3357,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) if (status) { return status; } - status = srtp_get_stream_roc(sender_session, sender_policy.ssrc.value, + status = srtp_stream_get_roc(sender_session, sender_policy.ssrc.value, &stream_roc); if (status) { return status; @@ -3372,7 +3372,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) if (status) { return status; } - status = srtp_get_stream_roc(sender_session, sender_policy.ssrc.value, + status = srtp_stream_get_roc(sender_session, sender_policy.ssrc.value, &stream_roc); if (status) { return status; @@ -3387,7 +3387,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) if (status) { return status; } - status = srtp_get_stream_roc(sender_session, sender_policy.ssrc.value, + status = srtp_stream_get_roc(sender_session, sender_policy.ssrc.value, &stream_roc); if (status) { return status; @@ -3402,7 +3402,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) if (status) { return status; } - status = srtp_get_stream_roc(receiver_session, receiver_policy.ssrc.value, + status = srtp_stream_get_roc(receiver_session, receiver_policy.ssrc.value, &stream_roc); if (status) { return status; @@ -3415,7 +3415,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) if (status) { return status; } - status = srtp_get_stream_roc(receiver_session, receiver_policy.ssrc.value, + status = srtp_stream_get_roc(receiver_session, receiver_policy.ssrc.value, &stream_roc); if (status) { return status; @@ -3428,7 +3428,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) if (status) { return status; } - status = srtp_get_stream_roc(receiver_session, receiver_policy.ssrc.value, + status = srtp_stream_get_roc(receiver_session, receiver_policy.ssrc.value, &stream_roc); if (status) { return status; @@ -3441,7 +3441,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) if (status) { return status; } - status = srtp_get_stream_roc(receiver_session, receiver_policy.ssrc.value, + status = srtp_stream_get_roc(receiver_session, receiver_policy.ssrc.value, &stream_roc); if (status) { return status; @@ -3454,7 +3454,7 @@ srtp_err_status_t srtp_test_out_of_order_after_rollover(void) if (status) { return status; } - status = srtp_get_stream_roc(receiver_session, receiver_policy.ssrc.value, + status = srtp_stream_get_roc(receiver_session, receiver_policy.ssrc.value, &stream_roc); if (status) { return status; @@ -3523,7 +3523,7 @@ srtp_err_status_t srtp_test_get_roc(void) return status; } - status = srtp_get_stream_roc(session, policy.ssrc.value, &roc); + status = srtp_stream_get_roc(session, policy.ssrc.value, &roc); if (status) { return status; } @@ -3672,7 +3672,7 @@ static srtp_err_status_t test_set_receiver_roc(uint32_t packets, memcpy(recv_pkt_2, pkt_2, protected_msg_len_octets_2); /* Set the ROC to the wanted value */ - status = srtp_set_stream_roc(receiver_session, receiver_policy.ssrc.value, + status = srtp_stream_set_roc(receiver_session, receiver_policy.ssrc.value, roc_to_set); if (status) { return status; @@ -3750,7 +3750,7 @@ static srtp_err_status_t test_set_sender_roc(uint16_t seq, uint32_t roc_to_set) } /* Set the ROC before encrypting the first packet */ - status = srtp_set_stream_roc(sender_session, sender_policy.ssrc.value, + status = srtp_stream_set_roc(sender_session, sender_policy.ssrc.value, roc_to_set); if (status != srtp_err_status_ok) { return status; @@ -3794,7 +3794,7 @@ static srtp_err_status_t test_set_sender_roc(uint16_t seq, uint32_t roc_to_set) memcpy(recv_pkt, pkt, protected_msg_len_octets); /* Set the ROC to the wanted value */ - status = srtp_set_stream_roc(receiver_session, receiver_policy.ssrc.value, + status = srtp_stream_set_roc(receiver_session, receiver_policy.ssrc.value, roc_to_set); if (status) { return status;