226 lines
7.1 KiB
Diff
226 lines
7.1 KiB
Diff
commit 300d9a21583e7cf0149a778a0611e76ff7c6680f
|
|
Author: Brad Fitzpatrick <bradfitz@golang.org>
|
|
Date: Tue Jun 30 14:21:15 2015 -0700
|
|
|
|
net/http: harden Server against request smuggling
|
|
|
|
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 <rsc@golang.org>
|
|
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
|
|
|
|
diff --git a/src/net/http/readrequest_test.go b/src/net/http/readrequest_test.go
|
|
index e930d99..1a3cf91 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) {
|
|
@@ -356,3 +383,39 @@ func TestReadRequest(t *testing.T) {
|
|
}
|
|
}
|
|
}
|
|
+
|
|
+// 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 5205003..3887604 100644
|
|
--- a/src/net/http/transfer.go
|
|
+++ b/src/net/http/transfer.go
|
|
@@ -143,6 +143,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
|
|
}
|
|
|
|
@@ -310,6 +313,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
|
|
@@ -325,7 +329,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
|
|
}
|
|
@@ -413,12 +417,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], ",")
|
|
@@ -443,10 +447,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
|
|
}
|
|
|
|
@@ -457,9 +466,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 {
|
|
@@ -470,13 +487,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 {
|
|
@@ -487,11 +512,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
|
|
}
|
|
|