Skip to content

Commit d1eba1d

Browse files
erikdubbelboererikdubbelboer
committed
Prevent chunk extension request smuggling
Improve fuzzers to make sure our handling of http requests and responses is the same as net/http.
1 parent 3471acf commit d1eba1d

File tree

5 files changed

+154
-33
lines changed

5 files changed

+154
-33
lines changed

‎fuzz_test.go‎

Lines changed: 73 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import (
66
"encoding/base64"
77
"encoding/binary"
88
"fmt"
9+
"io"
10+
"net/http"
911
"net/textproto"
1012
"net/url"
1113
"reflect"
@@ -67,10 +69,22 @@ func FuzzResponseReadLimitBody(f *testing.F) {
6769
return
6870
}
6971

70-
res := AcquireResponse()
71-
defer ReleaseResponse(res)
72+
t.Logf("%q %d", body, maxBodySize)
7273

73-
_ = res.ReadLimitBody(bufio.NewReader(bytes.NewReader(body)), maxBodySize)
74+
var res Response
75+
fastErr := res.ReadLimitBody(bufio.NewReader(bytes.NewReader(body)), maxBodySize)
76+
fastBody := res.Body()
77+
78+
netBody, netErr := readResponseBodyNetHTTPLimit(body, maxBodySize)
79+
if fastErr != nil {
80+
return
81+
}
82+
if netErr != nil {
83+
t.Fatalf("fasthttp:\n%s; net/http err=%v", res.String(), netErr)
84+
}
85+
if !bytes.Equal(fastBody, netBody) {
86+
t.Fatalf("body mismatch: fasthttp=%q net/http=%q", fastBody, netBody)
87+
}
7488
})
7589
}
7690

@@ -86,13 +100,66 @@ func FuzzRequestReadLimitBody(f *testing.F) {
86100
return
87101
}
88102

89-
req := AcquireRequest()
90-
defer ReleaseRequest(req)
103+
t.Logf("%q %d", body, maxBodySize)
104+
105+
var req Request
106+
fastErr := req.ReadLimitBody(bufio.NewReader(bytes.NewReader(body)), maxBodySize)
107+
fastBody := req.Body()
91108

92-
_ = req.ReadLimitBody(bufio.NewReader(bytes.NewReader(body)), maxBodySize)
109+
netBody, netErr := readRequestBodyNetHTTPLimit(body, maxBodySize)
110+
if fastErr != nil {
111+
return
112+
}
113+
if netErr != nil {
114+
if strings.Contains(netErr.Error(), "invalid URI for request") {
115+
return
116+
}
117+
t.Fatalf("fasthttp:\n%s; net/http err=%v", req.String(), netErr)
118+
}
119+
if !bytes.Equal(fastBody, netBody) {
120+
t.Fatalf("body mismatch: fasthttp=%q net/http=%q", fastBody, netBody)
121+
}
93122
})
94123
}
95124

125+
func readResponseBodyNetHTTPLimit(raw []byte, maxBodySize int) ([]byte, error) {
126+
br := bufio.NewReader(bytes.NewReader(raw))
127+
resp, err := http.ReadResponse(br, nil)
128+
if err != nil {
129+
return nil, err
130+
}
131+
defer resp.Body.Close()
132+
133+
return readBodyWithLimit(resp.Body, maxBodySize)
134+
}
135+
136+
func readRequestBodyNetHTTPLimit(raw []byte, maxBodySize int) ([]byte, error) {
137+
br := bufio.NewReader(bytes.NewReader(raw))
138+
req, err := http.ReadRequest(br)
139+
if err != nil {
140+
return nil, err
141+
}
142+
defer req.Body.Close()
143+
144+
return readBodyWithLimit(req.Body, maxBodySize)
145+
}
146+
147+
func readBodyWithLimit(r io.Reader, maxBodySize int) ([]byte, error) {
148+
if maxBodySize <= 0 {
149+
return io.ReadAll(r)
150+
}
151+
152+
limit := int64(maxBodySize) + 1
153+
body, err := io.ReadAll(io.LimitReader(r, limit))
154+
if err != nil {
155+
return nil, err
156+
}
157+
if len(body) > maxBodySize {
158+
return nil, ErrBodyTooLarge
159+
}
160+
return body, nil
161+
}
162+
96163
func FuzzURIUpdateBytes(f *testing.F) {
97164
f.Add([]byte(`http://foobar.com/aaa/bb?cc`))
98165
f.Add([]byte(`//foobar.com/aaa/bb?cc`))

‎header.go‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2790,7 +2790,22 @@ func (h *ResponseHeader) parseFirstLine(buf []byte) (int, error) {
27902790
}
27912791
return 0, fmt.Errorf("cannot find whitespace in the first line of response %q", buf)
27922792
}
2793-
h.noHTTP11 = !bytes.Equal(b[:n], strHTTP11)
2793+
if n == 0 {
2794+
if h.secureErrorLogMessage {
2795+
return 0, fmt.Errorf("unsupported HTTP version %q", b[:n])
2796+
}
2797+
return 0, fmt.Errorf("unsupported HTTP version %q in %q", b[:n], buf)
2798+
}
2799+
protoStr := b[:n]
2800+
if len(protoStr) != len(strHTTP11) || !bytes.HasPrefix(protoStr, strHTTP11[:5]) || protoStr[6] != '.' ||
2801+
protoStr[5] < '0' || protoStr[5] > '9' || protoStr[7] < '0' || protoStr[7] > '9' {
2802+
if h.secureErrorLogMessage {
2803+
return 0, fmt.Errorf("unsupported HTTP version %q", protoStr)
2804+
}
2805+
return 0, fmt.Errorf("unsupported HTTP version %q in %q", protoStr, buf)
2806+
}
2807+
h.noHTTP11 = !bytes.Equal(protoStr, strHTTP11)
2808+
h.protocol = append(h.protocol[:0], protoStr...)
27942809
b = b[n+1:]
27952810

27962811
// parse status code
@@ -2801,6 +2816,12 @@ func (h *ResponseHeader) parseFirstLine(buf []byte) (int, error) {
28012816
}
28022817
return 0, fmt.Errorf("cannot parse response status code: %w. Response %q", err, buf)
28032818
}
2819+
if n != 3 {
2820+
if h.secureErrorLogMessage {
2821+
return 0, ErrUnexpectedStatusCodeChar
2822+
}
2823+
return 0, fmt.Errorf("invalid response status code %q. Response %q", b[:n], buf)
2824+
}
28042825
if len(b) > n && b[n] != ' ' {
28052826
if h.secureErrorLogMessage {
28062827
return 0, ErrUnexpectedStatusCodeChar
@@ -2894,7 +2915,7 @@ func (h *RequestHeader) parseFirstLine(buf []byte) (int, error) {
28942915
}
28952916
return 0, fmt.Errorf("unsupported HTTP version %q in %q", protoStr, buf)
28962917
}
2897-
if protoStr[5] < '0' || protoStr[5] > '9' || protoStr[7] < '0' || protoStr[7] > '9' {
2918+
if protoStr[6] != '.' || protoStr[5] < '0' || protoStr[5] > '9' || protoStr[7] < '0' || protoStr[7] > '9' {
28982919
if h.secureErrorLogMessage {
28992920
return 0, fmt.Errorf("unsupported HTTP version %q", protoStr)
29002921
}

‎header_test.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1115,6 +1115,11 @@ func TestResponseHeaderOldVersion(t *testing.T) {
11151115
t.Fatalf("expecting 'Connection: close' for the response with old http protocol")
11161116
}
11171117

1118+
// Discard the body of the first response.
1119+
if n, err := br.Discard(h.ContentLength()); err != nil || n != h.ContentLength() {
1120+
t.Fatalf("unexpected discard: n=%d, want=%d, err=%v", n, h.ContentLength(), err)
1121+
}
1122+
11181123
if err := h.Read(br); err != nil {
11191124
t.Fatalf("unexpected error: %v", err)
11201125
}
@@ -1452,7 +1457,6 @@ func TestResponseHeaderHTTPVer(t *testing.T) {
14521457
// non-http/1.1
14531458
testResponseHeaderHTTPVer(t, "HTTP/1.0 200 OK\r\nContent-Type: aaa\r\nContent-Length: 123\r\n\r\n", true)
14541459
testResponseHeaderHTTPVer(t, "HTTP/0.9 200 OK\r\nContent-Type: aaa\r\nContent-Length: 123\r\n\r\n", true)
1455-
testResponseHeaderHTTPVer(t, "foobar 200 OK\r\nContent-Type: aaa\r\nContent-Length: 123\r\n\r\n", true)
14561460

14571461
// http/1.1
14581462
testResponseHeaderHTTPVer(t, "HTTP/1.1 200 OK\r\nContent-Type: aaa\r\nContent-Length: 123\r\n\r\n", false)

‎http.go‎

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1238,6 +1238,11 @@ func (req *Request) ReadLimitBody(r *bufio.Reader, maxBodySize int) error {
12381238
return err
12391239
}
12401240

1241+
// Host header is mandatory in HTTP/1.1 requests.
1242+
if req.Header.IsHTTP11() && len(req.Header.Host()) == 0 {
1243+
return errRequestHostRequired
1244+
}
1245+
12411246
return req.readLimitBody(r, maxBodySize, false, true)
12421247
}
12431248

@@ -1339,7 +1344,10 @@ func (req *Request) ContinueReadBody(r *bufio.Reader, maxBodySize int, preParseM
13391344

13401345
if contentLength == -1 {
13411346
err = req.Header.ReadTrailer(r)
1342-
if err != nil && err != io.EOF {
1347+
if err != nil {
1348+
if err == io.EOF {
1349+
return ErrBrokenChunk{error: io.ErrUnexpectedEOF}
1350+
}
13431351
return err
13441352
}
13451353
}
@@ -1477,7 +1485,10 @@ func (resp *Response) ReadLimitBody(r *bufio.Reader, maxBodySize int) error {
14771485
// A response without a body can't have trailers.
14781486
if resp.Header.ContentLength() == -1 && !resp.StreamBody && !resp.mustSkipBody() {
14791487
err = resp.Header.ReadTrailer(r)
1480-
if err != nil && err != io.EOF {
1488+
if err != nil {
1489+
if err == io.EOF {
1490+
return ErrBrokenChunk{error: io.ErrUnexpectedEOF}
1491+
}
14811492
return err
14821493
}
14831494
}
@@ -2614,31 +2625,43 @@ func parseChunkSize(r *bufio.Reader) (int, error) {
26142625
if err != nil {
26152626
return -1, err
26162627
}
2628+
inExt := false
26172629
for {
26182630
c, err := r.ReadByte()
26192631
if err != nil {
26202632
return -1, ErrBrokenChunk{
26212633
error: fmt.Errorf("cannot read '\\r' char at the end of chunk size: %w", err),
26222634
}
26232635
}
2624-
// Skip chunk extension after chunk size.
2625-
// Add support later if anyone needs it.
2626-
if c != '\r' {
2627-
// Security: Don't allow newlines in chunk extensions.
2628-
// This can lead to request smuggling issues with some reverse proxies.
2629-
if c == '\n' {
2636+
if c == '\r' {
2637+
if err := r.UnreadByte(); err != nil {
26302638
return -1, ErrBrokenChunk{
2631-
error: errors.New("invalid character '\\n' after chunk size"),
2639+
error: fmt.Errorf("cannot unread '\\r' char at the end of chunk size: %w", err),
26322640
}
26332641
}
2642+
break
2643+
}
2644+
// Security: Don't allow newlines in chunk extensions.
2645+
// This can lead to request smuggling issues with some reverse proxies.
2646+
if c == '\n' {
2647+
return -1, ErrBrokenChunk{
2648+
error: errors.New("invalid character '\\n' after chunk size"),
2649+
}
2650+
}
2651+
if inExt {
26342652
continue
26352653
}
2636-
if err := r.UnreadByte(); err != nil {
2654+
switch c {
2655+
case ' ', '\t':
2656+
continue
2657+
case ';':
2658+
inExt = true
2659+
continue
2660+
default:
26372661
return -1, ErrBrokenChunk{
2638-
error: fmt.Errorf("cannot unread '\\r' char at the end of chunk size: %w", err),
2662+
error: fmt.Errorf("invalid character %q after chunk size", c),
26392663
}
26402664
}
2641-
break
26422665
}
26432666
err = readCrLf(r)
26442667
if err != nil {

‎http_test.go‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,14 @@ import (
2121
func TestInvalidTrailers(t *testing.T) {
2222
t.Parallel()
2323

24-
if err := (&Response{}).Read(bufio.NewReader(strings.NewReader(" 0\nTransfer-Encoding:\xff\n\n0\r\n0"))); !errors.Is(err, io.EOF) {
25-
t.Fatalf("%#v", err)
24+
if err := (&Response{}).Read(bufio.NewReader(strings.NewReader("HTTP/1.1 200\r\nTransfer-Encoding:\xff\n\n0\r\n0"))); !errors.Is(err, io.EOF) {
25+
t.Errorf("%#v", err)
2626
}
27-
if err := (&Response{}).Read(bufio.NewReader(strings.NewReader("\xff \nTRaILeR:,\n\n"))); !errors.Is(err, errEmptyInt) {
28-
t.Fatal(err)
27+
if err := (&Response{}).Read(bufio.NewReader(strings.NewReader("HTTP/1.1 200 OK\r\nTRaILeR:,\r\n\r\n"))); !errors.Is(err, ErrBadTrailer) {
28+
t.Error(err)
2929
}
30-
if err := (&Response{}).Read(bufio.NewReader(strings.NewReader("TRaILeR:,\n\n"))); !strings.Contains(err.Error(), "cannot find whitespace in the first line of response") {
31-
t.Fatal(err)
30+
if err := (&Response{}).Read(bufio.NewReader(strings.NewReader("TRaILeR:,\r\n\r\n"))); !strings.Contains(err.Error(), "cannot find whitespace in the first line of response") {
31+
t.Error(err)
3232
}
3333
}
3434

@@ -586,7 +586,7 @@ func TestRequestContentTypeWithCharsetIssue100(t *testing.T) {
586586

587587
expectedContentType := "application/x-www-form-urlencoded; charset=UTF-8"
588588
expectedBody := "0123=56789"
589-
s := fmt.Sprintf("POST / HTTP/1.1\r\nContent-Type: %s\r\nContent-Length: %d\r\n\r\n%s",
589+
s := fmt.Sprintf("POST / HTTP/1.1\r\nHost: example.com\r\nContent-Type: %s\r\nContent-Length: %d\r\n\r\n%s",
590590
expectedContentType, len(expectedBody), expectedBody)
591591

592592
br := bufio.NewReader(bytes.NewBufferString(s))
@@ -1032,7 +1032,7 @@ func TestRequestReadNoBody(t *testing.T) {
10321032

10331033
var r Request
10341034

1035-
br := bufio.NewReader(bytes.NewBufferString("GET / HTTP/1.1\r\n\r\n"))
1035+
br := bufio.NewReader(bytes.NewBufferString("GET / HTTP/1.1\r\nHost: example.com\r\n\r\n"))
10361036
err := r.Read(br)
10371037
r.SetHost("foobar")
10381038
if err != nil {
@@ -1200,7 +1200,7 @@ func TestRequestReadGzippedBody(t *testing.T) {
12001200

12011201
bodyOriginal := "foo bar baz compress me better!"
12021202
body := AppendGzipBytes(nil, []byte(bodyOriginal))
1203-
s := fmt.Sprintf("POST /foobar HTTP/1.1\r\nContent-Type: foo/bar\r\nContent-Encoding: gzip\r\nContent-Length: %d\r\n\r\n%s",
1203+
s := fmt.Sprintf("POST /foobar HTTP/1.1\r\nHost: example.com\r\nContent-Type: foo/bar\r\nContent-Encoding: gzip\r\nContent-Length: %d\r\n\r\n%s",
12041204
len(body), body)
12051205
br := bufio.NewReader(bytes.NewBufferString(s))
12061206
if err := r.Read(br); err != nil {
@@ -1231,7 +1231,7 @@ func TestRequestReadPostNoBody(t *testing.T) {
12311231

12321232
var r Request
12331233

1234-
s := "POST /foo/bar HTTP/1.1\r\nContent-Type: aaa/bbb\r\n\r\naaaa"
1234+
s := "POST /foo/bar HTTP/1.1\r\nHost: example.com\r\nContent-Type: aaa/bbb\r\n\r\naaaa"
12351235
br := bufio.NewReader(bytes.NewBufferString(s))
12361236
if err := r.Read(br); err != nil {
12371237
t.Fatalf("unexpected error: %v", err)
@@ -1262,7 +1262,7 @@ func TestRequestReadPostNoBody(t *testing.T) {
12621262
func TestRequestContinueReadBody(t *testing.T) {
12631263
t.Parallel()
12641264

1265-
s := "PUT /foo/bar HTTP/1.1\r\nExpect: 100-continue\r\nContent-Length: 5\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
1265+
s := "PUT /foo/bar HTTP/1.1\r\nHost: example.org\r\nExpect: 100-continue\r\nContent-Length: 5\r\nContent-Type: foo/bar\r\n\r\nabcdef4343"
12661266
br := bufio.NewReader(bytes.NewBufferString(s))
12671267

12681268
var r Request
@@ -2034,7 +2034,7 @@ func TestResponseReadWithoutBody(t *testing.T) {
20342034
testResponseReadWithoutBody(t, &resp, "HTTP/1.1 123 AAA\r\nContent-Type: xxx\r\nContent-Length: 3434\r\n\r\n", false,
20352035
123, 3434, "xxx")
20362036

2037-
testResponseReadWithoutBody(t, &resp, "HTTP 200 OK\r\nContent-Type: text/xml\r\nContent-Length: 123\r\n\r\nfoobar\r\n", true,
2037+
testResponseReadWithoutBody(t, &resp, "HTTP/1.1 200 OK\r\nContent-Type: text/xml\r\nContent-Length: 123\r\n\r\nfoobar\r\n", true,
20382038
200, 123, "text/xml")
20392039

20402040
// '100 Continue' must be skipped.
@@ -3120,6 +3120,12 @@ func TestRequestMultipartFormPipeEmptyFormField(t *testing.T) {
31203120
if err != nil {
31213121
t.Fatalf("unexpected error: %v", err)
31223122
}
3123+
if _, err = bw.Write(strCRLF); err != nil {
3124+
t.Fatalf("unexpected error: %v", err)
3125+
}
3126+
if err = bw.Flush(); err != nil {
3127+
t.Fatalf("unexpected error: %v", err)
3128+
}
31233129

31243130
for e := range errs {
31253131
t.Fatalf("unexpected error in goroutine multiwriter: %v", e)

0 commit comments

Comments
 (0)