Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking bug for libsrtp update for cryptex #5

Open
juberti opened this issue Jul 30, 2020 · 8 comments
Open

Tracking bug for libsrtp update for cryptex #5

juberti opened this issue Jul 30, 2020 · 8 comments

Comments

@juberti
Copy link
Owner

juberti commented Jul 30, 2020

@murillo128 has volunteered

@murillo128
Copy link
Collaborator

@murillo128
Copy link
Collaborator

I have created a PR on libsrtp so we can do an early review of the code and get feedback as soon as possible on the design.

The PR adds a new attribute on the libsrtp policy use_cryptex which controls when cryptex is used on srtp_protect. For srtp_unprotect the 0xc0de and 0xc2de values of the define by profile attribute of the header extension is used for checking if cryptex has to be applied or not.

The code replaces the defined by profile in protect and unprotect so the application only needs to set the use_cryptex=true and no other modification is required. In case that the csrcs are set but the x bit is not set, the library returns an error as there might not be enough extra space in the buffer to add the empty header, it would be the app responsability to add it. I think using csrcs but no header extensions would be quite rare so I think it is the best tradeoff.

The modified code passes the previous tests so I hope there are no regressions when cryptex is not enabled, but I have not added any test for the new code yet as I am not sure where is the best way to add the tests for cryptex, @fluffy can you advise please?

@JonathanLennox
Copy link
Contributor

I have an implementation for the jitsi-srtp library at jitsi/jitsi-srtp#29 . We should generate some test vectors so we can confirm interop...

@murillo128
Copy link
Collaborator

that would be awesome.

I would say that we need to do one set for aead and non-aead suites, and each should cover at least

  • a packet without csrcs or header extension (that should output the same as "normal" srtp)
  • a packet with csrcs and a 0 length header extension (for both 1 and 2 bytes header extensions)
  • a packet without csrcs and header extension (for both 1 and 2 bytes header extensions)
  • a packet with csrcs and header extensions (for both 1 and 2 bytes header extensions)

@JonathanLennox
Copy link
Contributor

I've added a bunch of test packets to my jitsi-srtp PR -- do you want to try to test them against libsrtp and see if your implementation agrees?

@murillo128
Copy link
Collaborator

sure, will do it this sunday

@murillo128
Copy link
Collaborator

I have fixed the AAD issue and added test with the same tests vectors as you have on your jitsi PR. Everything seems to be working fine.

I will send a PR for adding them to the spec.

@juberti
Copy link
Owner Author

juberti commented Mar 7, 2022

See cisco/libsrtp#511 (still open)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants