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

Add new method to init the stack with threads but no UDP encapsulation #532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

lminiero
Copy link

@lminiero lminiero commented Oct 1, 2020

Hi all,

I recently found out that when the stack is initialized with usrsctp_init, a dedicated thread for SCTP over UDP encapsulation is started too, which binds to ports on both IPv4 and IPv6. In my case ports are random because I'm passing 0 to the init method. Since I'm using usrsctp as a stack in a WebRTC server, I don't need that encapsulation: I'm actually wondering if it might be "dangerous" to have in the first place, as I'm not sure what happens in my code if someone starts sending SCTP messages to those ports via UDP (possibly a risk of highjacking existing associations?).

Looking at the code, apparently the only way to prevent those sockets from being created is calling usrsctp_init_nothreads, that stops recv_thread_init from being called (which is where those sockets are created and managed). This also forces us to handle the usrsctp stack in our own event loop, which increases the complexity on our end. As such, I prepared this simple PR that adds a new init method, called usrsctp_init_noudpthread, that still allows the stack to create threads, but prevents recv_thread_init from being called.

This seems to do the trick, even though I'm not sure this is the right way to integrate it in the existing codebase. In fact, I'm "abusing" the start_threads argument passed to the internal sctp_init for my purposes. In the existing code:

  • usrsctp_init passes start_threads=1
  • usrsctp_init_nothreads passes start_threads=0

My PR adds code so that all the above still is true, and

  • usrsctp_init_noudpthread passes start_threads=2

This means that all checks that do something like if(start_threads) still resolve to TRUE as they should. To make sure the UDP encapsulation code is not started, I modified one of those checks to an explicit start_threads == 1 instead, so that it doesn't kick in if I pass 2. More precisely:

sctp_pcb_init(start_threads == 1);

As sctp_pcb_init is what invokes recv_thread_init if the start_threads it is aware of is not 0.

Please let me know if that makes sense and is acceptable as it is, or if it can have implications on other areas of the code. I still think we need a way to allow stacks not to create that UDP encapsulation stuff, as I believe it might be a vector of attacks in unsuspecting applications.

Thanks!

@tuexen
Copy link
Member

tuexen commented Oct 1, 2020

So you don't want the usrsctp library to process incoming SCTP/UPD/IP packets. But what is about SCTP/IP packets? I guess in WebRTC you are only interested in AF_CONN sockets or am I missing something?

@lminiero
Copy link
Author

lminiero commented Oct 1, 2020

Thanks for the quick answer @tuexen! In WebRTC, we only do SCTP over DTLS, which means we only exchange buffers around, and use DTLS as the encapsulation protocol for that at an application level. So we never do any SCTP over IP or UDP. The code was based on the rtcweb.c example provided in this repo and written by Ekr at the time.

@lminiero
Copy link
Author

lminiero commented Oct 1, 2020

To answer your other question, yes, we use AF_CONN when creating the socket, but then intercept messages so that we can encapsulate and send/receive them on our own terms (via DTLS bios).

@tuexen
Copy link
Member

tuexen commented Oct 1, 2020

So you don't want to process SCTP/UDP/IP and don't want to process SCTP/IP packets. Let me think...

However, I it is a bug that when providing 0 as the UDP port number in sctp_init(), ephemeral ports are used. Although considered a feature in #265, it should be fixed. That would resolve your issue with the SCTP/UDP/IP packets, but not with SCTP/IP. Assuming you are using Linux, are you running the process which runs the SCTP stack as root?

tuexen added a commit that referenced this pull request Oct 1, 2020
UDP sockets) when the UDP port specified in scpt_init() is 0.

This helps #532. Howver,
this removes the non-intended behaviour used in
#265.
@tuexen
Copy link
Member

tuexen commented Oct 1, 2020

I fixed in f15553f that UDP encapsulation is not disabled, when providing 0 as the UDP port.

What about adding a function sctp_init_no_recv_threads(), which does not start the SCTP/UDP/IP, SCTP/IP, routing, and in the future ICMP/IP and ICMP6/IPV6 threads. That should work for you...

@lminiero
Copy link
Author

lminiero commented Oct 1, 2020

Thanks for the commit! We were not using the "ephemeral ports as a feature", so it's fine by me. On your other question:

Assuming you are using Linux, are you running the process which runs the SCTP stack as root?

I almost always launch the stack as non-root (Janus doesn't require it) on my laptop, but in several deployments we do use root instead (as do others).

I think a sctp_init_no_recv_threads method would indeed help. Wouldn't it simply not call recv_thread_init as this patch does, though? Or is there anything else that is required for that to work as expected? Asking as, when you launch the stack with nothreads, that's already being disabled apparently, which means that nothreads seems to currently also imply that all that networking you mentioned is already disabled.

@tuexen
Copy link
Member

tuexen commented Oct 1, 2020

Here is the way for you: Just compile the library without INET and INET6 being defined. Then you get no support for SCTP/IP and SCTP/UDP/IP at all. Including the threads not getting started. Does this resolve you problem?

@lminiero
Copy link
Author

lminiero commented Oct 1, 2020

I think that would work too. I was just wondering if there was an easy way to disable things programmatically, as in principle the same library may be used by different projects on the same machine, and so disabling things at compile time may break features that some other application needs. Shouldn't be an issue in the vast majority of cases, though.

@tuexen
Copy link
Member

tuexen commented Oct 1, 2020

I think that would work too. I was just wondering if there was an easy way to disable things programmatically, as in principle the same library may be used by different projects on the same machine, and so disabling things at compile time may break features that some other application needs. Shouldn't be an issue in the vast majority of cases, though.

I have no idea how people use the library. I was assuming that they build it within their application and do not install them system wide. I'll add such a function, but you can just compile without INET and INET6 to get what you want right now.

@lminiero
Copy link
Author

lminiero commented Oct 1, 2020

I can confirm compiling with --disable-inet and --disable-inet6 works, so that's enough for me, thanks! Please feel free to close this PR, and thanks again for this very helpful exchange 🙂

@tuexen
Copy link
Member

tuexen commented Oct 1, 2020

Thanks for the commit! We were not using the "ephemeral ports as a feature", so it's fine by me. On your other question:

Assuming you are using Linux, are you running the process which runs the SCTP stack as root?

I almost always launch the stack as non-root (Janus doesn't require it) on my laptop, but in several deployments we do use root instead (as do others).

If you compile without INET and INET6, everything should be fine. If you compile with INET or INET6 and run as root, raw sockets would be opened. The library would process SCTP/IP packets as long as you don't have SCTP kernel support enabled. On linux this means if the sctp kernel module is loaded.

I think a sctp_init_no_recv_threads method would indeed help. Wouldn't it simply not call recv_thread_init as this patch does, though? Or is there anything else that is required for that to work as expected? Asking as, when you launch the stack with nothreads, that's already being disabled apparently, which means that nothreads seems to currently also imply that all that networking you mentioned is already disabled.

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

Successfully merging this pull request may close these issues.

2 participants