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

Denial of service possible (DoS) #71

Closed
bengivre opened this issue Nov 14, 2019 · 3 comments
Closed

Denial of service possible (DoS) #71

bengivre opened this issue Nov 14, 2019 · 3 comments

Comments

@bengivre
Copy link

Hello,
I'm doing some testing on this package, thanks for the great work. 👍

I wanted to share with you two possible denial of service.
Right now, anybody can open a connection and keep it for ever doing :

- Bad command ( commands that are not handle by the parse() function )
c.nbrErrors++ will be incremented but will never trigger a connection close()

- Speak Up (When command is empty)
A connection can stay open , sending empty commands for ever

In both cases, the attacker can open as many connection, send either "bad commands" not parsed. So , for example, 1 char, 2char, 3 char commands or just empty commands and will never reach any disconnect.

As a quick fix, I would suggest this to evaluate the c.nbrErrors counter and disconnect if > 3
In both scenarios

Example:
File: conn.go
Line: 101

	if cmd == "" {
		c.nbrErrors++
		if c.nbrErrors > 3 {
			c.WriteResponse(500, EnhancedCode{5, 5, 2}, "Too many errors")
			c.Close()
		}	
		c.WriteResponse(500, EnhancedCode{5, 5, 2}, "Speak up")
		return
	}

file: server.go
Line: 137

	if err != nil {
		c.nbrErrors++
		if c.nbrErrors > 3 {
			c.WriteResponse(500, EnhancedCode{5, 5, 2}, "Too many errors")
			c.Close()
		}					
	
		c.WriteResponse(501, EnhancedCode{5, 5, 2}, "Bad command")
		continue
	}	

Hope this help.

@foxcpp
Copy link
Collaborator

foxcpp commented Nov 14, 2019

nbrErrors is checked here:

if c.nbrErrors > 3 {

But yeah, I think empty lines should be handled as invalid commands too. And nbrErrors should be checked in server.go:137. Perhaps, make a PR?

Also, if there's intention to DoS the server, this can't prevent the attacker from using valid (and potentially much more expensive) commands.

@bengivre
Copy link
Author

In fact this check will only happen on commands with char > 4
I'll send you a PR

@emersion
Copy link
Owner

Fixed in #116

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

No branches or pull requests

3 participants