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

Jump to bottom

parseVersion1() is not secure #69

Closed
isedev opened this issue Feb 28, 2021 · 9 comments · Fixed by #71
Closed

parseVersion1() is not secure #69

isedev opened this issue Feb 28, 2021 · 9 comments · Fixed by #71

Comments

@isedev
Copy link

isedev commented Feb 28, 2021 

func parseVersion1(reader *bufio.Reader) (*Header, error) {
	// Read until LF shows up, otherwise fail.
	// At this point, can't be sure CR precedes LF which will be validated next.
	line, err := reader.ReadString('\n')

The reader is a default bufio.Reader wrapping a net.Conn. It will read from the connection until it finds a newline. Since no limits are implemented in the code, a deliberately malformed V1 header could be used to exhaust memory in a server process using this code - a form of DDoS. The exploit is simple: send a stream starting with "PROXY" and keep sending data (which does not contain a newline) until the target stops acknowledging.

In most real world circumstances, the actual risk is small since only trusted sources should be allowed to send proxy protocol headers. However, this is still a security issue and should be resolved.

Easiest fix: reader.Peek(107) and scan for a newline. If none is found, then it is not a valid version 1 header anyway, so you can fail fast (the maximum v1 header size is 107 bytes). Otherwise, proceed with the reader.ReadString('\n') in full confidence.
@emersion
Copy link
Contributor

Alternative fix: use Reader.ReadLine.

@isedev
Copy link
Author

isedev commented Feb 28, 2021 •
edited

You don't get to set delims with reader.ReadLine() and it doesn't return them either. You could probably make it work somehow, but doesn't seem straightforward at first glance.

@isedev
Copy link
Author

isedev commented Feb 28, 2021

Other thought: with the Peek approach, you leave the buffer intact until there's a strong indication of a valid header being present. Not sure how much that matters at this point thought... what's the consumer going to do with a stream starting with "PROXY"? So yes, either way, whatever is makes more sense from an implementation point of view.

@pires
Copy link
Owner

pires commented Mar 1, 2021

IIRC we setup a buffered reader with implicit size of 4096 which means if the delimiter is not present, an EOF error will be thrown.

@isedev
Copy link
Author

isedev commented Mar 1, 2021 •
edited

ReadBytes and ReadString do things differently: they will both accumulate as many full buffers and final fragment as necessary to find the delimiter.

https://golang.org/src/bufio/bufio.go?s=13166:13221#L482
https://golang.org/src/bufio/bufio.go?s=13166:13221#L430

I also tried it directly with a test program, just in case I misread the code. Both methods will return slices that are bigger than the internal buffer size.

@isedev
Copy link
Author

isedev commented Mar 1, 2021

This might save you some time: https://pastebin.com/M130q0Bb. Simple server doing a single Readline('\n') on implicitly sized buffered reader and a client that spams it before sending a newline... that one call returns over 90KB in the test example.

Output is:

wrote 10240 bytes
server reading
wrote 10240 bytes
wrote 10240 bytes
wrote 10240 bytes
wrote 10240 bytes
wrote 10240 bytes
wrote 10240 bytes
wrote 10240 bytes
wrote 10240 bytes
wrote 10240 bytes
read 92261 bytes
server done

@isedev isedev mentioned this issue Mar 1, 2021
Closed
@pires
Copy link
Owner

pires commented Mar 2, 2021

Very interesting. Will have to spend some time on this which I don't have right now, I'm sorry.

isedev pushed a commit to isedev/go-proxyproto that referenced this issue Mar 3, 2021
issue pires#69: add parseVersion1 overflow tests
b37d4ef
isedev pushed a commit to isedev/go-proxyproto that referenced this issue Mar 3, 2021
add tests proving issue pires#69
a9e1a53
isedev pushed a commit to isedev/go-proxyproto that referenced this issue Mar 3, 2021
add tests proving issue pires#69
ddfa812
isedev pushed a commit to isedev/go-proxyproto that referenced this issue Mar 3, 2021
add tests proving issue pires#69
3e0124d
@isedev isedev mentioned this issue Mar 3, 2021
Merged
isedev pushed a commit to isedev/go-proxyproto that referenced this issue Mar 3, 2021
fixes for pires#69
d36684f
isedev pushed a commit to isedev/go-proxyproto that referenced this issue Mar 3, 2021
add tests proving issue pires#69
ac2f466
isedev pushed a commit to isedev/go-proxyproto that referenced this issue Mar 3, 2021
fixes for pires#69
a368adc
@pires pires closed this as completed in #71 Mar 4, 2021
@mschneider82 mschneider82 mentioned this issue Mar 4, 2021
Merged
This was referenced Mar 8, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@abergmann
Copy link

CVE-2021-23351 was assigned to this issue.

@pires
Copy link
Owner

pires commented Mar 8, 2021

Thanks to the Snyk folks for this https://snyk.io/vuln/SNYK-GOLANG-GITHUBCOMPIRESGOPROXYPROTO-1081577

This was referenced Mar 12, 2021
Closed
Closed
@dependabot dependabot bot mentioned this issue Mar 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@emersion @pires @isedev @abergmann