search

KallDrexx / rust-media-libs

Rust based libraries for misc media functionality
Apache License 2.0
229 stars 58 forks source link

Limit stream creation in ServerSession? #39

Open sfackler opened 1 year ago

sfackler commented 1 year ago

ServerSession currently always allows the creation of new streams, and maintains a little bit of state for each in a hash map. In a scenario where you're authenticating clients on connect, the current behavior allows for unauthenticated clients to create an unlimited number of streams and e.g. OOM the server.

The RTMP spec is not super explicit, but I think it's implied that the connect command is always expected to come first before any createStreams. Do you think it'd make sense to check that the session is already connected in createStream similar to what's done when handling publish, play, etc?

It might also be nice to allow consumers to limit the number of concurrent streams even post-connect. It seems like RTMP use in the modern era is always one stream per connection (I think?), so enforcing a limit in ServerSession could help protect against malicious/broken clients as well.

KallDrexx commented 1 year ago

I just woke up (and still working on coffee) but it sounds like the two scenarios you are asking about are:

1) Client sends createStream call before connect (or never even calls connect) 2) Client sends connect, then sends many createStream calls but may never use them.

While the first doesn't seem like that big of an issue to me (and the few RTMP clients I wiresharked seemed to always connect before create stream, a malicious client could theoretically cause a panic if arithmetic overflow checking is enabled in situation #2.

So yeah I think it does make sense for the create stream pattern to change to return a createStreamRequest that the consumer of the server session has to accept or reject. This also means that server can decide the logic on its own whether to allow multiple create stream calls that aren't bound.

The accepting of create stream requests should probably also properly define what happens in an overflow situation. It's extremely unlikely a normal RTMP client would ever consume 4 billion streams so I'm not sure it's worth dealing with wrapping and re-using unused stream ids.