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>

diff --git a/src/net/http/readrequest_test.go b/src/net/http/readrequest_test.go
index 1a3cf913bb39ecaedc50211044b59d2151c52fe1..60e2be41d177f9d0d874627ecc28f43d5cd4ffd7 100644 (file)
--- 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
index 3c868bd1320ef6c9bfae38bb7a87f17d98c1295c..fbbbf2417ace3a88245601ca61034f7f956d1671 100644 (file)
--- 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
        }