x/net/http2: reject HTTP/2 Content-Length headers containing a sign

Continuing the work of CL 234817, we enforce the same for HTTP/2 connections; that Content-Length header values with a sign (such as "+5") are rejected or ignored. In each place, the handling of such incorrect headers follows the approach already in place. Fixes golang/go#39017 Change-Id: Ie4854962dd0091795cb861d1cb3a78ce753fd3a8 Reviewed-on: https://go-review.googlesource.com/c/net/+/236098 Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
golang · Sep 4, 2020 · 62affa3 · 62affa3
1 parent c890458
commit 62affa3
Show file tree

Hide file tree

Showing 4 changed files with 183 additions and 6 deletions.
diff --git 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
@@ -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 = "<html>this is HTML."
 	testServerResponse(t, func(w http.ResponseWriter, r *http.Request) error {

diff --git 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
@@ -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) {