Skip to content
Permalink
Browse files

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>
  • Loading branch information...
bradfitz committed Jul 7, 2015
1 parent 7929a0d commit 143822585e32449860e624cace9d2e521deee62e
Showing with 44 additions and 12 deletions.
  1. +30 −5 src/net/http/readrequest_test.go
  2. +14 −7 src/net/http/transfer.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
@@ -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
}

0 comments on commit 1438225

Please sign in to comment.
You can’t perform that action at this time.