net/http: only support "chunked" in inbound Transfer-Encoding headers

This is a security hardening measure against HTTP request smuggling.
Thank you to ZeddYu for reporting this issue.

We weren't parsing things correctly anyway, allowing "identity" to be
combined with "chunked", and ignoring any Transfer-Encoding header past
the first. This is a delicate security surface that already broke
before, just be strict and don't add complexity to support cases not
observed in the wild (nginx removed "identity" support [1] and multiple
TE header support [2]) and removed by RFC 7230 (see page 81).

It'd probably be good to also drop support for anything other than
"chunked" in outbound TE headers, as "identity" is not a thing anymore,
and we are probably off-spec for anything other than "chunked", but it
should not be a security concern, so leaving it for now. See #38867.

[1]: https://hg.nginx.org/nginx/rev/fe5976aae0e3
[2]: https://hg.nginx.org/nginx/rev/aca005d232ff

Change-Id: If17d0827f9c6167a0b19a158e2bc5844ec803288
Reviewed-on: https://go-review.googlesource.com/c/go/+/231418
Reviewed-by: Katie Hockman <katie@golang.org>

src/net/http/response_test.go

@@ -734,6 +734,7 @@ func TestReadResponseCloseInMiddle(t *testing.T) {
 }
 
 func diff(t *testing.T, prefix string, have, want interface{}) {
+	t.Helper()
 	hv := reflect.ValueOf(have).Elem()
 	wv := reflect.ValueOf(want).Elem()
 	if hv.Type() != wv.Type() {

src/net/http/serve_test.go

@@ -1347,37 +1347,6 @@ func TestServerAllowsBlockingRemoteAddr(t *testing.T) {
 	}
 }
 
-func TestIdentityResponseHeaders(t *testing.T) {
-	// Not parallel; changes log output.
-	defer afterTest(t)
-	log.SetOutput(ioutil.Discard) // is noisy otherwise
-	defer log.SetOutput(os.Stderr)
-
-	ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
-		w.Header().Set("Transfer-Encoding", "identity")
-		w.(Flusher).Flush()
-		fmt.Fprintf(w, "I am an identity response.")
-	}))
-	defer ts.Close()
-
-	c := ts.Client()
-	res, err := c.Get(ts.URL)
-	if err != nil {
-		t.Fatalf("Get error: %v", err)
-	}
-	defer res.Body.Close()
-
-	if g, e := res.TransferEncoding, []string(nil); !reflect.DeepEqual(g, e) {
-		t.Errorf("expected TransferEncoding of %v; got %v", e, g)
-	}
-	if _, haveCL := res.Header["Content-Length"]; haveCL {
-		t.Errorf("Unexpected Content-Length")
-	}
-	if !res.Close {
-		t.Errorf("expected Connection: close; got %v", res.Close)
-	}
-}
-
 // TestHeadResponses verifies that all MIME type sniffing and Content-Length
 // counting of GET requests also happens on HEAD requests.
 func TestHeadResponses_h1(t *testing.T) { testHeadResponses(t, h1Mode) }

src/net/http/transfer.go

@@ -425,11 +425,11 @@ type transferReader struct {
 	ProtoMajor    int
 	ProtoMinor    int
 	// Output
-	Body             io.ReadCloser
-	ContentLength    int64
-	TransferEncoding []string
-	Close            bool
-	Trailer          Header
+	Body          io.ReadCloser
+	ContentLength int64
+	Chunked       bool
+	Close         bool
+	Trailer       Header
 }
 
 func (t *transferReader) protoAtLeast(m, n int) bool {
@@ -501,13 +501,12 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
 		t.ProtoMajor, t.ProtoMinor = 1, 1
 	}
 
-	// Transfer encoding, content length
-	err = t.fixTransferEncoding()
-	if err != nil {
+	// Transfer-Encoding: chunked, and overriding Content-Length.
+	if err := t.parseTransferEncoding(); err != nil {
 		return err
 	}
 
-	realLength, err := fixLength(isResponse, t.StatusCode, t.RequestMethod, t.Header, t.TransferEncoding)
+	realLength, err := fixLength(isResponse, t.StatusCode, t.RequestMethod, t.Header, t.Chunked)
 	if err != nil {
 		return err
 	}
@@ -522,7 +521,7 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
 	}
 
 	// Trailer
-	t.Trailer, err = fixTrailer(t.Header, t.TransferEncoding)
+	t.Trailer, err = fixTrailer(t.Header, t.Chunked)
 	if err != nil {
 		return err
 	}
@@ -532,9 +531,7 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
 	// See RFC 7230, section 3.3.
 	switch msg.(type) {
 	case *Response:
-		if realLength == -1 &&
-			!chunked(t.TransferEncoding) &&
-			bodyAllowedForStatus(t.StatusCode) {
+		if realLength == -1 && !t.Chunked && bodyAllowedForStatus(t.StatusCode) {
 			// Unbounded body.
 			t.Close = true
 		}
@@ -543,7 +540,7 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
 	// Prepare body reader. ContentLength < 0 means chunked encoding
 	// or close connection when finished, since multipart is not supported yet
 	switch {
-	case chunked(t.TransferEncoding):
+	case t.Chunked:
 		if noResponseBodyExpected(t.RequestMethod) || !bodyAllowedForStatus(t.StatusCode) {
 			t.Body = NoBody
 		} else {
@@ -569,13 +566,17 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
 	case *Request:
 		rr.Body = t.Body
 		rr.ContentLength = t.ContentLength
-		rr.TransferEncoding = t.TransferEncoding
+		if t.Chunked {
+			rr.TransferEncoding = []string{"chunked"}
+		}
 		rr.Close = t.Close
 		rr.Trailer = t.Trailer
 	case *Response:
 		rr.Body = t.Body
 		rr.ContentLength = t.ContentLength
-		rr.TransferEncoding = t.TransferEncoding
+		if t.Chunked {
+			rr.TransferEncoding = []string{"chunked"}
+		}
 		rr.Close = t.Close
 		rr.Trailer = t.Trailer
 	}
@@ -605,8 +606,8 @@ func isUnsupportedTEError(err error) bool {
 	return ok
 }
 
-// fixTransferEncoding sanitizes t.TransferEncoding, if needed.
-func (t *transferReader) fixTransferEncoding() error {
+// parseTransferEncoding sets t.Chunked based on the Transfer-Encoding header.
+func (t *transferReader) parseTransferEncoding() error {
 	raw, present := t.Header["Transfer-Encoding"]
 	if !present {
 		return nil
@@ -618,56 +619,38 @@ func (t *transferReader) fixTransferEncoding() error {
 		return nil
 	}
 
-	encodings := strings.Split(raw[0], ",")
-	te := make([]string, 0, len(encodings))
-	// TODO: Even though we only support "identity" and "chunked"
-	// encodings, the loop below is designed with foresight. One
-	// invariant that must be maintained is that, if present,
-	// chunked encoding must always come first.
-	for _, encoding := range encodings {
-		encoding = strings.ToLower(strings.TrimSpace(encoding))
-		// "identity" encoding is not recorded
-		if encoding == "identity" {
-			break
-		}
-		if encoding != "chunked" {
-			return &unsupportedTEError{fmt.Sprintf("unsupported transfer encoding: %q", encoding)}
-		}
-		te = te[0 : len(te)+1]
-		te[len(te)-1] = encoding
+	// Like nginx, we only support a single Transfer-Encoding header field, and
+	// only if set to "chunked". This is one of the most security sensitive
+	// surfaces in HTTP/1.1 due to the risk of request smuggling, so we keep it
+	// strict and simple.
+	if len(raw) != 1 {
+		return &unsupportedTEError{fmt.Sprintf("too many transfer encodings: %q", raw)}
 	}
-	if len(te) > 1 {
-		return badStringError("too many transfer encodings", strings.Join(te, ","))
-	}
-	if len(te) > 0 {
-		// 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."
-		//
-		// 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(t.Header, "Content-Length")
-		t.TransferEncoding = te
-		return nil
+	if strings.ToLower(textproto.TrimString(raw[0])) != "chunked" {
+		return &unsupportedTEError{fmt.Sprintf("unsupported transfer encoding: %q", raw[0])}
 	}
 
+	// 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."
+	//
+	// 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(t.Header, "Content-Length")
+
+	t.Chunked = true
 	return nil
 }
 
 // Determine the expected body length, using RFC 7230 Section 3.3. This
 // function is not a method, because ultimately it should be shared by
 // ReadResponse and ReadRequest.
-func fixLength(isResponse bool, status int, requestMethod string, header Header, te []string) (int64, error) {
+func fixLength(isResponse bool, status int, requestMethod string, header Header, chunked bool) (int64, error) {
 	isRequest := !isResponse
 	contentLens := header["Content-Length"]
 
@@ -711,7 +694,7 @@ func fixLength(isResponse bool, status int, requestMethod string, header Header,
 	}
 
 	// Logic based on Transfer-Encoding
-	if chunked(te) {
+	if chunked {
 		return -1, nil
 	}
 
@@ -766,12 +749,12 @@ func shouldClose(major, minor int, header Header, removeCloseHeader bool) bool {
 }
 
 // Parse the trailer header
-func fixTrailer(header Header, te []string) (Header, error) {
+func fixTrailer(header Header, chunked bool) (Header, error) {
 	vv, ok := header["Trailer"]
 	if !ok {
 		return nil, nil
 	}
-	if !chunked(te) {
+	if !chunked {
 		// Trailer and no chunking:
 		// this is an invalid use case for trailer header.
 		// Nevertheless, no error will be returned and we

src/net/http/transfer_test.go

@@ -279,7 +279,7 @@ func TestTransferWriterWriteBodyReaderTypes(t *testing.T) {
 	}
 }
 
-func TestFixTransferEncoding(t *testing.T) {
+func TestParseTransferEncoding(t *testing.T) {
 	tests := []struct {
 		hdr     Header
 		wantErr error
@@ -290,7 +290,23 @@ func TestFixTransferEncoding(t *testing.T) {
 		},
 		{
 			hdr:     Header{"Transfer-Encoding": {"chunked, chunked", "identity", "chunked"}},
-			wantErr: badStringError("too many transfer encodings", "chunked,chunked"),
+			wantErr: &unsupportedTEError{`too many transfer encodings: ["chunked, chunked" "identity" "chunked"]`},
+		},
+		{
+			hdr:     Header{"Transfer-Encoding": {""}},
+			wantErr: &unsupportedTEError{`unsupported transfer encoding: ""`},
+		},
+		{
+			hdr:     Header{"Transfer-Encoding": {"chunked, identity"}},
+			wantErr: &unsupportedTEError{`unsupported transfer encoding: "chunked, identity"`},
+		},
+		{
+			hdr:     Header{"Transfer-Encoding": {"chunked", "identity"}},
+			wantErr: &unsupportedTEError{`too many transfer encodings: ["chunked" "identity"]`},
+		},
+		{
+			hdr:     Header{"Transfer-Encoding": {"\x0bchunked"}},
+			wantErr: &unsupportedTEError{`unsupported transfer encoding: "\vchunked"`},
 		},
 		{
 			hdr:     Header{"Transfer-Encoding": {"chunked"}},
@@ -304,7 +320,7 @@ func TestFixTransferEncoding(t *testing.T) {
 			ProtoMajor: 1,
 			ProtoMinor: 1,
 		}
-		gotErr := tr.fixTransferEncoding()
+		gotErr := tr.parseTransferEncoding()
 		if !reflect.DeepEqual(gotErr, tt.wantErr) {
 			t.Errorf("%d.\ngot error:\n%v\nwant error:\n%v\n\n", i, gotErr, tt.wantErr)
 		}