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

Metadata-pushes rate is not capped on responder, may lead to server resources starvation #865

Closed
mostroverkhov opened this issue Jun 12, 2020 · 2 comments
Labels
needs information superseded Issue is superseded by another

Comments

@mostroverkhov
Copy link
Member

External clients can start hundreds of thousands of metadata-pushes over multiple connections.

If metadata-push handler is present and It does not implement rate limiting on its own, then server is subject to resource starvation attack.

If metadata-push handler is absent, then exception with stacktrace is created for every metadata-push - this just consumes cpu cycles. With sufficiently large number of metadata-pushes RSocket becomes unresponsive.

Problem worsens with websocket transport of this repository. By default It sets frame size limit to ~16MB - this opens opportunity for memory overflow given there may be arbitrarily large number of concurrent metadata-pushes with size close to said size limit.

@OlegDokuka
Copy link
Member

OlegDokuka commented Jun 12, 2020

Hey!

First of all, we appreciate issues and bug reports!

However, sometimes it is HARD to figure out what the issue is about. In certain: how to reproduce it; what is the root cause; what is possible solutions to mitigate it!

Having no such info just makes it challenging for maintainers to help submitter / improve project by resolving the issue.

Therefore, we created issues' templates for that purpose so the issue submitter can express all important points that maintainers need in order to resolve an issue. Otherwise, if thoughts were not expressed clearly just make the issue useless and waste maintainers' time on figuring out what was said.

Therefore, please respect maintainers by following general issues' templates if you submitting an issue!

Thanks in advance.


  1. Any code samples to reproduce that?

  2. I believe the same issue can happen with the fnf call if leasing is not enabled. Is not it?

If metadata-push handler is absent, then exception with stacktrace is created for every metadata-push - this just consumes cpu cycles. With sufficiently large number of metadata-pushes RSocket becomes unresponsive.

  1. This is a bug 🐞. However, the issue is not critical and a workaround for everything lower than 1.0.2 will be overriding of all RSocket methods with a static response error

  2. The solution for this issue will be creating static Mono.error/Flux.error for RSocket.class so stack trace will not be filled on every call.

Problem worsens with websocket transport of this repository. By default It sets frame size limit to ~16MB - this opens opportunity for memory overflow given there may be arbitrarily large number of concurrent metadata-pushes with size close to said size limit.

The same as for point 1 - any code that reproduces that, or is it just a theoretical assumption that has no proof?

On the RSocketRequester side we have Payload validation which ensures that payload is not overdue 16 content size. That said it is impossible to send frame bigger than 16 MB -> since validation code ensures that frame size is less or equal to 16MB (total sum of headers and content)

  1. Apart, if one sends many requests with the large payload for any request type may be an opportunity for memory overflow is not it? How metadata-push differs from the same fnf or requestResponse calls no counting that metadata push is not a subject for leasing? I guess this is an application concern on how to prevent memory overflow attacks and extra application security can be introduced to protect against the malicious caller. Is not it?

WDYT @rstoyanchev @rwinch?

Cheers,
Oleh

@OlegDokuka OlegDokuka added this to the 1.x Backlog milestone Jul 27, 2020
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jul 27, 2020

I think metadata pushes are not meant to be used for sending a lot of data or to be sent very frequently. We can add some basic rate limiting mechanism that allows up a certain number of pushes and/or metadata size within some period of time. That can be much lower than the general 16 MB frame size from the spec.

Also since metadata isn't subject to fragmentation then we are not aggregating fragments and an application can use interception to reject large metadata pushes or any metadata pushes as a workaround.

@OlegDokuka OlegDokuka modified the milestones: 1.x Backlog, 1.0.3 Jul 27, 2020
rstoyanchev added a commit to rstoyanchev/rsocket-java that referenced this issue Sep 22, 2020
Closes rsocketgh-865

Signed-off-by: Rossen Stoyanchev <rstoyanchev@vmware.com>
rstoyanchev added a commit to rstoyanchev/rsocket-java that referenced this issue Sep 24, 2020
Closes rsocketgh-865

Signed-off-by: Rossen Stoyanchev <rstoyanchev@vmware.com>
@rstoyanchev rstoyanchev added the superseded Issue is superseded by another label Sep 24, 2020
@rstoyanchev rstoyanchev removed this from the 1.0.3 milestone Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs information superseded Issue is superseded by another
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants