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>
golang · Jul 7, 2015 · 1438225 · 1438225
1 parent 7929a0d
commit 1438225
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 12 deletions.
diff --git 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",
@@ -399,11 +429,6 @@ var badRequestTests = []struct {
 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

diff --git 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
 	}