Use unique IDs and internal hashtable to map SCTP associations with usrsctp #2302
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
blablabla but looks ok |
|
Well, look at it this way: you can just open a PR with no text and say "read here"... I did you a favor! |
|
Thanks I'll do |
|
Just warn them that the description might be a bit impolite... |
ibc
added a commit
to versatica/mediasoup
that referenced
this pull request
Jul 29, 2020
instead of passing this pointer to usrsctp API - Related commit in libwebrtc: https://webrtc-review.googlesource.com/c/src/+/176422/5/media/sctp/sctp_transport.cc - Related commit in Janus: meetecho/janus-gateway#2302
ibc
added a commit
to versatica/mediasoup
that referenced
this pull request
Jul 29, 2020
…ons (#439) Fix the usrsctp vulnerability by using a global map for SctpAssociations instead of passing this pointer to usrsctp API - Related commit in libwebrtc: https://webrtc-review.googlesource.com/c/src/+/176422/5/media/sctp/sctp_transport.cc - Related commit in Janus: meetecho/janus-gateway#2302
|
I don't see any objection, so I'll merge. |
Actually you don't see anything. |
|
Yeah, that was my point. But I can't wait for feedback forever, so merging they HAVE to test. |
Yeah, once we update! :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch aims at addressing a vulnerability that apparently still affects usrsctp, and so you should test this if you use datachannels in your application. You can learn more about the vulnerability (and the workaround we implemented) by reading this thread on Twitter.
Basically, with usrsctp almost everyone (including us, libwebrtc developers, and others) passed our custom structures/classes as opaque pointers when creating associations, as this way usrsctp would provide the same pointer back in callbacks, making it easier to address, e.g., incoming messages and events and relate them to the right instance. Apparently, though, usrsctp is actually putting the address of that pointer in SCTP messages (as part of the cookie), which creates an obvious vulnerability. I was made aware of this issue some time ago, but at the same time the notice said this had been fixed in usrsctp itself about a year ago: since we always recommend installing the latest version, I assumed we were fine, but apparently that's not the case. As such, I'm implementing the same workaround the libwebrtc developers implemented, that is an internal map with unique IDs: we pass these harmless IDs to usrsctp, and then look for the right SCTP instance when we get an ID in one of the callbacks.
While it seems to be working as expected in the few tests I made, you're encouraged to test this more thoroughly yourself, especially if you rely on datachannels in your applications. This new map I added also adds a new mutex, and as such there's always the risk of deadlocks in unexpected circumstances (which should happen, but you never know).