From 5fa6923b1ea891400153d04ddf1545e23b40041b Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 28 Jun 2023 13:20:08 -0700 Subject: [PATCH] [release-branch.go1.19] net/http: validate Host header before sending Verify that the Host header we send is valid. Avoids surprising behavior such as a Host of "go.dev\r\nX-Evil:oops" adding an X-Evil header to HTTP/1 requests. Add a test, skip the test for HTTP/2. HTTP/2 is not vulnerable to header injection in the way HTTP/1 is, but x/net/http2 doesn't validate the header and will go into a retry loop when the server rejects it. CL 506995 adds the necessary validation to x/net/http2. Updates #60374 Fixes #61075 For CVE-2023-29406 Change-Id: I05cb6866a9bead043101954dfded199258c6dd04 Reviewed-on: https://go-review.googlesource.com/c/go/+/506996 Reviewed-by: Tatiana Bradley TryBot-Result: Gopher Robot Run-TryBot: Damien Neil (cherry picked from commit 499458f7ca04087958987a33c2703c3ef03e27e2) Reviewed-on: https://go-review.googlesource.com/c/go/+/507358 Run-TryBot: Tatiana Bradley Reviewed-by: Roland Shoemaker Upstream-Status: Backport [https://github.com/golang/go/commit/5fa6923b1ea891400153d04ddf1545e23b40041b] CVE: CVE-2023-29406 Signed-off-by: Vivek Kumbhar --- src/net/http/http_test.go | 29 --------------------- src/net/http/request.go | 47 ++++++++-------------------------- src/net/http/request_test.go | 11 ++------ src/net/http/transport_test.go | 18 +++++++++++++ 4 files changed, 31 insertions(+), 74 deletions(-) diff --git a/src/net/http/http_test.go b/src/net/http/http_test.go index f4ea52d..ea38cb4 100644 --- a/src/net/http/http_test.go +++ b/src/net/http/http_test.go @@ -49,35 +49,6 @@ func TestForeachHeaderElement(t *testing.T) { } } -func TestCleanHost(t *testing.T) { - tests := []struct { - in, want string - }{ - {"www.google.com", "www.google.com"}, - {"www.google.com foo", "www.google.com"}, - {"www.google.com/foo", "www.google.com"}, - {" first character is a space", ""}, - {"[1::6]:8080", "[1::6]:8080"}, - - // Punycode: - {"гофер.рф/foo", "xn--c1ae0ajs.xn--p1ai"}, - {"bücher.de", "xn--bcher-kva.de"}, - {"bücher.de:8080", "xn--bcher-kva.de:8080"}, - // Verify we convert to lowercase before punycode: - {"BÜCHER.de", "xn--bcher-kva.de"}, - {"BÜCHER.de:8080", "xn--bcher-kva.de:8080"}, - // Verify we normalize to NFC before punycode: - {"gophér.nfc", "xn--gophr-esa.nfc"}, // NFC input; no work needed - {"goph\u0065\u0301r.nfd", "xn--gophr-esa.nfd"}, // NFD input - } - for _, tt := range tests { - got := cleanHost(tt.in) - if tt.want != got { - t.Errorf("cleanHost(%q) = %q, want %q", tt.in, got, tt.want) - } - } -} - // Test that cmd/go doesn't link in the HTTP server. // // This catches accidental dependencies between the HTTP transport and diff --git a/src/net/http/request.go b/src/net/http/request.go index cb2edd2..2706300 100644 --- a/src/net/http/request.go +++ b/src/net/http/request.go @@ -18,7 +18,6 @@ import ( "io/ioutil" "mime" "mime/multipart" - "net" "net/http/httptrace" "net/textproto" "net/url" @@ -26,7 +25,8 @@ import ( "strconv" "strings" "sync" - + + "golang.org/x/net/http/httpguts" "golang.org/x/net/idna" ) @@ -557,12 +557,19 @@ func (r *Request) write(w io.Writer, usingProxy bool, extraHeaders Header, waitF // is not given, use the host from the request URL. // // Clean the host, in case it arrives with unexpected stuff in it. - host := cleanHost(r.Host) + host := r.Host if host == "" { if r.URL == nil { return errMissingHost } - host = cleanHost(r.URL.Host) + host = r.URL.Host + } + host, err = httpguts.PunycodeHostPort(host) + if err != nil { + return err + } + if !httpguts.ValidHostHeader(host) { + return errors.New("http: invalid Host header") } // According to RFC 6874, an HTTP client, proxy, or other @@ -717,38 +724,6 @@ func idnaASCII(v string) (string, error) { return idna.Lookup.ToASCII(v) } -// cleanHost cleans up the host sent in request's Host header. -// -// It both strips anything after '/' or ' ', and puts the value -// into Punycode form, if necessary. -// -// Ideally we'd clean the Host header according to the spec: -// https://tools.ietf.org/html/rfc7230#section-5.4 (Host = uri-host [ ":" port ]") -// https://tools.ietf.org/html/rfc7230#section-2.7 (uri-host -> rfc3986's host) -// https://tools.ietf.org/html/rfc3986#section-3.2.2 (definition of host) -// But practically, what we are trying to avoid is the situation in -// issue 11206, where a malformed Host header used in the proxy context -// would create a bad request. So it is enough to just truncate at the -// first offending character. -func cleanHost(in string) string { - if i := strings.IndexAny(in, " /"); i != -1 { - in = in[:i] - } - host, port, err := net.SplitHostPort(in) - if err != nil { // input was just a host - a, err := idnaASCII(in) - if err != nil { - return in // garbage in, garbage out - } - return a - } - a, err := idnaASCII(host) - if err != nil { - return in // garbage in, garbage out - } - return net.JoinHostPort(a, port) -} - // removeZone removes IPv6 zone identifier from host. // E.g., "[fe80::1%en0]:8080" to "[fe80::1]:8080" func removeZone(host string) string { diff --git a/src/net/http/request_test.go b/src/net/http/request_test.go index 461d66e..0d417ff 100644 --- a/src/net/http/request_test.go +++ b/src/net/http/request_test.go @@ -676,15 +676,8 @@ func TestRequestBadHost(t *testing.T) { } req.Host = "foo.com with spaces" req.URL.Host = "foo.com with spaces" - req.Write(logWrites{t, &got}) - want := []string{ - "GET /after HTTP/1.1\r\n", - "Host: foo.com\r\n", - "User-Agent: " + DefaultUserAgent + "\r\n", - "\r\n", - } - if !reflect.DeepEqual(got, want) { - t.Errorf("Writes = %q\n Want = %q", got, want) + if err := req.Write(logWrites{t, &got}); err == nil { + t.Errorf("Writing request with invalid Host: succeded, want error") } } diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go index fa0c370..0afb6b9 100644 --- a/src/net/http/transport_test.go +++ b/src/net/http/transport_test.go @@ -6249,3 +6249,21 @@ func TestIssue32441(t *testing.T) { t.Error(err) } } + +func TestRequestSanitization(t *testing.T) { + setParallel(t) + defer afterTest(t) + + ts := newClientServerTest(t, h1Mode, HandlerFunc(func(rw ResponseWriter, req *Request) { + if h, ok := req.Header["X-Evil"]; ok { + t.Errorf("request has X-Evil header: %q", h) + } + })).ts + defer ts.Close() + req, _ := NewRequest("GET", ts.URL, nil) + req.Host = "go.dev\r\nX-Evil:evil" + resp, _ := ts.Client().Do(req) + if resp != nil { + resp.Body.Close() + } +} -- 2.25.1