net/http: revert overly-strict part of earlier smuggling defense

The recent https://golang.org/cl/11810 is reportedly a bit too
aggressive.

Apparently some HTTP requests in the wild do contain both a
Transfer-Encoding along with a bogus Content-Length. Instead of
returning a 400 Bad Request error, we should just ignore the
Content-Length like we did before.

Change-Id: I0001be90d09f8293a34f04691f608342875ff5c4
Reviewed-on: https://go-review.googlesource.com/11962
Reviewed-by: Andrew Gerrand <adg@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

2 files changed, 44 insertions, 12 deletions

diff --git a/src/net/http/readrequest_test.go b/src/net/http/readrequest_test.go
index 1a3cf913bb..60e2be41d1 100644
--- a/src/net/http/readrequest_test.go
+++ b/src/net/http/readrequest_test.go
@@ -178,6 +178,36 @@ var reqTests = []reqTest{
 		noError,
 	},
 
+	// Tests chunked body and a bogus Content-Length which should be deleted.
+	{
+		"POST / HTTP/1.1\r\n" +
+			"Host: foo.com\r\n" +
+			"Transfer-Encoding: chunked\r\n" +
+			"Content-Length: 9999\r\n\r\n" + // to be removed.
+			"3\r\nfoo\r\n" +
+			"3\r\nbar\r\n" +
+			"0\r\n" +
+			"\r\n",
+		&Request{
+			Method: "POST",
+			URL: &url.URL{
+				Path: "/",
+			},
+			TransferEncoding: []string{"chunked"},
+			Proto:            "HTTP/1.1",
+			ProtoMajor:       1,
+			ProtoMinor:       1,
+			Header:           Header{},
+			ContentLength:    -1,
+			Host:             "foo.com",
+			RequestURI:       "/",
+		},
+
+		"foobar",
+		noTrailer,
+		noError,
+	},
+
 	// CONNECT request with domain name:
 	{
 		"CONNECT www.google.com:443 HTTP/1.1\r\n\r\n",
@@ -400,11 +430,6 @@ 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`)},
diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go
index 3c868bd132..fbbbf2417a 100644
--- a/src/net/http/transfer.go
+++ b/src/net/http/transfer.go
@@ -430,7 +430,6 @@ func fixTransferEncoding(isResponse bool, requestMethod string, header Header) (
 	if !present {
 		return nil, nil
 	}
-	isRequest := !isResponse
 	delete(header, "Transfer-Encoding")
 
 	encodings := strings.Split(raw[0], ",")
@@ -458,12 +457,20 @@ func fixTransferEncoding(isResponse bool, requestMethod string, header Header) (
 		// 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")
-		}
+		//
+		// but also:
+		// "If a message is received with both a
+		// Transfer-Encoding and a Content-Length header
+		// field, the Transfer-Encoding overrides the
+		// Content-Length. Such a message might indicate an
+		// attempt to perform request smuggling (Section 9.5)
+		// or response splitting (Section 9.4) and ought to be
+		// handled as an error. A sender MUST remove the
+		// received Content-Length field prior to forwarding
+		// such a message downstream."
+		//
+		// Reportedly, these appear in the wild.
+		delete(header, "Content-Length")
 		return te, nil
 	}