backport cryptex to v2 branch #778
Conversation
|
Unfortunately this adds a new "use_cryptex" member to the srtp_policy_t struct, breaking binary compatibility. |
There was a problem hiding this comment.
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
| 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 */ |
There was a problem hiding this comment.
Since this is only used in once place, let's document more clearly.
| srtp_err_status_cryptex_err = 28 /**< cryptex error */ | |
| srtp_err_status_cryptex_err = 28 /**< cryptex with CSRC and no header */ |
There was a problem hiding this comment.
I think it main it can be returned for different reasons but I am ok with this
| /** | ||
| * @brief srtp_set_stream_use_cryptex(session, ssrc) | ||
| * | ||
| * Enable cryptex processing for the stream identified by the given SSRC. For |
There was a problem hiding this comment.
I would reference RFC 9335 somewhere in this doc comment.
| return ntohs(xtn_hdr->profile_specific); | ||
| } | ||
|
|
||
| static void srtp_cryptex_adjust_buffer(const srtp_hdr_t *hdr, uint8_t *rtp) |
There was a problem hiding this comment.
I would prefer a more descriptive name, but I can't think of something better. Maybe srtp_cryptex_join? And split instead of restore?
There was a problem hiding this comment.
This is back port from main ... would prefer to keep it the same
| 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); |
There was a problem hiding this comment.
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.)
| 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); |
There was a problem hiding this comment.
It is is list of csrc's not a single one.
| 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); |
There was a problem hiding this comment.
As above.
| 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); |
| return srtp_err_status_ok; | ||
| } | ||
|
|
||
| static srtp_err_status_t srtp_cryptex_protect(srtp_hdr_t *hdr, uint8_t *rtp) |
There was a problem hiding this comment.
| 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.
| 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); |
There was a problem hiding this comment.
Rather than walking back, wouldn't it be simpler just to say "fixed header plus four"?
| 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; |
| 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); |
There was a problem hiding this comment.
| 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; |
2 participants
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