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.

Sign up for GitHub

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

Jump to bottom

libsrtp: wraparound with loss decryption failure #1437

Closed
ibc opened this issue Aug 5, 2024 · 2 comments · Fixed by #1438
Closed

libsrtp: wraparound with loss decryption failure #1437

ibc opened this issue Aug 5, 2024 · 2 comments · Fixed by #1438
Labels
bug dependencies 3rd party dependencies/subprojects
Milestone
v3 updates

Comments

@ibc
Copy link
Member

ibc commented Aug 5, 2024

Rationale in the ticket: https://webrtc-review.googlesource.com/c/src/+/358360

Theoretical solution is that the first output seq number of each mediasoup XxxxConsumer is not 0 or 1 (to avoid that a previous packet arriving later triggers the bug in libsrtp) and it's smaller than 2^15.
@ibc ibc added the bug label Aug 5, 2024
@ibc ibc added this to the v3 updates milestone Aug 5, 2024
@ibc ibc added the dependencies 3rd party dependencies/subprojects label Aug 5, 2024
@ibc ibc changed the title Mitigate libsrtp bug when first generated output seq number is around 0 libsrtp: wraparound with loss decryption failure Aug 5, 2024
ibc added a commit that referenced this issue Aug 5, 2024
@ibc
Mitigate libsrtp wraparound with loss decryption failure
759cce8 
Fixes #1437

### Details

- Read the issue, please.
- So solution is that `SeqManager` now includes a second constructor with `initialOutput` and we use it in all `XxxxConsumer` classes.
@ibc ibc mentioned this issue Aug 5, 2024
Merged
@fippo
Copy link
Contributor

fippo commented Aug 5, 2024

does your sequence number mapper handle this case "correctly":

  • map(5) -> 5
  • map(4) -> 4
    I always assumed one would not have to bother with 4 in this case since it is too old but...

@ibc
Copy link
Member Author

ibc commented Aug 5, 2024

does your sequence number mapper handle this case "correctly":

  • map(5) -> 5

  • map(4) -> 4

I always assumed one would not have to bother with 4 in this case since it is too old but...

There are like 10000 tests covering that case and more complex ones in TestSeqManager.cpp.

@fippo fippo mentioned this issue Aug 6, 2024
Closed
fippo added a commit to fippo/mediasoup that referenced this issue Aug 6, 2024
@fippo
add unit test for mapping packets with lower sequence number than the…
9a7c0c0 
… first mapped packet

see versatica#1437 and versatica#1438. Works but why not have a test
fippo added a commit to fippo/mediasoup that referenced this issue Aug 6, 2024
@fippo
add unit test for mapping packets with lower sequence number than the…
29703d4 
… first mapped packet

see versatica#1437 and versatica#1438. Works but why not have a test
@ibc ibc closed this as completed in #1438 Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug dependencies 3rd party dependencies/subprojects
Milestone
v3 updates
Development

Successfully merging a pull request may close this issue.

Mitigate libsrtp wraparound with loss decryption failure versatica/mediasoup
2 participants
@ibc @fippo