net/http: harden Server against request smuggling

See RFC 7230.

Thanks to Régis Leroy for the report.

Change-Id: Ic1779bc2180900430d4d7a4938cac04ed73c304c
Reviewed-on: https://go-review.googlesource.com/11810Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>

src/net/http/readrequest_test.go

@@ -9,6 +9,7 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"net/url"
 	"reflect"
 	"strings"
@@ -323,6 +324,32 @@ var reqTests = []reqTest{
 		noTrailer,
 		noError,
 	},
+
+	// HEAD with Content-Length 0. Make sure this is permitted,
+	// since I think we used to send it.
+	{
+		"HEAD / HTTP/1.1\r\nHost: issue8261.com\r\nConnection: close\r\nContent-Length: 0\r\n\r\n",
+		&Request{
+			Method: "HEAD",
+			URL: &url.URL{
+				Path: "/",
+			},
+			Header: Header{
+				"Connection":     []string{"close"},
+				"Content-Length": []string{"0"},
+			},
+			Host:       "issue8261.com",
+			Proto:      "HTTP/1.1",
+			ProtoMajor: 1,
+			ProtoMinor: 1,
+			Close:      true,
+			RequestURI: "/",
+		},
+
+		noBody,
+		noTrailer,
+		noError,
+	},
 }
 
 func TestReadRequest(t *testing.T) {
@@ -357,10 +384,38 @@ func TestReadRequest(t *testing.T) {
 	}
 }
 
-func TestReadRequest_BadConnectHost(t *testing.T) {
-	data := []byte("CONNECT []%20%48%54%54%50%2f%31%2e%31%0a%4d%79%48%65%61%64%65%72%3a%20%31%32%33%0a%0a HTTP/1.0\n\n")
-	r, err := ReadRequest(bufio.NewReader(bytes.NewReader(data)))
-	if err == nil {
-		t.Fatal("Got unexpected request = %#v", r)
+// reqBytes treats req as a request (with \n delimiters) and returns it with \r\n delimiters,
+// ending in \r\n\r\n
+func reqBytes(req string) []byte {
+	return []byte(strings.Replace(strings.TrimSpace(req), "\n", "\r\n", -1) + "\r\n\r\n")
+}
+
+var badRequestTests = []struct {
+	name string
+	req  []byte
+}{
+	{"bad_connect_host", reqBytes("CONNECT []%20%48%54%54%50%2f%31%2e%31%0a%4d%79%48%65%61%64%65%72%3a%20%31%32%33%0a%0a HTTP/1.0")},
+	{"smuggle_two_contentlen", reqBytes(`POST / HTTP/1.1
+Content-Length: 3
+Content-Length: 4
+
+abc`)},
+	{"smuggle_chunked_and_len", reqBytes(`POST / HTTP/1.1
+Transfer-Encoding: chunked
+Content-Length: 3
+
+abc`)},
+	{"smuggle_content_len_head", reqBytes(`HEAD / HTTP/1.1
+Host: foo
+Content-Length: 5`)},
+}
+
+func TestReadRequest_Bad(t *testing.T) {
+	for _, tt := range badRequestTests {
+		got, err := ReadRequest(bufio.NewReader(bytes.NewReader(tt.req)))
+		if err == nil {
+			all, err := ioutil.ReadAll(got.Body)
+			t.Errorf("%s: got unexpected request = %#v\n  Body = %q, %v", tt.name, got, all, err)
+		}
 	}
 }

src/net/http/transfer.go

@@ -148,6 +148,9 @@ func (t *transferWriter) shouldSendContentLength() bool {
 		return true
 	}
 	if t.ContentLength == 0 && isIdentity(t.TransferEncoding) {
+		if t.Method == "GET" || t.Method == "HEAD" {
+			return false
+		}
 		return true
 	}
 
@@ -317,6 +320,7 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
 		}
 	case *Request:
 		t.Header = rr.Header
+		t.RequestMethod = rr.Method
 		t.ProtoMajor = rr.ProtoMajor
 		t.ProtoMinor = rr.ProtoMinor
 		// Transfer semantics for Requests are exactly like those for
@@ -333,7 +337,7 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
 	}
 
 	// Transfer encoding, content length
-	t.TransferEncoding, err = fixTransferEncoding(t.RequestMethod, t.Header)
+	t.TransferEncoding, err = fixTransferEncoding(isResponse, t.RequestMethod, t.Header)
 	if err != nil {
 		return err
 	}
@@ -421,12 +425,12 @@ func chunked(te []string) bool { return len(te) > 0 && te[0] == "chunked" }
 func isIdentity(te []string) bool { return len(te) == 1 && te[0] == "identity" }
 
 // Sanitize transfer encoding
-func fixTransferEncoding(requestMethod string, header Header) ([]string, error) {
+func fixTransferEncoding(isResponse bool, requestMethod string, header Header) ([]string, error) {
 	raw, present := header["Transfer-Encoding"]
 	if !present {
 		return nil, nil
 	}
-
+	isRequest := !isResponse
 	delete(header, "Transfer-Encoding")
 
 	encodings := strings.Split(raw[0], ",")
@@ -451,10 +455,15 @@ func fixTransferEncoding(requestMethod string, header Header) ([]string, error)
 		return nil, &badStringError{"too many transfer encodings", strings.Join(te, ",")}
 	}
 	if len(te) > 0 {
-		// Chunked encoding trumps Content-Length. See RFC 2616
-		// Section 4.4. Currently len(te) > 0 implies chunked
-		// encoding.
-		delete(header, "Content-Length")
+		// RFC 7230 3.3.2 says "A sender MUST NOT send a
+		// Content-Length header field in any message that
+		// contains a Transfer-Encoding header field."
+		if len(header["Content-Length"]) > 0 {
+			if isRequest {
+				return nil, errors.New("http: invalid Content-Length with Transfer-Encoding")
+			}
+			delete(header, "Content-Length")
+		}
 		return te, nil
 	}
 
@@ -465,9 +474,17 @@ func fixTransferEncoding(requestMethod string, header Header) ([]string, error)
 // function is not a method, because ultimately it should be shared by
 // ReadResponse and ReadRequest.
 func fixLength(isResponse bool, status int, requestMethod string, header Header, te []string) (int64, error) {
-
+	contentLens := header["Content-Length"]
+	isRequest := !isResponse
 	// Logic based on response type or status
 	if noBodyExpected(requestMethod) {
+		// For HTTP requests, as part of hardening against request
+		// smuggling (RFC 7230), don't allow a Content-Length header for
+		// methods which don't permit bodies. As an exception, allow
+		// exactly one Content-Length header if its value is "0".
+		if isRequest && len(contentLens) > 0 && !(len(contentLens) == 1 && contentLens[0] == "0") {
+			return 0, fmt.Errorf("http: method cannot contain a Content-Length; got %q", contentLens)
+		}
 		return 0, nil
 	}
 	if status/100 == 1 {
@@ -478,13 +495,21 @@ func fixLength(isResponse bool, status int, requestMethod string, header Header,
 		return 0, nil
 	}
 
+	if len(contentLens) > 1 {
+		// harden against HTTP request smuggling. See RFC 7230.
+		return 0, errors.New("http: message cannot contain multiple Content-Length headers")
+	}
+
 	// Logic based on Transfer-Encoding
 	if chunked(te) {
 		return -1, nil
 	}
 
 	// Logic based on Content-Length
-	cl := strings.TrimSpace(header.get("Content-Length"))
+	var cl string
+	if len(contentLens) == 1 {
+		cl = strings.TrimSpace(contentLens[0])
+	}
 	if cl != "" {
 		n, err := parseContentLength(cl)
 		if err != nil {
@@ -495,11 +520,14 @@ func fixLength(isResponse bool, status int, requestMethod string, header Header,
 		header.Del("Content-Length")
 	}
 
-	if !isResponse && requestMethod == "GET" {
-		// RFC 2616 doesn't explicitly permit nor forbid an
+	if !isResponse {
+		// RFC 2616 neither explicitly permits nor forbids an
 		// entity-body on a GET request so we permit one if
 		// declared, but we default to 0 here (not -1 below)
 		// if there's no mention of a body.
+		// Likewise, all other request methods are assumed to have
+		// no body if neither Transfer-Encoding chunked nor a
+		// Content-Length are set.
 		return 0, nil
 	}