Repository: pires/go-proxyproto (go)

Issue:
parseVersion1() is not secure
```
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.

Pull request description:
tests and fixes for issue #69
2As suggested in PR#70, a new pull request which captures just the tests proving issue #69, with some refinements:

- header overflow (i.e. reads more than the bufio.Reader default buffer size of 4096).
- "slow loris" type vulnerability, where the header read blocks waiting for terminator.

The second issue above can be used for DoS by consuming server process sockets.

Will then submit fixes - further testing shows that pull request #70 does not solve the problem properly.

Fixes #69

Patch:
diff --git a/header.go b/header.go
index db877e8..81ebeb3 100644
--- a/header.go
+++ b/header.go
@@ -16,7 +16,9 @@ var (
 	SIGV1 = []byte{'\x50', '\x52', '\x4F', '\x58', '\x59'}
 	SIGV2 = []byte{'\x0D', '\x0A', '\x0D', '\x0A', '\x00', '\x0D', '\x0A', '\x51', '\x55', '\x49', '\x54', '\x0A'}
 
-	ErrLineMustEndWithCrlf                  = errors.New("proxyproto: header is invalid, must end with \\r\\n")
+	ErrCantReadVersion1Header               = errors.New("proxyproto: can't read version 1 header")
+	ErrVersion1HeaderTooLong                = errors.New("proxyproto: version 1 header must be 107 bytes or less")
+	ErrLineMustEndWithCrlf                  = errors.New("proxyproto: version 1 header is invalid, must end with \\r\\n")
 	ErrCantReadProtocolVersionAndCommand    = errors.New("proxyproto: can't read proxy protocol version and command")
 	ErrCantReadAddressFamilyAndProtocol     = errors.New("proxyproto: can't read address family or protocol")
 	ErrCantReadLength                       = errors.New("proxyproto: can't read length")
diff --git a/v1.go b/v1.go
index 9ff686a..23de95e 100644
--- a/v1.go
+++ b/v1.go
@@ -3,6 +3,7 @@ package proxyproto
 import (
 	"bufio"
 	"bytes"
+	"fmt"
 	"net"
 	"strconv"
 	"strings"
@@ -22,17 +23,79 @@ func initVersion1() *Header {
 }
 
 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')
-	if err != nil {
-		return nil, ErrLineMustEndWithCrlf
-	}
-	if !strings.HasSuffix(line, crlf) {
+	//The header cannot be more than 107 bytes long. Per spec:
+	//
+	//   (...)
+	//   - worst case (optional fields set to 0xff) :
+	//     "PROXY UNKNOWN ffff:f...f:ffff ffff:f...f:ffff 65535 65535\r\n"
+	//     => 5 + 1 + 7 + 1 + 39 + 1 + 39 + 1 + 5 + 1 + 5 + 2 = 107 chars
+	//
+	//   So a 108-byte buffer is always enough to store all the line and a
+	//   trailing zero for string processing.
+	//
+	// It must also be CRLF terminated, as above. The header does not otherwise
+	// contain a CR or LF byte.
+	//
+	// ISSUE #69
+	// We can't use Peek here as it will block trying to fill the buffer, which
+	// will never happen if the header is TCP4 or TCP6 (max. 56 and 104 bytes
+	// respectively) and the server is expected to speak first.
+	//
+	// Similarly, we can't use ReadString or ReadBytes as these will keep reading
+	// until the delimiter is found; an abusive client could easily disrupt a
+	// server by sending a large amount of data that do not contain a LF byte.
+	// Another means of attack would be to start connections and simply not send
+	// data after the initial PROXY signature bytes, accumulating a large
+	// number of blocked goroutines on the server. ReadSlice will also block for
+	// a delimiter when the internal buffer does not fill up.
+	//
+	// A plain Read is also problematic since we risk reading past the end of the
+	// header without being able to easily put the excess bytes back into the reader's
+	// buffer (with the current implementation's design).
+	//
+	// So we use a ReadByte loop, which solves the overflow problem and avoids
+	// reading beyond the end of the header. However, we need one more trick to harden
+	// against partial header attacks (slow loris) - per spec:
+	//
+	//    (..) The sender must always ensure that the header is sent at once, so that
+	//    the transport layer maintains atomicity along the path to the receiver. The
+	//    receiver may be tolerant to partial headers or may simply drop the connection
+	//    when receiving a partial header. Recommendation is to be tolerant, but
+	//    implementation constraints may not always easily permit this.
+	//
+	// We are subject to such implementation constraints. So we return an error if
+	// the header cannot be fully extracted with a single read of the underlying
+	// reader.
+	buf := make([]byte, 0, 107)
+	for {
+		b, err := reader.ReadByte()
+		if err != nil {
+			return nil, fmt.Errorf(ErrCantReadVersion1Header.Error()+": %v", err)
+		}
+		buf = append(buf, b)
+		if b == '\n' {
+			// End of header found
+			break
+		}
+		if len(buf) == 107 {
+			// No delimiter in first 107 bytes
+			return nil, ErrVersion1HeaderTooLong
+		}
+		if reader.Buffered() == 0 {
+			// Header was not buffered in a single read. Since we can't
+			// differentiate between genuine slow writers and DoS agents,
+			// we abort. On healthy networks, this should never happen.
+			return nil, ErrCantReadVersion1Header
+		}
+	}
+
+	// Check for CR before LF.
+	if len(buf) < 2 || buf[len(buf)-2] != '\r' {
 		return nil, ErrLineMustEndWithCrlf
 	}
+
 	// Check full signature.
-	tokens := strings.Split(line[:len(line)-2], separator)
+	tokens := strings.Split(string(buf[:len(buf)-2]), separator)
 
 	// Expect at least 2 tokens: "PROXY" and the transport protocol.
 	if len(tokens) < 2 {


Test patch:
diff --git a/header_test.go b/header_test.go
index 5385369..9b8662a 100644
--- a/header_test.go
+++ b/header_test.go
@@ -13,11 +13,12 @@ import (
 // Stuff to be used in both versions tests.
 
 const (
-	NO_PROTOCOL  = "There is no spoon"
-	IP4_ADDR     = "127.0.0.1"
-	IP6_ADDR     = "::1"
-	PORT         = 65533
-	INVALID_PORT = 99999
+	NO_PROTOCOL   = "There is no spoon"
+	IP4_ADDR      = "127.0.0.1"
+	IP6_ADDR      = "::1"
+	IP6_LONG_ADDR = "1234:5678:9abc:def0:cafe:babe:dead:2bad"
+	PORT          = 65533
+	INVALID_PORT  = 99999
 )
 
 var (
diff --git a/v1_test.go b/v1_test.go
index 8d6dab1..0267580 100644
--- a/v1_test.go
+++ b/v1_test.go
@@ -3,19 +3,25 @@ package proxyproto
 import (
 	"bufio"
 	"bytes"
+	"io"
+	"net"
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 )
 
 var (
 	IPv4AddressesAndPorts        = strings.Join([]string{IP4_ADDR, IP4_ADDR, strconv.Itoa(PORT), strconv.Itoa(PORT)}, separator)
 	IPv4AddressesAndInvalidPorts = strings.Join([]string{IP4_ADDR, IP4_ADDR, strconv.Itoa(INVALID_PORT), strconv.Itoa(INVALID_PORT)}, separator)
 	IPv6AddressesAndPorts        = strings.Join([]string{IP6_ADDR, IP6_ADDR, strconv.Itoa(PORT), strconv.Itoa(PORT)}, separator)
+	IPv6LongAddressesAndPorts    = strings.Join([]string{IP6_LONG_ADDR, IP6_LONG_ADDR, strconv.Itoa(PORT), strconv.Itoa(PORT)}, separator)
 
 	fixtureTCP4V1 = "PROXY TCP4 " + IPv4AddressesAndPorts + crlf + "GET /"
 	fixtureTCP6V1 = "PROXY TCP6 " + IPv6AddressesAndPorts + crlf + "GET /"
 
+	fixtureTCP6V1Overflow = "PROXY TCP6 " + IPv6LongAddressesAndPorts
+
 	fixtureUnknown              = "PROXY UNKNOWN" + crlf
 	fixtureUnknownWithAddresses = "PROXY UNKNOWN " + IPv4AddressesAndInvalidPorts + crlf
 )
@@ -58,7 +64,7 @@ var invalidParseV1Tests = []struct {
 	{
 		desc:          "incomplete signature TCP4",
 		reader:        newBufioReader([]byte("PROXY TCP4 " + IPv4AddressesAndPorts)),
-		expectedError: ErrLineMustEndWithCrlf,
+		expectedError: ErrCantReadVersion1Header,
 	},
 	{
 		desc:          "TCP6 with IPv4 addresses",
@@ -75,13 +81,18 @@ var invalidParseV1Tests = []struct {
 		reader:        newBufioReader([]byte("PROXY TCP4 " + IPv4AddressesAndInvalidPorts + crlf)),
 		expectedError: ErrInvalidPortNumber,
 	},
+	{
+		desc:          "header too long",
+		reader:        newBufioReader([]byte("PROXY UNKNOWN " + IPv6LongAddressesAndPorts + " " + crlf)),
+		expectedError: ErrVersion1HeaderTooLong,
+	},
 }
 
 func TestReadV1Invalid(t *testing.T) {
 	for _, tt := range invalidParseV1Tests {
 		t.Run(tt.desc, func(t *testing.T) {
 			if _, err := Read(tt.reader); err != tt.expectedError {
-				t.Fatalf("expected %s, actual %s", tt.expectedError, err.Error())
+				t.Fatalf("expected %s, actual %v", tt.expectedError, err)
 			}
 		})
 	}
@@ -175,3 +186,150 @@ func TestWriteV1Valid(t *testing.T) {
 		})
 	}
 }
+
+// Tests for parseVersion1 overflow - issue #69.
+
+type dataSource struct {
+	NBytes int
+	NRead  int
+}
+
+func (ds *dataSource) Read(b []byte) (int, error) {
+	if ds.NRead >= ds.NBytes {
+		return 0, io.EOF
+	}
+	avail := ds.NBytes - ds.NRead
+	if len(b) < avail {
+		avail = len(b)
+	}
+	for i := 0; i < avail; i++ {
+		b[i] = 0x20
+	}
+	ds.NRead += avail
+	return avail, nil
+}
+
+func TestParseVersion1Overflow(t *testing.T) {
+	ds := &dataSource{}
+	reader := bufio.NewReader(ds)
+	bufSize := reader.Size()
+	ds.NBytes = bufSize * 16
+	parseVersion1(reader)
+	if ds.NRead > bufSize {
+		t.Fatalf("read: expected max %d bytes, actual %d\n", bufSize, ds.NRead)
+	}
+}
+
+func listen(t *testing.T) *Listener {
+	l, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	return &Listener{Listener: l}
+}
+
+func client(t *testing.T, addr, header string, length int, terminate bool, wait time.Duration, done chan struct{}) {
+	c, err := net.Dial("tcp", addr)
+	if err != nil {
+		t.Fatalf("dial: %v", err)
+	}
+	defer c.Close()
+
+	if terminate && length < 2 {
+		length = 2
+	}
+
+	buf := make([]byte, len(header)+length)
+	copy(buf, []byte(header))
+	for i := 0; i < length-2; i++ {
+		buf[i+len(header)] = 0x20
+	}
+	if terminate {
+		copy(buf[len(header)+length-2:], []byte(crlf))
+	}
+
+	n, err := c.Write(buf)
+	if err != nil {
+		t.Fatalf("write: %v", err)
+	}
+	if n != len(buf) {
+		t.Fatalf("write; short write")
+	}
+
+	time.Sleep(wait)
+	close(done)
+}
+
+func TestVersion1Overflow(t *testing.T) {
+	done := make(chan struct{})
+
+	l := listen(t)
+	go client(t, l.Addr().String(), fixtureTCP6V1Overflow, 10240, true, 10*time.Second, done)
+
+	c, err := l.Accept()
+	if err != nil {
+		t.Fatalf("accept: %v", err)
+	}
+
+	b := []byte{}
+	_, err = c.Read(b)
+	if err == nil {
+		t.Fatalf("net.Conn: no error reported for oversized header")
+	}
+}
+
+func TestVersion1SlowLoris(t *testing.T) {
+	done := make(chan struct{})
+	timeout := make(chan error)
+
+	l := listen(t)
+	go client(t, l.Addr().String(), fixtureTCP6V1Overflow, 0, false, 10*time.Second, done)
+
+	c, err := l.Accept()
+	if err != nil {
+		t.Fatalf("accept: %v", err)
+	}
+
+	go func() {
+		b := []byte{}
+		_, err = c.Read(b)
+		timeout <- err
+	}()
+
+	select {
+	case <-done:
+		t.Fatalf("net.Conn: reader still blocked after 10 seconds")
+	case err := <-timeout:
+		if err == nil {
+			t.Fatalf("net.Conn: no error reported for incomplete header")
+		}
+	}
+}
+
+func TestVersion1SlowLorisOverflow(t *testing.T) {
+	done := make(chan struct{})
+	timeout := make(chan error)
+
+	l := listen(t)
+	go client(t, l.Addr().String(), fixtureTCP6V1Overflow, 10240, false, 10*time.Second, done)
+
+	c, err := l.Accept()
+	if err != nil {
+		t.Fatalf("accept: %v", err)
+	}
+
+	go func() {
+		b := []byte{}
+		_, err = c.Read(b)
+		timeout <- err
+	}()
+
+	select {
+	case <-done:
+		t.Fatalf("net.Conn: reader still blocked after 10 seconds")
+	case err := <-timeout:
+		if err == nil {
+			t.Fatalf("net.Conn: no error reported for incomplete and overflowed header")
+		}
+	}
+}