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

Enforce a message size limit #53

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

Conversation

luk3yx
Copy link
Contributor

@luk3yx luk3yx commented Oct 2, 2018

Forces all messages to be at most 512 bytes, to stop cmd help all from the irc_commands DoSing the server (making it quit with the message RecvQ exceeded).

This is done in the core IRC mod so it limits everything rather than just irc_commands.

Credit to @bigfoot547 for finding this vulnerability.

Forces all messages to be at most 512 bytes, to stop 'cmd help all' from irc_commands DoSing the server.
This is done in the core IRC mod so it limits everything rather than just irc_commands.
@sofar
Copy link
Member

sofar commented Nov 3, 2018

Yes, but, can we please maybe get some logging in the console as well if players want to cause floods this way? Silently dropping output may also cause unintentional side effects, so we should consider replying back with an error message and throttling the user for at least a few seconds, too.

@luk3yx
Copy link
Contributor Author

luk3yx commented Nov 5, 2018

What about adding ... to the end of long messages?

PRIVMSG #channel :A really long testing messag...

@sofar
Copy link
Member

sofar commented Nov 5, 2018

I'd almost favor dropping the message and sending an error to the client. That may help prevent floods.

@luk3yx
Copy link
Contributor Author

luk3yx commented Nov 6, 2018

That would need a change somewhere else, there may be rouge irc.say()-s in other mods that need fixing too.

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