net/http: validate trailers when writing requests and responses When writing an HTTP/1 request or response, trailer names were copied verbatim onto the "Trailer:" declaration line by writeHeader. The only prior filtering was a switch rejecting the three reserved keys, and CanonicalHeaderKey returns a key containing invalid bytes unchanged, so a trailer name such as "X-Trailer\r\nInjected: 1" was written to the wire as written, permitting header injection on the Trailer line. Validate trailer names and values in newTransferWriter, which is the shared HTTP/1 serialization path for Request.Write and Response.Write, before any trailer is written. Names that fail httpguts.ValidHeaderFieldName and values that fail httpguts.ValidHeaderFieldValue now cause the write to return an error. This path is distinct from Transport.roundTrip, which already validates req.Trailer for client requests, and from the HTTP/2 and server ResponseWriter trailer paths, which validate separately. The gap closed here is the direct Request.Write / Response.Write serialization used by callers such as httputil and code that writes a request to a connection directly. Rejecting invalid trailer values is stricter than the previous behavior, in which the final trailer block sanitized CR and LF to spaces via Header.writeSubset rather than erroring. Values whose final value is set while the request body is read are still written through that sanitizing path; the new check covers values present when the write begins. Fixes #78775 Change-Id: I3ed66185ab262a215d0c8bbba005fd8b3df69acb GitHub-Last-Rev: 9337c9d3f207fe8080e2dfd6b7f99971a7c56f33 GitHub-Pull-Request: golang/go#79971 Reviewed-on: https://go-review.googlesource.com/c/go/+/789960 Reviewed-by: Nicholas Husin <husin@google.com> Auto-Submit: Emmanuel Odeke <emmanuel@orijtech.com> Reviewed-by: Sayer Turner <sayerturner65@gmail.com> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Reviewed-by: Nicholas Husin <nsh@golang.org> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
diff --git a/src/net/http/request.go b/src/net/http/request.go index eb6e598..a95e991 100644 --- a/src/net/http/request.go +++ b/src/net/http/request.go
@@ -279,6 +279,10 @@ // the request body is read. Once the body returns EOF, the caller must // not mutate Trailer. // + // Writing a request whose Trailer contains a key with invalid bytes + // (such as CR or LF), or such a value present when Write begins, + // returns an error. + // // Few HTTP clients, servers, or proxies support HTTP trailers. Trailer Header
diff --git a/src/net/http/requestwrite_test.go b/src/net/http/requestwrite_test.go index 8b097cd..8903f2c 100644 --- a/src/net/http/requestwrite_test.go +++ b/src/net/http/requestwrite_test.go
@@ -608,6 +608,121 @@ "User-Agent: Go-http-client/1.1\r\n" + "Content-Length: 0\r\n\r\n", }, + + // Valid Trailer keeps working after trailer validation. Issue #78775 + 27: { + Req: Request{ + Method: "POST", + URL: &url.URL{ + Scheme: "http", + Host: "example.com", + Path: "/", + }, + ProtoMajor: 1, + ProtoMinor: 1, + Header: Header{}, + TransferEncoding: []string{"chunked"}, + Trailer: Header{"X-Trailer": {"ok"}}, + }, + + Body: []byte("abcdef"), + + WantWrite: "POST / HTTP/1.1\r\n" + + "Host: example.com\r\n" + + "User-Agent: Go-http-client/1.1\r\n" + + "Transfer-Encoding: chunked\r\n" + + "Trailer: X-Trailer\r\n\r\n" + + chunk("abcdef") + + "0\r\n" + + "X-Trailer: ok\r\n" + + "\r\n", + }, + + // Trailer names with control characters must not reach the wire, + // where they would permit header injection on the "Trailer:" line. + // Issue #78775 + 28: { + Req: Request{ + Method: "POST", + URL: &url.URL{ + Scheme: "http", + Host: "example.com", + Path: "/", + }, + ProtoMajor: 1, + ProtoMinor: 1, + Header: Header{}, + TransferEncoding: []string{"chunked"}, + Trailer: Header{"X-Trailer\r\nInjected: 1": {"ok"}}, + }, + + Body: []byte("abcdef"), + + WantError: errors.New(`net/http: invalid trailer field name "X-Trailer\r\nInjected: 1"`), + }, + + // Trailer values with control characters are rejected as well. Issue #78775 + 29: { + Req: Request{ + Method: "POST", + URL: &url.URL{ + Scheme: "http", + Host: "example.com", + Path: "/", + }, + ProtoMajor: 1, + ProtoMinor: 1, + Header: Header{}, + TransferEncoding: []string{"chunked"}, + Trailer: Header{"X-Trailer": {"evil\r\nInjected: 1"}}, + }, + + Body: []byte("abcdef"), + + WantError: errors.New(`net/http: invalid trailer field value for "X-Trailer"`), + }, + + // An empty Trailer name is rejected. Issue #78775 + 30: { + Req: Request{ + Method: "POST", + URL: &url.URL{ + Scheme: "http", + Host: "example.com", + Path: "/", + }, + ProtoMajor: 1, + ProtoMinor: 1, + Header: Header{}, + TransferEncoding: []string{"chunked"}, + Trailer: Header{"": {"ok"}}, + }, + + Body: []byte("abcdef"), + + WantError: errors.New(`net/http: invalid trailer field name ""`), + }, + + // A later (non-first) value in a Trailer is validated too. Issue #78775 + 31: { + Req: Request{ + Method: "POST", + URL: &url.URL{ + Scheme: "http", + Host: "example.com", + Path: "/", + }, + ProtoMajor: 1, + ProtoMinor: 1, + Header: Header{}, + TransferEncoding: []string{"chunked"}, + Trailer: Header{"X-Trailer": {"ok", "evil\r\nInjected: 1"}}, + }, + + Body: []byte("abcdef"), + + WantError: errors.New(`net/http: invalid trailer field value for "X-Trailer"`), + }, } func TestRequestWrite(t *testing.T) {
diff --git a/src/net/http/responsewrite_test.go b/src/net/http/responsewrite_test.go index 226ad72..84e0f3b 100644 --- a/src/net/http/responsewrite_test.go +++ b/src/net/http/responsewrite_test.go
@@ -288,3 +288,45 @@ } } } + +// Response.Write shares the trailer validation added for Issue #78775 with +// Request.Write, so an invalid trailer name or value must be rejected rather +// than written. +func TestResponseWriteInvalidTrailer(t *testing.T) { + tests := []struct { + name string + trailer Header + wantErr string + }{ + { + name: "key", + trailer: Header{"X-Trailer\r\nInjected: 1": {"ok"}}, + wantErr: `net/http: invalid trailer field name "X-Trailer\r\nInjected: 1"`, + }, + { + name: "value", + trailer: Header{"X-Trailer": {"evil\r\nInjected: 1"}}, + wantErr: `net/http: invalid trailer field value for "X-Trailer"`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resp := Response{ + StatusCode: 200, + ProtoMajor: 1, + ProtoMinor: 1, + Request: dummyReq("GET"), + Header: Header{}, + Body: io.NopCloser(strings.NewReader("abcdef")), + ContentLength: -1, + TransferEncoding: []string{"chunked"}, + Trailer: tt.trailer, + } + var b strings.Builder + err := resp.Write(&b) + if err == nil || err.Error() != tt.wantErr { + t.Fatalf("Response.Write error = %v, want %q", err, tt.wantErr) + } + }) + } +}
diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go index 6755512..faa1c38 100644 --- a/src/net/http/transfer.go +++ b/src/net/http/transfer.go
@@ -146,6 +146,13 @@ t.Trailer = nil } + // Validate Trailer names and values. The names are later written + // unmodified on the "Trailer:" line of the header, so invalid bytes + // (in particular CR and LF) would permit header injection. (Issue 78775.) + if err := validateHeaders(t.Trailer); err != "" { + return nil, fmt.Errorf("net/http: invalid trailer %s", err) + } + return t, nil }