http2: validate client/outgoing trailers

odeke-em · gopherbot · commit 89f602b7bbf2 · 2024-03-19T21:21:07.000Z
This change is a counterpart to the HTTP/1.1 trailers validation CL 572615. This change will ensure that we validate trailers that were set on the HTTP client before they are transformed to HTTP/2 equivalents. Updates golang/go#64766 Change-Id: Id1afd7f7e9af820ea969ef226bbb16e4de6d57a5 Reviewed-on: https://go-review.googlesource.com/c/net/+/572655 Auto-Submit: Damien Neil <dneil@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Damien Neil <dneil@google.com> Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: David Chase <drchase@google.com>
diff --git a/http2/transport.go b/http2/transport.go
@@ -2019,6 +2019,22 @@ func (cs *clientStream) awaitFlowControl(maxBytes int) (taken int32, err error)
 	}
 }
 
+func validateHeaders(hdrs http.Header) string {
+	for k, vv := range hdrs {
+		if !httpguts.ValidHeaderFieldName(k) {
+			return fmt.Sprintf("name %q", k)
+		}
+		for _, v := range vv {
+			if !httpguts.ValidHeaderFieldValue(v) {
+				// Don't include the value in the error,
+				// because it may be sensitive.
+				return fmt.Sprintf("value for header %q", k)
+			}
+		}
+	}
+	return ""
+}
+
 var errNilRequestURL = errors.New("http2: Request.URI is nil")
 
 // requires cc.wmu be held.
@@ -2056,19 +2072,14 @@ func (cc *ClientConn) encodeHeaders(req *http.Request, addGzipHeader bool, trail
 		}
 	}
 
-	// Check for any invalid headers and return an error before we
+	// Check for any invalid headers+trailers and return an error before we
 	// potentially pollute our hpack state. (We want to be able to
 	// continue to reuse the hpack encoder for future requests)
-	for k, vv := range req.Header {
-		if !httpguts.ValidHeaderFieldName(k) {
-			return nil, fmt.Errorf("invalid HTTP header name %q", k)
-		}
-		for _, v := range vv {
-			if !httpguts.ValidHeaderFieldValue(v) {
-				// Don't include the value in the error, because it may be sensitive.
-				return nil, fmt.Errorf("invalid HTTP header value for header %q", k)
-			}
-		}
+	if err := validateHeaders(req.Header); err != "" {
+		return nil, fmt.Errorf("invalid HTTP header %s", err)
+	}
+	if err := validateHeaders(req.Trailer); err != "" {
+		return nil, fmt.Errorf("invalid HTTP trailer %s", err)
 	}
 
 	enumerateHeaders := func(f func(name, value string)) {
diff --git a/http2/transport_test.go b/http2/transport_test.go
@@ -2290,7 +2290,8 @@ func TestTransportRejectsContentLengthWithSign(t *testing.T) {
 }
 
 // golang.org/issue/14048
-func TestTransportFailsOnInvalidHeaders(t *testing.T) {
+// golang.org/issue/64766
+func TestTransportFailsOnInvalidHeadersAndTrailers(t *testing.T) {
 	st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
 		var got []string
 		for k := range r.Header {
@@ -2303,6 +2304,7 @@ func TestTransportFailsOnInvalidHeaders(t *testing.T) {
 
 	tests := [...]struct {
 		h       http.Header
+		t       http.Header
 		wantErr string
 	}{
 		0: {
@@ -2321,6 +2323,14 @@ func TestTransportFailsOnInvalidHeaders(t *testing.T) {
 			h:       http.Header{"foo": {"foo\x01bar"}},
 			wantErr: `invalid HTTP header value for header "foo"`,
 		},
+		4: {
+			t:       http.Header{"foo": {"foo\x01bar"}},
+			wantErr: `invalid HTTP trailer value for header "foo"`,
+		},
+		5: {
+			t:       http.Header{"x-\r\nda": {"foo\x01bar"}},
+			wantErr: `invalid HTTP trailer name "x-\r\nda"`,
+		},
 	}
 
 	tr := &Transport{TLSClientConfig: tlsConfigInsecure}
@@ -2329,6 +2339,7 @@ func TestTransportFailsOnInvalidHeaders(t *testing.T) {
 	for i, tt := range tests {
 		req, _ := http.NewRequest("GET", st.ts.URL, nil)
 		req.Header = tt.h
+		req.Trailer = tt.t
 		res, err := tr.RoundTrip(req)
 		var bad bool
 		if tt.wantErr == "" {
