]> Cypherpunks.ru repositories - gostls13.git/commitdiff
net/http: only support "chunked" in inbound Transfer-Encoding headers
authorFilippo Valsorda <filippo@golang.org>
Fri, 1 May 2020 04:58:55 +0000 (00:58 -0400)
committerFilippo Valsorda <filippo@golang.org>
Wed, 6 May 2020 16:25:30 +0000 (16:25 +0000)
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
src/net/http/serve_test.go
src/net/http/transfer.go
src/net/http/transfer_test.go

index 0c78df6f3fdc8eb40127033fc2c8c9c39e8cae0a..ce872606b1492096a770f92dead67da00cad0cbc 100644 (file)
@@ -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() {
index 49f6941223471a2b937c123ce596505b52e6f759..5f5693277828b3d30e7c7e56dc08b6394ef2f6f7 100644 (file)
@@ -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) }
index 960f8ac565e01ad6aafbdc08f511fd303e5a7490..350403c3667bb6250d41b9238c9ac11466201d49 100644 (file)
@@ -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
-       }
-       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
+       // 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 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
index a6846f7dcbb748dd83eec1854dad7ff22071c8a6..e27d34dd7812a3bf5adb0360f373baacd489c70a 100644 (file)
@@ -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)
                }