diff options
author | Brad Fitzpatrick <bradfitz@golang.org> | 2015-07-07 13:19:44 -0600 |
---|---|---|
committer | Brad Fitzpatrick <bradfitz@golang.org> | 2015-07-07 21:33:06 +0000 |
commit | 143822585e32449860e624cace9d2e521deee62e (patch) | |
tree | 7374e0184becab6a1307e0be500ff92edaeb05ff | |
parent | 7929a0ddfae27d66a6feb4d6fe069359fad613f1 (diff) | |
download | go-143822585e32449860e624cace9d2e521deee62e.tar.gz go-143822585e32449860e624cace9d2e521deee62e.zip |
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>
-rw-r--r-- | src/net/http/readrequest_test.go | 35 | ||||
-rw-r--r-- | src/net/http/transfer.go | 21 |
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 } |