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

Prevent instability and crash due to message flood #3510

Merged
merged 1 commit into from Aug 30, 2018

Conversation

Projects
None yet
3 participants
@tu-maurice
Copy link
Contributor

tu-maurice commented Aug 30, 2018

This patch adds a rate limiting to selected patches. The underlying rate limiter
used is the Leaky-Bucket algorithm. It allows for a burst of messages, but
limits them after a specified amount of messages within a time frame.
If the ratelimit hits the messages are simply ignored.

For now its set to a burst of 30 allowed messages and a subsequent limit of 4 messages per second, which seems to do the trick without inconveniencing normal users.

It should to some extend prevent the recent issues with bots decribed in #3505.

What was tested with the patch:

  • Joining (works)
  • Talking with UDP and TCP mode (works)
  • Switching channels (works)
  • Bringing down the server with a bot, tested with two different bots (does not work)

I'm thanking the Zom.bi community for testing and @Natenom for some insight into the problem.

I'm open to any suggestions.

@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

tu-maurice commented Aug 30, 2018

@davidebeatrici May I ask what priority/P0 entails?

@davidebeatrici

This comment has been minimized.

Copy link
Member

davidebeatrici commented Aug 30, 2018

From https://wiki.mumble.info/wiki/Issue_Priorities:

MUST be fixed ASAP. Issues that prevent further development; legal issues; serious data loss issues; build issues for platforms we provide binaries for.

@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

tu-maurice commented Aug 30, 2018

@davidebeatrici How may I understand this? Is it my patch that "MUST be fixed ASAP" or does the patch fix something that "MUST be fixed ASAP"?

@davidebeatrici

This comment has been minimized.

Copy link
Member

davidebeatrici commented Aug 30, 2018

The patch fixes something that "MUST be fixed ASAP".

@davidebeatrici

This comment has been minimized.

Copy link
Member

davidebeatrici commented Aug 30, 2018

Qt 4 builds failing because:

In file included from /usr/include/c++/4.8/chrono:35:0,
                 from ./ServerUser.h:17,
                 from ../ACL.cpp:14:
/usr/include/c++/4.8/bits/c++0x_warning.h:32:2: error: #error This file requires compiler and library support for the ISO C++ 2011 standard. This support is currently experimental, and must be enabled with the -std=c++11 or -std=gnu++11 compiler options.
 #error This file requires compiler and library support for the \
  ^

MinGW builds failing because:

ServerUser.h:106:36: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
   bool limit = this->currentTokens > (MAX_TOKENS - tokens);
                                    ^
@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

tu-maurice commented Aug 30, 2018

Alright, I can work with that. The first problem should be fixable by relying on pre-C++11 means to measure time if the software is not build using at least a C++11 compiler, unless we can have this as a requirement for building.

The second problem I didn't have, but it is easily fixable.

@Kissaki

This comment has been minimized.

Copy link
Member

Kissaki commented Aug 30, 2018

After landing we should also put this into 1.2.

@davidebeatrici

This comment has been minimized.

Copy link
Member

davidebeatrici commented Aug 30, 2018

I think that we shouldn't create another 1.2.x release.

@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

tu-maurice commented Aug 30, 2018

This edit to the patch should allow for pre-C++11 builds and fixes the warning with the unsigned signed comparison.

Edit: Is it normal that the CI job is pending for such a long time? It seems to be finished according to the "Details" link.

@tu-maurice tu-maurice force-pushed the tu-maurice:exploitpatch branch from 1b106c9 to 9ef8fb1 Aug 30, 2018

@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

tu-maurice commented Aug 30, 2018

I hope you don't mind that I squashed them, so the history looks nicer. 👍

@davidebeatrici

This comment has been minimized.

Copy link
Member

davidebeatrici commented Aug 30, 2018

No problem, we usually prefer commits to be squashed if they're all related.

@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

tu-maurice commented Aug 30, 2018

Perfect. Now do the last changes solve the problems you mentioned early? I'm sadly not able to easily try compiling it with Qt4 or MinGW.

@mumble-voip mumble-voip deleted a comment from davidebeatrici Aug 30, 2018

@davidebeatrici

This comment has been minimized.

Copy link
Member

davidebeatrici commented Aug 30, 2018

Builds succeeded.

Feel free to review my changes and squash the commits.

Prevent instability and crash due to message flood
This patch adds a rate limiting to selected patches. The underlying rate limiter
used is the Leaky-Bucket algorithm. It allows for a burst of messages, but
limits them after a specified amount of messages within a time frame.

@tu-maurice tu-maurice force-pushed the tu-maurice:exploitpatch branch from f7274d9 to 0daec57 Aug 30, 2018

@tu-maurice

This comment has been minimized.

Copy link
Contributor Author

tu-maurice commented Aug 30, 2018

I separated the now and millisecondsBetween methods from the LeakyBucket class, as they were neither operating directly on the object nor public. And I added static inline to each to encourage the compile to inline them.

Also I moved a comment.

Please look at it again to see if you're okay with that.

@davidebeatrici davidebeatrici merged commit 44b9004 into mumble-voip:master Aug 30, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@davidebeatrici

This comment has been minimized.

Copy link
Member

davidebeatrici commented Aug 30, 2018

Thank you very much for your contribution!

@davidebeatrici davidebeatrici referenced this pull request Aug 30, 2018

Closed

Rate limiting #3505

@tu-maurice tu-maurice deleted the tu-maurice:exploitpatch branch Aug 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment