tests and fixes for issue #69

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")

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 (

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 {

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")
+		}
+	}
+}