diff --git a/http2/server.go b/http2/server.go index 345b7cd8..02714ade 100644 --- a/http2/server.go +++ b/http2/server.go @@ -2020,7 +2020,11 @@ func (sc *serverConn) newWriterAndRequest(st *stream, f *MetaHeadersFrame) (*res } if bodyOpen { if vv, ok := rp.header["Content-Length"]; ok { - req.ContentLength, _ = strconv.ParseInt(vv[0], 10, 64) + if cl, err := strconv.ParseUint(vv[0], 10, 63); err == nil { + req.ContentLength = int64(cl) + } else { + req.ContentLength = 0 + } } else { req.ContentLength = -1 } @@ -2403,9 +2407,8 @@ func (rws *responseWriterState) writeChunk(p []byte) (n int, err error) { var ctype, clen string if clen = rws.snapHeader.Get("Content-Length"); clen != "" { rws.snapHeader.Del("Content-Length") - clen64, err := strconv.ParseInt(clen, 10, 64) - if err == nil && clen64 >= 0 { - rws.sentContentLen = clen64 + if cl, err := strconv.ParseUint(clen, 10, 63); err == nil { + rws.sentContentLen = int64(cl) } else { clen = "" } diff --git a/http2/server_test.go b/http2/server_test.go index 23c11882..fa473701 100644 --- a/http2/server_test.go +++ b/http2/server_test.go @@ -1755,6 +1755,117 @@ func TestServer_Response_NoData_Header_FooBar(t *testing.T) { }) } +// Reject content-length headers containing a sign. +// See https://golang.org/issue/39017 +func TestServerIgnoresContentLengthSignWhenWritingChunks(t *testing.T) { + tests := []struct { + name string + cl string + wantCL string + }{ + { + name: "proper content-length", + cl: "3", + wantCL: "3", + }, + { + name: "ignore cl with plus sign", + cl: "+3", + wantCL: "0", + }, + { + name: "ignore cl with minus sign", + cl: "-3", + wantCL: "0", + }, + { + name: "max int64, for safe uint64->int64 conversion", + cl: "9223372036854775807", + wantCL: "9223372036854775807", + }, + { + name: "overflows int64, so ignored", + cl: "9223372036854775808", + wantCL: "0", + }, + } + + for _, tt := range tests { + testServerResponse(t, func(w http.ResponseWriter, r *http.Request) error { + w.Header().Set("content-length", tt.cl) + return nil + }, func(st *serverTester) { + getSlash(st) + hf := st.wantHeaders() + goth := st.decodeHeader(hf.HeaderBlockFragment()) + wanth := [][2]string{ + {":status", "200"}, + {"content-length", tt.wantCL}, + } + if !reflect.DeepEqual(goth, wanth) { + t.Errorf("For case %q, value %q, got = %q; want %q", tt.name, tt.cl, goth, wanth) + } + }) + } +} + +// Reject content-length headers containing a sign. +// See https://golang.org/issue/39017 +func TestServerRejectsContentLengthWithSignNewRequests(t *testing.T) { + tests := []struct { + name string + cl string + wantCL int64 + }{ + { + name: "proper content-length", + cl: "3", + wantCL: 3, + }, + { + name: "ignore cl with plus sign", + cl: "+3", + wantCL: 0, + }, + { + name: "ignore cl with minus sign", + cl: "-3", + wantCL: 0, + }, + { + name: "max int64, for safe uint64->int64 conversion", + cl: "9223372036854775807", + wantCL: 9223372036854775807, + }, + { + name: "overflows int64, so ignored", + cl: "9223372036854775808", + wantCL: 0, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + writeReq := func(st *serverTester) { + st.writeHeaders(HeadersFrameParam{ + StreamID: 1, // clients send odd numbers + BlockFragment: st.encodeHeader("content-length", tt.cl), + EndStream: false, + EndHeaders: true, + }) + st.writeData(1, false, []byte("")) + } + checkReq := func(r *http.Request) { + if r.ContentLength != tt.wantCL { + t.Fatalf("Got: %q\nWant: %q", r.ContentLength, tt.wantCL) + } + } + testServerRequest(t, writeReq, checkReq) + }) + } +} + func TestServer_Response_Data_Sniff_DoesntOverride(t *testing.T) { const msg = "this is HTML." testServerResponse(t, func(w http.ResponseWriter, r *http.Request) error { diff --git a/http2/transport.go b/http2/transport.go index 2482f7bf..4ec32669 100644 --- a/http2/transport.go +++ b/http2/transport.go @@ -2006,8 +2006,8 @@ func (rl *clientConnReadLoop) handleResponse(cs *clientStream, f *MetaHeadersFra if !streamEnded || isHead { res.ContentLength = -1 if clens := res.Header["Content-Length"]; len(clens) == 1 { - if clen64, err := strconv.ParseInt(clens[0], 10, 64); err == nil { - res.ContentLength = clen64 + if cl, err := strconv.ParseUint(clens[0], 10, 63); err == nil { + res.ContentLength = int64(cl) } else { // TODO: care? unlike http/1, it won't mess up our framing, so it's // more safe smuggling-wise to ignore. diff --git a/http2/transport_test.go b/http2/transport_test.go index ec7493c7..2fdb117a 100644 --- a/http2/transport_test.go +++ b/http2/transport_test.go @@ -2255,6 +2255,69 @@ func TestTransportRejectsConnHeaders(t *testing.T) { } } +// Reject content-length headers containing a sign. +// See https://golang.org/issue/39017 +func TestTransportRejectsContentLengthWithSign(t *testing.T) { + tests := []struct { + name string + cl []string + wantCL string + }{ + { + name: "proper content-length", + cl: []string{"3"}, + wantCL: "3", + }, + { + name: "ignore cl with plus sign", + cl: []string{"+3"}, + wantCL: "", + }, + { + name: "ignore cl with minus sign", + cl: []string{"-3"}, + wantCL: "", + }, + { + name: "max int64, for safe uint64->int64 conversion", + cl: []string{"9223372036854775807"}, + wantCL: "9223372036854775807", + }, + { + name: "overflows int64, so ignored", + cl: []string{"9223372036854775808"}, + wantCL: "", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", tt.cl[0]) + }, optOnlyServer) + defer st.Close() + tr := &Transport{TLSClientConfig: tlsConfigInsecure} + defer tr.CloseIdleConnections() + + req, _ := http.NewRequest("HEAD", st.ts.URL, nil) + res, err := tr.RoundTrip(req) + + var got string + if err != nil { + got = fmt.Sprintf("ERROR: %v", err) + } else { + got = res.Header.Get("Content-Length") + res.Body.Close() + } + + if got != tt.wantCL { + t.Fatalf("Got: %q\nWant: %q", got, tt.wantCL) + } + }) + } +} + // golang.org/issue/14048 func TestTransportFailsOnInvalidHeaders(t *testing.T) { st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {