Skip to content

Commit

Permalink
net/http: harden Server against request smuggling
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@bradfitz
bradfitz committed Jun 30, 2015
1 parent 2714005 commit 300d9a2
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 16 deletions.
65 changes: 60 additions & 5 deletions src/net/http/readrequest_test.go
View file
Expand Up @@ -9,6 +9,7 @@ import (
"bytes"
"fmt"
"io"
"io/ioutil"
"net/url"
"reflect"
"strings"
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
}
}
50 changes: 39 additions & 11 deletions src/net/http/transfer.go
View file
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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], ",")
Expand All @@ -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
}

Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
}

Expand Down

0 comments on commit 300d9a2

Please sign in to comment.