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

fluffy · 2022-03-08T00:19:26Z

@bifurcation - Did you ever ger to look at this ?

bifurcation · 2022-03-08T14:00:49Z

Yes, that's what led to this: juberti/cryptex#31

…

On Mon, Mar 7, 2022 at 7:19 PM Cullen Jennings ***@***.***> wrote: @bifurcation <https://github.com/bifurcation> - Did you ever ger to look at this ? — Reply to this email directly, view it on GitHub <#511 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAASOTKHLJ6J64N74CA52CDU62MJVANCNFSM4TL45Z7A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

hakarim740-com-ra · 2024-05-24T14:12:54Z

When will this be merged?

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.

bifurcation · 2024-05-28T13:58:14Z

include/srtp.h

@@ -333,6 +333,8 @@ typedef struct srtp_policy_t {
    int *enc_xtn_hdr;              /**< List of header ids to encrypt.      */
    int enc_xtn_hdr_count;         /**< Number of entries in list of header */
                                   /**<  ids.                               */
+    unsigned int use_cryptex;      /**< Encrypt header block and CSRCS with */


Seems like this should be int for consistency with other flags, or bool to match the "v3" API.

bifurcation · 2024-05-28T13:58:27Z

include/srtp_priv.h

@@ -149,6 +149,7 @@ typedef struct srtp_stream_ctx_t_ {
    int *enc_xtn_hdr;
    int enc_xtn_hdr_count;
    uint32_t pending_roc;
+    unsigned int use_cryptex;


Same int/bool comment here.

bifurcation · 2024-05-28T14:00:22Z

srtp/srtp.c

+        for (unsigned char i = hdr->cc; i > 0; --i)
+            csrcs[i] = csrcs[i - 1];


{} around for blocks, please.

Or even better, memmove.

bifurcation · 2024-05-28T20:44:59Z

srtp/srtp.c

+        /* Change profiles by cryptex values */
+        if (xtn_profile_specific == 0xbede) {
+            xtn_hdr_profile_and_value = htonl(0xc0de << 16 | xtn_hdr_length);
+        } else if (xtn_profile_specific == 0x1000) {


This is incorrect. The low-order bits don't have to be zero.

cf. https://github.com/cisco/libsrtp/blob/main/srtp/srtp.c#L1638

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.

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 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for cryptex #511

Add support for cryptex #511

murillo128 commented Nov 5, 2020

pabuhler commented Nov 9, 2020

fluffy commented Dec 27, 2020

murillo128 commented Dec 27, 2020

fluffy commented Jul 26, 2021

pabuhler commented Aug 9, 2021

murillo128 commented Aug 9, 2021

paulej left a comment

paulej Aug 9, 2021 •

edited

murillo128 Aug 24, 2021

murillo128 commented Aug 24, 2021

murillo128 commented Aug 29, 2021

fluffy commented Mar 8, 2022

bifurcation commented Mar 8, 2022 via email

hakarim740-com-ra commented May 24, 2024

pabuhler commented May 27, 2024

hakarim740-com-ra commented May 28, 2024

bifurcation left a comment

bifurcation May 28, 2024

bifurcation May 28, 2024

bifurcation May 28, 2024

bifurcation May 28, 2024

bifurcation May 28, 2024

murillo128 commented May 28, 2024

pabuhler commented May 29, 2024

		for (unsigned char i = hdr->cc; i > 0; --i)
		csrcs[i] = csrcs[i - 1];

Add support for cryptex #511

Are you sure you want to change the base?

Add support for cryptex #511

Conversation

murillo128 commented Nov 5, 2020

pabuhler commented Nov 9, 2020

fluffy commented Dec 27, 2020

murillo128 commented Dec 27, 2020

fluffy commented Jul 26, 2021

pabuhler commented Aug 9, 2021

murillo128 commented Aug 9, 2021

paulej left a comment

Choose a reason for hiding this comment

paulej Aug 9, 2021 • edited

Choose a reason for hiding this comment

murillo128 Aug 24, 2021

Choose a reason for hiding this comment

murillo128 commented Aug 24, 2021

murillo128 commented Aug 29, 2021

fluffy commented Mar 8, 2022

bifurcation commented Mar 8, 2022 via email

hakarim740-com-ra commented May 24, 2024

pabuhler commented May 27, 2024

hakarim740-com-ra commented May 28, 2024

bifurcation left a comment

Choose a reason for hiding this comment

bifurcation May 28, 2024

Choose a reason for hiding this comment

bifurcation May 28, 2024

Choose a reason for hiding this comment

bifurcation May 28, 2024

Choose a reason for hiding this comment

bifurcation May 28, 2024

Choose a reason for hiding this comment

bifurcation May 28, 2024

Choose a reason for hiding this comment

murillo128 commented May 28, 2024

pabuhler commented May 29, 2024

paulej Aug 9, 2021 •

edited