backport cryptex to v2 branch by pabuhler · Pull Request #778 · cisco/libsrtp

pabuhler · 2025-11-23T21:44:28Z

This backports cryptex support added in #551 (76f23aa). The tests are nearly unchanged but the code was slightly simplified as non-in-place io is not supported in the v2 branch.

#777

pabuhler · 2025-11-23T21:49:08Z

Unfortunately this adds a new "use_cryptex" member to the srtp_policy_t struct, breaking binary compatibility.
This should maybe be changed to a setter function like srtp_set_cryptex_enabled() that should called after srtp_create() but before any data has been handled.

bifurcation

A few minor / aesthetic comments, which would probably have been better on the original Cryptex PR. If you agree, maybe let's implement them here, and then forward-port to main.

Also, looks like fuzzing CI is reliably failing (Copilot says it's due to an extra -- argument). Filed #780

bifurcation · 2025-12-02T13:04:30Z

include/srtp.h

-    srtp_err_status_pkt_idx_adv = 27    /**< packet index advanced, reset    */
+    srtp_err_status_pkt_idx_adv = 27,   /**< packet index advanced, reset    */
                                        /**< needed                          */
+    srtp_err_status_cryptex_err = 28    /**< cryptex error                   */


Since this is only used in once place, let's document more clearly.

Suggested change

srtp_err_status_cryptex_err = 28 /**< cryptex error */

srtp_err_status_cryptex_err = 28 /**< cryptex with CSRC and no header */

I think it main it can be returned for different reasons but I am ok with this

bifurcation · 2025-12-02T13:05:28Z

include/srtp.h

+/**
+ * @brief srtp_set_stream_use_cryptex(session, ssrc)
+ *
+ * Enable cryptex processing for the stream identified by the given SSRC. For


I would reference RFC 9335 somewhere in this doc comment.

bifurcation · 2025-12-02T13:10:36Z

srtp/srtp.c

+    return ntohs(xtn_hdr->profile_specific);
+}
+
+static void srtp_cryptex_adjust_buffer(const srtp_hdr_t *hdr, uint8_t *rtp)


I would prefer a more descriptive name, but I can't think of something better. Maybe srtp_cryptex_join? And split instead of restore?

This is back port from main ... would prefer to keep it the same

bifurcation · 2025-12-02T13:19:46Z

srtp/srtp.c

+        uint8_t tmp[4];
+        uint8_t *ptr = rtp + srtp_get_rtp_hdr_len(hdr);
+        size_t cc_list_size = hdr->cc * 4;
+        memcpy(tmp, ptr, 4);
+        ptr -= cc_list_size;
+        memmove(ptr + 4, ptr, cc_list_size);
+        memcpy(ptr, tmp, 4);


It would read a little more clearly to me not to interleave the pointer operations and the memory operations. (Also note that the field being moved is csrc, not cc_list.)

Suggested change

uint8_t tmp[4];

uint8_t *ptr = rtp + srtp_get_rtp_hdr_len(hdr);

size_t cc_list_size = hdr->cc * 4;

memcpy(tmp, ptr, 4);

ptr -= cc_list_size;

memmove(ptr + 4, ptr, cc_list_size);

memcpy(ptr, tmp, 4);

uint8_t tmp[4];

uint8_t *xtn_hdr = rtp + srtp_get_rtp_hdr_len(hdr);

uint8_t *csrc = rtp + octets_in_rtp_header;

size_t csrc_size = hdr->cc * 4;

memcpy(tmp, xtn_hdr, 4);

memmove(csrc + 4, csrc, csrc_size);

memcpy(csrc, tmp, 4);

It is is list of csrc's not a single one.

bifurcation · 2025-12-02T13:21:48Z

srtp/srtp.c

+        uint8_t tmp[4];
+        uint8_t *ptr = rtp + octets_in_rtp_header;
+        size_t cc_list_size = hdr->cc * 4;
+        memcpy(tmp, ptr, 4);
+        memmove(ptr, ptr + 4, cc_list_size);
+        ptr += cc_list_size;
+        memcpy(ptr, tmp, 4);


As above.

Suggested change

uint8_t tmp[4];

uint8_t *ptr = rtp + octets_in_rtp_header;

size_t cc_list_size = hdr->cc * 4;

memcpy(tmp, ptr, 4);

memmove(ptr, ptr + 4, cc_list_size);

ptr += cc_list_size;

memcpy(ptr, tmp, 4);

uint8_t tmp[4];

uint8_t *xtn_hdr = rtp + srtp_get_rtp_hdr_len(hdr);

uint8_t *csrc = rtp + octets_in_rtp_header;

size_t csrc_size = hdr->cc * 4;

memcpy(tmp, csrc, 4);

memmove(csrc, csrc + 4, csrc_size);

memcpy(xtn_hdr, tmp, 4);

bifurcation · 2025-12-02T13:23:50Z

srtp/srtp.c

+    return srtp_err_status_ok;
+}
+
+static srtp_err_status_t srtp_cryptex_protect(srtp_hdr_t *hdr, uint8_t *rtp)


Suggested change

static srtp_err_status_t srtp_cryptex_protect(srtp_hdr_t *hdr, uint8_t *rtp)

static srtp_err_status_t srtp_cryptex_protect_init(srtp_hdr_t *hdr, uint8_t *rtp)

The pattern with unprotect is better, since this isn't doing any actual protection.

bifurcation · 2025-12-02T13:26:08Z

srtp/srtp.c

+        srtp_hdr_xtnd_t *xtn_hdr = srtp_get_rtp_xtn_hdr(hdr);
+        *enc_start -=
+            (srtp_get_rtp_xtn_hdr_len(xtn_hdr) - octets_in_rtp_xtn_hdr);
+        *enc_start -= (hdr->cc * 4);


Rather than walking back, wouldn't it be simpler just to say "fixed header plus four"?

Suggested change

srtp_hdr_xtnd_t *xtn_hdr = srtp_get_rtp_xtn_hdr(hdr);

*enc_start -=

(srtp_get_rtp_xtn_hdr_len(xtn_hdr) - octets_in_rtp_xtn_hdr);

*enc_start -= (hdr->cc * 4);

*enc_start = hdr + octets_in_rtp_xtn_hdr;

bifurcation · 2025-12-02T13:28:09Z

srtp/srtp.c

+        srtp_hdr_xtnd_t *xtn_hdr = srtp_get_rtp_xtn_hdr(hdr);
+        *enc_start -=
+            (srtp_get_rtp_xtn_hdr_len(xtn_hdr) - octets_in_rtp_xtn_hdr);
+        *enc_start -= (hdr->cc * 4);


Suggested change

srtp_hdr_xtnd_t *xtn_hdr = srtp_get_rtp_xtn_hdr(hdr);

*enc_start -=

(srtp_get_rtp_xtn_hdr_len(xtn_hdr) - octets_in_rtp_xtn_hdr);

*enc_start -= (hdr->cc * 4);

*enc_start = hdr + octets_in_rtp_xtn_hdr;

This backports cryptex support added in cisco#551 (76f23aa). The tests are nearly unchanged but the code was slightly simplified as non-in-place io is not supported in the v2 branch. cisco#777

pabuhler · 2026-01-16T18:32:18Z

Rebased to pick up CI fixes, will merge this in and then do some of the suggested updates in main first.

This applies some of the changes that where suggested when backporting cryptex to 2.x branch, #778 . Refer to header extension as hdr_xtnd in function names.

This applies some of the changes that where suggested when backporting cryptex to 2.x branch, cisco#778 . Refer to header extension as hdr_xtnd in function names. (cherry picked from commit f26401e)

pabuhler requested a review from bifurcation November 26, 2025 10:33

bifurcation approved these changes Dec 2, 2025

View reviewed changes

bifurcation mentioned this pull request Dec 2, 2025

Fix fuzzing CI #780

Closed

pabuhler added 2 commits January 16, 2026 19:31

backport cryptex to v2 branch

e510383

This backports cryptex support added in cisco#551 (76f23aa). The tests are nearly unchanged but the code was slightly simplified as non-in-place io is not supported in the v2 branch. cisco#777

Add cryptex setter API to avoid srtp_policy_t ABI change

a7d48bf

pabuhler force-pushed the backport-cryptex-v2 branch from b456da9 to a7d48bf Compare January 16, 2026 18:31

pabuhler merged commit bea7225 into cisco:2_x_dev Jan 16, 2026
33 checks passed

pabuhler added a commit that referenced this pull request Feb 4, 2026

apply some suggested changes to cryptex

f26401e

This applies some of the changes that where suggested when backporting cryptex to 2.x branch, #778 . Refer to header extension as hdr_xtnd in function names.

pabuhler mentioned this pull request Feb 4, 2026

apply some suggested changes to cryptex #788

Merged

pabuhler mentioned this pull request Feb 8, 2026

apply some suggested changes to cryptex #789

Merged

	srtp_err_status_cryptex_err = 28 /*< cryptex error /
	srtp_err_status_cryptex_err = 28 /*< cryptex with CSRC and no header /

	static srtp_err_status_t srtp_cryptex_protect(srtp_hdr_t hdr, uint8_t rtp)
	static srtp_err_status_t srtp_cryptex_protect_init(srtp_hdr_t hdr, uint8_t rtp)

Conversation

pabuhler commented Nov 23, 2025

Uh oh!

pabuhler commented Nov 23, 2025

Uh oh!

bifurcation left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pabuhler commented Jan 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bifurcation left a comment •

edited

Loading