From 300d9a21583e7cf0149a778a0611e76ff7c6680f Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 30 Jun 2015 14:21:15 -0700 Subject: [PATCH] net/http: harden Server against request smuggling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See RFC 7230. Thanks to Régis Leroy for the report. Change-Id: Ic1779bc2180900430d4d7a4938cac04ed73c304c Reviewed-on: https://go-review.googlesource.com/11810 Reviewed-by: Russ Cox Run-TryBot: Brad Fitzpatrick --- src/net/http/readrequest_test.go | 65 +++++++++++++++++++++++++++++--- src/net/http/transfer.go | 50 ++++++++++++++++++------ 2 files changed, 99 insertions(+), 16 deletions(-) diff --git a/src/net/http/readrequest_test.go b/src/net/http/readrequest_test.go index bda22c88eebf0..1a3cf913bb39e 100644 --- a/src/net/http/readrequest_test.go +++ b/src/net/http/readrequest_test.go @@ -9,6 +9,7 @@ import ( "bytes" "fmt" "io" + "io/ioutil" "net/url" "reflect" "strings" @@ -323,6 +324,32 @@ var reqTests = []reqTest{ noTrailer, noError, }, + + // HEAD with Content-Length 0. Make sure this is permitted, + // since I think we used to send it. + { + "HEAD / HTTP/1.1\r\nHost: issue8261.com\r\nConnection: close\r\nContent-Length: 0\r\n\r\n", + &Request{ + Method: "HEAD", + URL: &url.URL{ + Path: "/", + }, + Header: Header{ + "Connection": []string{"close"}, + "Content-Length": []string{"0"}, + }, + Host: "issue8261.com", + Proto: "HTTP/1.1", + ProtoMajor: 1, + ProtoMinor: 1, + Close: true, + RequestURI: "/", + }, + + noBody, + noTrailer, + noError, + }, } func TestReadRequest(t *testing.T) { @@ -357,10 +384,38 @@ func TestReadRequest(t *testing.T) { } } -func TestReadRequest_BadConnectHost(t *testing.T) { - data := []byte("CONNECT []%20%48%54%54%50%2f%31%2e%31%0a%4d%79%48%65%61%64%65%72%3a%20%31%32%33%0a%0a HTTP/1.0\n\n") - r, err := ReadRequest(bufio.NewReader(bytes.NewReader(data))) - if err == nil { - t.Fatal("Got unexpected request = %#v", r) +// reqBytes treats req as a request (with \n delimiters) and returns it with \r\n delimiters, +// ending in \r\n\r\n +func reqBytes(req string) []byte { + return []byte(strings.Replace(strings.TrimSpace(req), "\n", "\r\n", -1) + "\r\n\r\n") +} + +var badRequestTests = []struct { + name string + req []byte +}{ + {"bad_connect_host", reqBytes("CONNECT []%20%48%54%54%50%2f%31%2e%31%0a%4d%79%48%65%61%64%65%72%3a%20%31%32%33%0a%0a HTTP/1.0")}, + {"smuggle_two_contentlen", reqBytes(`POST / HTTP/1.1 +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`)}, +} + +func TestReadRequest_Bad(t *testing.T) { + for _, tt := range badRequestTests { + got, err := ReadRequest(bufio.NewReader(bytes.NewReader(tt.req))) + if err == nil { + all, err := ioutil.ReadAll(got.Body) + t.Errorf("%s: got unexpected request = %#v\n Body = %q, %v", tt.name, got, all, err) + } } } diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go index 0cd94eb16f87c..3c868bd1320ef 100644 --- a/src/net/http/transfer.go +++ b/src/net/http/transfer.go @@ -148,6 +148,9 @@ func (t *transferWriter) shouldSendContentLength() bool { return true } if t.ContentLength == 0 && isIdentity(t.TransferEncoding) { + if t.Method == "GET" || t.Method == "HEAD" { + return false + } return true } @@ -317,6 +320,7 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) { } case *Request: t.Header = rr.Header + t.RequestMethod = rr.Method t.ProtoMajor = rr.ProtoMajor t.ProtoMinor = rr.ProtoMinor // Transfer semantics for Requests are exactly like those for @@ -333,7 +337,7 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) { } // Transfer encoding, content length - t.TransferEncoding, err = fixTransferEncoding(t.RequestMethod, t.Header) + t.TransferEncoding, err = fixTransferEncoding(isResponse, t.RequestMethod, t.Header) if err != nil { return err } @@ -421,12 +425,12 @@ func chunked(te []string) bool { return len(te) > 0 && te[0] == "chunked" } func isIdentity(te []string) bool { return len(te) == 1 && te[0] == "identity" } // Sanitize transfer encoding -func fixTransferEncoding(requestMethod string, header Header) ([]string, error) { +func fixTransferEncoding(isResponse bool, requestMethod string, header Header) ([]string, error) { raw, present := header["Transfer-Encoding"] if !present { return nil, nil } - + isRequest := !isResponse delete(header, "Transfer-Encoding") encodings := strings.Split(raw[0], ",") @@ -451,10 +455,15 @@ func fixTransferEncoding(requestMethod string, header Header) ([]string, error) return nil, &badStringError{"too many transfer encodings", strings.Join(te, ",")} } if len(te) > 0 { - // Chunked encoding trumps Content-Length. See RFC 2616 - // Section 4.4. Currently len(te) > 0 implies chunked - // encoding. - delete(header, "Content-Length") + // 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") + } return te, nil } @@ -465,9 +474,17 @@ func fixTransferEncoding(requestMethod string, header Header) ([]string, error) // 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) { - + contentLens := header["Content-Length"] + isRequest := !isResponse // Logic based on response type or status if noBodyExpected(requestMethod) { + // For HTTP requests, as part of hardening against request + // smuggling (RFC 7230), don't allow a Content-Length header for + // methods which don't permit bodies. As an exception, allow + // exactly one Content-Length header if its value is "0". + if isRequest && len(contentLens) > 0 && !(len(contentLens) == 1 && contentLens[0] == "0") { + return 0, fmt.Errorf("http: method cannot contain a Content-Length; got %q", contentLens) + } return 0, nil } if status/100 == 1 { @@ -478,13 +495,21 @@ func fixLength(isResponse bool, status int, requestMethod string, header Header, return 0, nil } + if len(contentLens) > 1 { + // harden against HTTP request smuggling. See RFC 7230. + return 0, errors.New("http: message cannot contain multiple Content-Length headers") + } + // Logic based on Transfer-Encoding if chunked(te) { return -1, nil } // Logic based on Content-Length - cl := strings.TrimSpace(header.get("Content-Length")) + var cl string + if len(contentLens) == 1 { + cl = strings.TrimSpace(contentLens[0]) + } if cl != "" { n, err := parseContentLength(cl) if err != nil { @@ -495,11 +520,14 @@ func fixLength(isResponse bool, status int, requestMethod string, header Header, header.Del("Content-Length") } - if !isResponse && requestMethod == "GET" { - // RFC 2616 doesn't explicitly permit nor forbid an + if !isResponse { + // RFC 2616 neither explicitly permits nor forbids an // entity-body on a GET request so we permit one if // declared, but we default to 0 here (not -1 below) // if there's no mention of a body. + // Likewise, all other request methods are assumed to have + // no body if neither Transfer-Encoding chunked nor a + // Content-Length are set. return 0, nil }