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>
diff --git a/http2/server.go b/http2/server.go
index 345b7cd..02714ad 100644
--- a/http2/server.go
+++ b/http2/server.go
@@ -2020,7 +2020,11 @@
}
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 @@
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 23c1188..fa47370 100644
--- a/http2/server_test.go
+++ b/http2/server_test.go
@@ -1755,6 +1755,117 @@
})
}
+// 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
index 2482f7b..4ec3266 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -2006,8 +2006,8 @@
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 ec7493c..2fdb117 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -2255,6 +2255,69 @@
}
}
+// 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) {
