CVE-2015-5741: Inconsistent Interpretation of HTTP Requests ('HTTP Request/Response Smuggling') (CWE-444)
value: The net/http library in net/http/transfer.go in Go before 1.4.3 does not properly parse HTTP headers, which allows remote attackers to conduct HTTP request smuggling attacks via a request that contains Content-Length and Transfer-Encoding header fields.
CWE description: The product acts as an intermediary HTTP agent
         (such as a proxy or firewall) in the data flow between two
         entities such as a client and server, but it does not
         interpret malformed HTTP requests or responses in ways that
         are consistent with how the messages will be processed by
         those entities that are at the ultimate destination.
CVSS v3 base score: 9.8
Published: 2020-02-08T19:15Z
Repository: https://github.com/golang/go
Language: Go
Files changed: src/net/http/readrequest_test.go, src/net/http/transfer.go
Commit date: 2015-06-30 22:56:30 +0000
Commit message: 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/11810
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>

Diff: diff --git a/src/net/http/readrequest_test.go b/src/net/http/readrequest_test.go
index bda22c88ee..1a3cf913bb 100644
--- a/src/net/http/readrequest_test.go
+++ b/src/net/http/readrequest_test.go
@@ -7,10 +7,11 @@ package http
 import (
 	"bufio"
 	"bytes"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"net/url"
 	"reflect"
 	"strings"
 	"testing"
 )
@@ -321,10 +322,36 @@ var reqTests = []reqTest{
 
 		noBody,
 		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) {
 	for i := range reqTests {
 		tt := &reqTests[i]
@@ -355,12 +382,40 @@ func TestReadRequest(t *testing.T) {
 			t.Errorf("%s: Trailers differ.\n got: %v\nwant: %v", testName, req.Trailer, tt.Trailer)
 		}
 	}
 }
 
-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)
+		}
 	}
 }
diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go
index 0cd94eb16f..3c868bd132 100644
--- a/src/net/http/transfer.go
+++ b/src/net/http/transfer.go
@@ -146,10 +146,13 @@ func (t *transferWriter) shouldSendContentLength() bool {
 	// Many servers expect a Content-Length for these methods
 	if t.Method == "POST" || t.Method == "PUT" {
 		return true
 	}
 	if t.ContentLength == 0 && isIdentity(t.TransferEncoding) {
+		if t.Method == "GET" || t.Method == "HEAD" {
+			return false
+		}
 		return true
 	}
 
 	return false
 }
@@ -315,10 +318,11 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
 		if rr.Request != nil {
 			t.RequestMethod = rr.Request.Method
 		}
 	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
 		// Responses with status code 200, responding to a GET method
 		t.StatusCode = 200
@@ -331,11 +335,11 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
 	if t.ProtoMajor == 0 && t.ProtoMinor == 0 {
 		t.ProtoMajor, t.ProtoMinor = 1, 1
 	}
 
 	// 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
 	}
 
 	realLength, err := fixLength(isResponse, t.StatusCode, t.RequestMethod, t.Header, t.TransferEncoding)
@@ -419,16 +423,16 @@ func chunked(te []string) bool { return len(te) > 0 && te[0] == "chunked" }
 
 // Checks whether the encoding is explicitly "identity".
 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], ",")
 	te := make([]string, 0, len(encodings))
 	// TODO: Even though we only support "identity" and "chunked"
@@ -449,59 +453,83 @@ func fixTransferEncoding(requestMethod string, header Header) ([]string, error)
 	}
 	if len(te) > 1 {
 		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
 	}
 
 	return nil, nil
 }
 
 // Determine the expected body length, using RFC 2616 Section 4.4. This
 // 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 {
 		return 0, nil
 	}
 	switch status {
 	case 204, 304:
 		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 {
 			return -1, err
 		}
 		return n, nil
 	} else {
 		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
 	}
 
 	// Body-EOF logic based on other methods (like closing, or chunked coding)
 	return -1, nil

Vulnerable code: 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)
	t.TransferEncoding, err = fixTransferEncoding(t.RequestMethod, t.Header)
func fixTransferEncoding(requestMethod string, header Header) ([]string, error) {

		// Chunked encoding trumps Content-Length. See RFC 2616
		// Section 4.4. Currently len(te) > 0 implies chunked
		// encoding.
		delete(header, "Content-Length")

	cl := strings.TrimSpace(header.get("Content-Length"))
	if !isResponse && requestMethod == "GET" {
		// RFC 2616 doesn't explicitly permit nor forbid an
Fixed code: 	"io/ioutil"

	// 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,
	},
// 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)
		}
		if t.Method == "GET" || t.Method == "HEAD" {
			return false
		}
		t.RequestMethod = rr.Method
	t.TransferEncoding, err = fixTransferEncoding(isResponse, t.RequestMethod, t.Header)
func fixTransferEncoding(isResponse bool, requestMethod string, header Header) ([]string, error) {
	isRequest := !isResponse
		// 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")
		}
	contentLens := header["Content-Length"]
	isRequest := !isResponse
		// 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)
		}
	if len(contentLens) > 1 {
		// harden against HTTP request smuggling. See RFC 7230.
		return 0, errors.New("http: message cannot contain multiple Content-Length headers")
	}

	var cl string
	if len(contentLens) == 1 {
		cl = strings.TrimSpace(contentLens[0])
	}
	if !isResponse {
		// RFC 2616 neither explicitly permits nor forbids an
		// Likewise, all other request methods are assumed to have
		// no body if neither Transfer-Encoding chunked nor a
		// Content-Length are set.