Add support for cryptex #511

murillo128 · 2020-11-05T21:44:33Z

Implementation of cryptex as per https://github.com/juberti/cryptex

srtp/srtp.c

pabuhler · 2020-11-09T10:11:23Z

hi, it would be good to get the CI build passing as soon as possible, are you able to fix that or doe you need some help?

…cryptex

srtp/srtp.c

fluffy · 2020-12-27T16:03:17Z

Great to have this on a branch but I would like to see the IETF draft to get further along before we merge it to the main branch.

murillo128 · 2020-12-27T16:08:18Z

@fluffy I agree. My main intent was to get feedback on the changes and spot any issue on the implementation that could impact the spec.

Would be great if it could be thoroughly reviewed so we can confirm that the test vectors can be incorporated into the rfc and be ready to merge the branch as soon as the rfc is ready.

fluffy · 2021-07-26T23:13:53Z

Given the IETF draft is about at WGLC, we should merge this in once we are happy with the tests.

pabuhler · 2021-08-09T17:50:25Z

Given the IETF draft is about at WGLC, we should merge this in once we are happy with the tests.

@murillo128 are you able to fix the compile issues? Maybe rebase on master as well?

@paulej have you thought to review these changes ?

murillo128 · 2021-08-09T18:45:25Z

I'll do it later this month when I am back from vacations

paulej

Looks good. I appreciate that there is test code to go with the implementation, because I'll admit I didn't check every field to ensure the math is right.

paulej · 2021-08-09T18:33:11Z

srtp/srtp.c

+            /* Get CSRCs block position or profile if no CSRCs */
+            uint32_t *csrcs = (uint32_t *)hdr + uint32s_in_rtp_header;
+            /* Move CSRCS so block is contiguous with extension header block */
+            for (unsigned char i = hdr->cc; i > 0; --i)


Any reason to do this vs. memcpy() or memmove()? Loops are usually slower, but maybe this doesn't matter.

just thought it would be easier to review the change and check it was spec compliant, I can change it to memcpy or memmove if needed

backport changes from master

murillo128 · 2021-08-24T14:43:00Z

I have also rebased to master and fixed the CI errors of the previous version. Could anyone run the github workflows to check if everything is correct now?

murillo128 · 2021-08-29T10:58:10Z

fixed format and mem leak on deallocating recv sessions on tests

pabuhler · 2024-05-27T19:53:54Z

hmm, I see this made it to an RFC now, https://datatracker.ietf.org/doc/html/rfc9335 , this was not the case when this code was committed or originally reviewed. I am unsure if the current implementation follows the RFC or not, would need some input from @murillo128 on that.
Either way this code needs to be updated before it can be merge.
Do you have a use for this code?

hakarim740-com-ra · 2024-05-28T12:53:05Z

Mainly to be used with WebRTC

bifurcation

I would hold on merging this until a few more folks chime in saying they'll use it. Otherwise it's unnecessary complexity.

include/srtp.h

include/srtp_priv.h

srtp/srtp.c

murillo128 · 2024-05-28T22:57:49Z

If this gets merged and upstreamed by libwebrtc I will provide a patch for adding support to cryptex in libwebrtc

pabuhler · 2024-05-29T06:04:15Z

ok, I can update it to work on top of the v3 work if you like, it will give me a chance to better review the code.

include/srtp.h

include/srtp_priv.h

srtp/srtp.c

murillo128 · 2024-07-19T11:27:13Z

i have rebased the PR and addressed the comments. could we run the tests? not completly sure I did everything correctly

JonathanLennox · 2024-07-19T19:58:43Z

If you want to test interop, once you have the WebRTC patch, Jitsi Meet's Videobridge supports cryptex, and I should be able to configure an instance to negotiate it in our WebRTC client.

pabuhler · 2024-07-19T20:40:32Z

include/srtp_priv.h

@@ -125,6 +125,8 @@ typedef struct srtp_stream_ctx_t_ {
    uint8_t *enc_xtn_hdr;
    size_t enc_xtn_hdr_count;
    uint32_t pending_roc;
+    bool use_cryptex;
+    struct srtp_stream_ctx_t_ *next; /* linked list of streams */


next was removed, I guess this slipped in while merging ?

There have been a lot of changes in main, are you sure the merge went well ? All builds are failing.
Also the input buffer to the protect/unprotect functions is now const, not sure you should just cast that away and move the memory around.

i think I have been too optimistic about my rebasing 😅

Wil try to get a look next week

Yeah, I had a quick attempt a few weeks ago and understood that this was not an easy merge :( . Let me know if you want to discuss anything.

Code is merged but tests are disabled and code if #defed out. Starting point for porting after major code changes.

pabuhler · 2024-09-08T16:42:56Z

@murillo128 I have created a new PR #724 , that tries to merge your changes in to main. It takes a slightly different approach given the non in-place io support that is in main now.
With the new API the input buffer is always const, this means that modifying it in-place to create a continues memory block to encrypt is maybe not feasible. The new code will encrypt the csrc list first before encrypting the header extensions and payload in one block. The code also does not currently support encrypting csrc's in gcm mode as today in gcm mode there can only be one call to encrypt. So something needs to change if cryptex is to be supported.
The two options I see are:

Add support to call encrypt/decrypt multiple times in gcm mode, I think most backends can support this.
Change the input buffers from const to non const.

If option 1 is done then it could still be possible to modify the input to supportone call to encrypt when it is detected that this is in-place io.
I prefer not to remove the const from the input buffers but it is something that could be discussed. I am not sure of the performance penalty of call encrypt twice, it would probably depend alot on the backend.

murillo128 · 2024-09-10T07:28:04Z

@pabuhler thank you very much for taking the time to work on this!

Removing the const is not my preferred option too, would be great if the gcm apis could be updated to allow multiple calls.

On my experience, some backends (expecially OpenSSL 3.0) have a huge overhead when making multiple calls to the api, but we are already having that performance issue anyway: #645

pabuhler · 2024-09-10T08:36:41Z

@pabuhler thank you very much for taking the time to work on this!

Removing the const is not my preferred option too, would be great if the gcm apis could be updated to allow multiple calls.

On my experience, some backends (expecially OpenSSL 3.0) have a huge overhead when making multiple calls to the api, but we are already having that performance issue anyway: #645

OK, I will tryout gcm api supporting multiple calls to verify that it is possible. Then at least cryptex can be "functionaly" complete and can look at optimizations afterwards. It is not sure that the performance problems in #645 will be such and issue when making multiple calls to openssl if IV has not changed but time will tell.

pabuhler · 2024-09-10T13:04:14Z

I managed to get it to fully work with gcm and openssl, cryptex in this mode also requires multiple calls to srtp_cipher_set_aad but it seamed to work. Will look into the other backends.

pabuhler · 2024-11-13T00:53:14Z

@murillo128, I have updated the code in #724, I have abandoned the attempt to support calling encrypt/decrypt multiple times when using a gcm cipher as it is not well support by the different backends. Can look in to this again later.

So the current state is that if the io is in-place and there are CC's in the header then the input buffer will be rearranged similar to your original PR, this is supported by both icm and gcm. If there are CC's in the header and io is not in-place then there will multiple calls to encrypt or decrypt, this is only supported by icm. If there are no CC's in the header then the start offset is just updated and there is a single call to encrypt or decrypt, this is supported by both icm and gcm.

Short summary is if you want to use cryptex then you should use in-place io.

Do you think could merge that code in to this PR and it could get reviewed and hopefully merged?
There are a couple of failing CI builds in #724 which I will follow up on. I do not think they are cryptex specific so will try to reproduce and fix independently.

pabuhler · 2024-11-26T05:34:17Z

@murillo128 have you had a chance to look at my proposal? I am thinking to merge it in to your branch soon so it can be reviewed and tested.
@JonathanLennox I am not sure how to test with WebRTC, but if some one could provide a pcap and a key I could run some tests against that.

Move cryptex code to separate functions so it can be reused. Due to support for non in-place io it is not always possible to modify the input buffer in the way the cryptex draft expected therefore use multiple encrypt / decrypt calls when io is not in-place. The gcm ciphers do not currently support multiple operations so non in-place io with csrc's is not supported.

pabuhler · 2024-12-17T14:33:25Z

@murillo128 I have force pushed my changes to your branch, overwriting your recent merging with main attempt, I hope that is ok. I have made a local copy of your changes if you would like it reverted.

It would be could to get this reviewed again and then it can be merged. There are some limitations as described earlier but I think it is acceptable for now.

JonathanLennox · 2025-02-12T15:06:27Z

srtp/srtp.c

+    bool *inplace,
+    size_t *enc_start)
+{
+    if (stream->use_cryptex && stream->rtp_services & sec_serv_conf) {


I'd recommend putting the second clause in parentheses? I think this is probably correct but the relative precedence of logical and vs. bitwise and is confusing enough it'd be better not to risk it.

JonathanLennox · 2025-02-12T15:11:18Z

srtp/srtp.c

+    if (stream->use_cryptex && stream->rtp_services & sec_serv_conf) {
+        if (hdr->cc && hdr->x == 0) {
+            /* Cryptex can only encrypt CSRCS if header extension is present */
+            return srtp_err_status_parse_err;


I wonder if we should define a new status code rather than calling this a parse error.

Any suggestions?
Something very specific like
srtp_err_status_cryptex_rtp_header_extension_required
or a little more generic
srtp_err_status_cryptex_unsupported

JonathanLennox · 2025-02-12T15:19:37Z

test/srtp_driver.c

+    policy.use_cryptex = true;
+    policy.next = NULL;
+
+    for (size_t i = 0; i < 6; ++i) {


Can we define const int num_vectors = sizeof(vectors) / sizeof(vectors[0]) above and use that rather than hard-coding 6 here?

JonathanLennox · 2025-02-12T15:20:45Z

test/srtp_driver.c

+        CHECK_OK(srtp_dealloc(srtp_recv));
+    }
+
+    return srtp_err_status_ok;


Since you have separate code paths for in-place and non-in-place cryptex (which I don't love, is there any way we can avoid that?) we should make sure this test (and the GCM one) tests both cases.

If you run the srtp_driver test app with "-n" then it use the non-in-place api for all tests, not the best solution but it was how we could ensure all existing functionality was tested with both io options. In CMakeList.txt that is added as a separate test so it is run as part of the CI task in github.

JonathanLennox · 2025-02-12T15:21:55Z

srtp/srtp.c

+    if (cryptex_inuse && !cryptex_inplace && hdr->cc) {
+        debug_print0(mod_srtp,
+                     "unsupported cryptex mode, AEAD, CC and not inplace io");
+        return srtp_err_status_bad_param;


It seems like this ought to be supportable?

Oh, I guess the problem is that srtp_cipher_encrypt is a one-and-done API, not an incremental API.

This could be changed, but it'd be a big enough change that it probably shouldn't be part of this PR just for non-in-place cryptex-with-CC.

srtp_cipher_encrypt API works ok incrementally for normal AES but does not work for AES GCM. I made an attempt to turn it into an incremental API but had problems with the different back ends, some just did not support it or required it to be in AES block size increments :( . Like you say we can try again at some point after this PR so it does not mix things up to much.

JonathanLennox · 2025-02-12T15:22:42Z

srtp/srtp.c

+    if (stream->use_cryptex && stream->rtp_services & sec_serv_conf) {
+        if (hdr->cc && hdr->x == 0) {
+            /* Cryptex can only encrypt CSRCS if header extension is present */
+            return srtp_err_status_parse_err;


This should have a test case, to confirm that we do indeed fail, rather than silently sending packets unencrypted or something else bad.

murillo128 added 4 commits November 5, 2020 04:29

WIP

9063d2e

replace memmove by for loop

95b0e97

cryptex in aead

62bf7a8

fix csrcs type

4939886

dbrgn reviewed Nov 6, 2020

View reviewed changes

srtp/srtp.c Outdated Show resolved Hide resolved

cyrptex -> cryptex

e16a815

murillo128 added 4 commits November 9, 2020 22:55

fix test/srtp_driver structs

664da14

Merge branch 'cryptex' of https://github.com/murillo128/libsrtp into …

6fa8d3b

…cryptex

clang-format

3fe608b

remove tabs

696ae82

JonathanLennox reviewed Nov 13, 2020

View reviewed changes

srtp/srtp.c Outdated Show resolved Hide resolved

srtp/srtp.c Outdated Show resolved Hide resolved

srtp/srtp.c Outdated Show resolved Hide resolved

srtp/srtp.c Outdated Show resolved Hide resolved

murillo128 added 4 commits November 16, 2020 00:22

[wip]

4ab5255

Fix AAD and add tests

ce58dd6

format

6536373

clang

fbf23e2

paulej approved these changes Aug 9, 2021

View reviewed changes

murillo128 added 3 commits August 24, 2021 16:19

Merge pull request #1 from murillo128/master

f09a939

backport changes from master

fix warnings

3f21c3e

rename policy.ekt to policy.deprecated_ekt to aling with master

5124a97

murillo128 added 2 commits August 29, 2021 12:56

fix format

f8a374b

fix mem leak on deallocating recv sessions

90d2345

juberti mentioned this pull request Mar 7, 2022

Tracking bug for libsrtp update for cryptex juberti/cryptex#5

Open

bifurcation reviewed May 28, 2024

View reviewed changes

include/srtp.h Outdated Show resolved Hide resolved

include/srtp_priv.h Outdated Show resolved Hide resolved

srtp/srtp.c Outdated Show resolved Hide resolved

srtp/srtp.c Outdated Show resolved Hide resolved