Skip to content

Commit 48d899d

Browse files
panjf2000neild
authored andcommitted
net/http: reject requests with invalid Content-Length headers
According to RFC 9110 and RFC 9112, invalid "Content-Length" headers might involve request smuggling or response splitting, which could also cause security failures. Currently, `net/http` ignores all "Content-Length" headers when there is a "Transfer-Encoding" header and forward the message anyway while other mainstream HTTP implementations such as Apache Tomcat, Nginx, HAProxy, Node.js, Deno, Tornado, etc. reject invalid Content-Length headers regardless of the presence of a "Transfer-Encoding" header and only forward chunked-encoding messages with either valid "Content-Length" headers or no "Content-Length" headers. Fixes #65505 Change-Id: I73af2ee0785137e56c7546a4cce4a5c5c348dbc5 Reviewed-on: https://go-review.googlesource.com/c/go/+/561075 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Bryan Mills <[email protected]> Reviewed-by: Damien Neil <[email protected]>
1 parent d90a57f commit 48d899d

File tree

3 files changed

+45
-19
lines changed

3 files changed

+45
-19
lines changed

src/net/http/readrequest_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,22 @@ var reqTests = []reqTest{
207207
noError,
208208
},
209209

210+
// Tests chunked body and an invalid Content-Length.
211+
{
212+
"POST / HTTP/1.1\r\n" +
213+
"Host: foo.com\r\n" +
214+
"Transfer-Encoding: chunked\r\n" +
215+
"Content-Length: notdigits\r\n\r\n" + // raise an error
216+
"3\r\nfoo\r\n" +
217+
"3\r\nbar\r\n" +
218+
"0\r\n" +
219+
"\r\n",
220+
nil,
221+
noBodyStr,
222+
noTrailer,
223+
`bad Content-Length "notdigits"`,
224+
},
225+
210226
// CONNECT request with domain name:
211227
{
212228
"CONNECT www.google.com:443 HTTP/1.1\r\n\r\n",

src/net/http/serve_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4819,6 +4819,12 @@ func TestServerValidatesHeaders(t *testing.T) {
48194819
// See RFC 7230, Section 3.2.
48204820
{": empty key\r\n", 400},
48214821

4822+
// Requests with invalid Content-Length headers should be rejected
4823+
// regardless of the presence of a Transfer-Encoding header.
4824+
// Check out RFC 9110, Section 8.6 and RFC 9112, Section 6.3.3.
4825+
{"Content-Length: notdigits\r\n", 400},
4826+
{"Content-Length: notdigits\r\nTransfer-Encoding: chunked\r\n\r\n0\r\n\r\n", 400},
4827+
48224828
{"foo: foo foo\r\n", 200}, // LWS space is okay
48234829
{"foo: foo\tfoo\r\n", 200}, // LWS tab is okay
48244830
{"foo: foo\x00foo\r\n", 400}, // CTL 0x00 in value is bad

src/net/http/transfer.go

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -650,27 +650,14 @@ func (t *transferReader) parseTransferEncoding() error {
650650
return &unsupportedTEError{fmt.Sprintf("unsupported transfer encoding: %q", raw[0])}
651651
}
652652

653-
// RFC 7230 3.3.2 says "A sender MUST NOT send a Content-Length header field
654-
// in any message that contains a Transfer-Encoding header field."
655-
//
656-
// but also: "If a message is received with both a Transfer-Encoding and a
657-
// Content-Length header field, the Transfer-Encoding overrides the
658-
// Content-Length. Such a message might indicate an attempt to perform
659-
// request smuggling (Section 9.5) or response splitting (Section 9.4) and
660-
// ought to be handled as an error. A sender MUST remove the received
661-
// Content-Length field prior to forwarding such a message downstream."
662-
//
663-
// Reportedly, these appear in the wild.
664-
delete(t.Header, "Content-Length")
665-
666653
t.Chunked = true
667654
return nil
668655
}
669656

670657
// Determine the expected body length, using RFC 7230 Section 3.3. This
671658
// function is not a method, because ultimately it should be shared by
672659
// ReadResponse and ReadRequest.
673-
func fixLength(isResponse bool, status int, requestMethod string, header Header, chunked bool) (int64, error) {
660+
func fixLength(isResponse bool, status int, requestMethod string, header Header, chunked bool) (n int64, err error) {
674661
isRequest := !isResponse
675662
contentLens := header["Content-Length"]
676663

@@ -694,6 +681,14 @@ func fixLength(isResponse bool, status int, requestMethod string, header Header,
694681
contentLens = header["Content-Length"]
695682
}
696683

684+
// Reject requests with invalid Content-Length headers.
685+
if len(contentLens) > 0 {
686+
n, err = parseContentLength(contentLens)
687+
if err != nil {
688+
return -1, err
689+
}
690+
}
691+
697692
// Logic based on response type or status
698693
if isResponse && noResponseBodyExpected(requestMethod) {
699694
return 0, nil
@@ -706,17 +701,26 @@ func fixLength(isResponse bool, status int, requestMethod string, header Header,
706701
return 0, nil
707702
}
708703

704+
// According to RFC 9112, "If a message is received with both a
705+
// Transfer-Encoding and a Content-Length header field, the Transfer-Encoding
706+
// overrides the Content-Length. Such a message might indicate an attempt to
707+
// perform request smuggling (Section 11.2) or response splitting (Section 11.1)
708+
// and ought to be handled as an error. An intermediary that chooses to forward
709+
// the message MUST first remove the received Content-Length field and process
710+
// the Transfer-Encoding (as described below) prior to forwarding the message downstream."
711+
//
712+
// Chunked-encoding requests with either valid Content-Length
713+
// headers or no Content-Length headers are accepted after removing
714+
// the Content-Length field from header.
715+
//
709716
// Logic based on Transfer-Encoding
710717
if chunked {
718+
header.Del("Content-Length")
711719
return -1, nil
712720
}
713721

722+
// Logic based on Content-Length
714723
if len(contentLens) > 0 {
715-
// Logic based on Content-Length
716-
n, err := parseContentLength(contentLens)
717-
if err != nil {
718-
return -1, err
719-
}
720724
return n, nil
721725
}
722726

0 commit comments

Comments
 (0)