parseVersion1() is not secure

[isedev]

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]

Alternative fix: use Reader.ReadLine.

[isedev]

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]

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]

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]

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]

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

[pires]

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

[abergmann]

CVE-2021-23351 was assigned to this issue.

[pires]

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