net/textproto: don't normalize headers with spaces before the colon

RFC 7230 is clear about headers with a space before the colon, like

X-Answer : 42

being invalid, but we've been accepting and normalizing them for compatibility
purposes since CL 5690059 in 2012.

On the client side, this is harmless and indeed most browsers behave the same
to this day. On the server side, this becomes a security issue when the
behavior doesn't match that of a reverse proxy sitting in front of the server.

For example, if a WAF accepts them without normalizing them, it might be
possible to bypass its filters, because the Go server would interpret the
header differently. Worse, if the reverse proxy coalesces requests onto a
single HTTP/1.1 connection to a Go server, the understanding of the request
boundaries can get out of sync between them, allowing an attacker to tack an
arbitrary method and path onto a request by other clients, including
authentication headers unknown to the attacker.

This was recently presented at multiple security conferences:
https://portswigger.net/blog/http-desync-attacks-request-smuggling-reborn

net/http servers already reject header keys with invalid characters.
Simply stop normalizing extra spaces in net/textproto, let it return them
unchanged like it does for other invalid headers, and let net/http enforce
RFC 7230, which is HTTP specific. This loses us normalization on the client
side, but there's no right answer on the client side anyway, and hiding the
issue sounds worse than letting the application decide.

Fixes CVE-2019-16276
Fixes #34540

Change-Id: I6d272de827e0870da85d93df770d6a0e161bbcf1
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/549719
Reviewed-by: Brad Fitzpatrick <bradfitz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/197503
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>

src/net/http/serve_test.go

@@ -4755,6 +4755,10 @@ func TestServerValidatesHeaders(t *testing.T) {
 		{"foo\xffbar: foo\r\n", 400},                         // binary in header
 		{"foo\x00bar: foo\r\n", 400},                         // binary in header
 		{"Foo: " + strings.Repeat("x", 1<<21) + "\r\n", 431}, // header too large
+		// Spaces between the header key and colon are not allowed.
+		// See RFC 7230, Section 3.2.4.
+		{"Foo : bar\r\n", 400},
+		{"Foo\t: bar\r\n", 400},
 
 		{"foo: foo foo\r\n", 200},    // LWS space is okay
 		{"foo: foo\tfoo\r\n", 200},   // LWS tab is okay

src/net/http/transport_test.go

@@ -5692,3 +5692,30 @@ func TestTransportIgnores408(t *testing.T) {
 	}
 	t.Fatalf("timeout after %v waiting for Transport connections to die off", time.Since(t0))
 }
+
+func TestInvalidHeaderResponse(t *testing.T) {
+	setParallel(t)
+	defer afterTest(t)
+	cst := newClientServerTest(t, h1Mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+		conn, buf, _ := w.(Hijacker).Hijack()
+		buf.Write([]byte("HTTP/1.1 200 OK\r\n" +
+			"Date: Wed, 30 Aug 2017 19:09:27 GMT\r\n" +
+			"Content-Type: text/html; charset=utf-8\r\n" +
+			"Content-Length: 0\r\n" +
+			"Foo : bar\r\n\r\n"))
+		buf.Flush()
+		conn.Close()
+	}))
+	defer cst.close()
+	res, err := cst.c.Get(cst.ts.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+	if v := res.Header.Get("Foo"); v != "" {
+		t.Errorf(`unexpected "Foo" header: %q`, v)
+	}
+	if v := res.Header.Get("Foo "); v != "bar" {
+		t.Errorf(`bad "Foo " header value: %q, want %q`, v, "bar")
+	}
+}

src/net/textproto/reader.go

@@ -495,18 +495,12 @@ func (r *Reader) ReadMIMEHeader() (MIMEHeader, error) {
 			return m, err
 		}
 
-		// Key ends at first colon; should not have trailing spaces
+		// Key ends at first colon.
-		// but they appear in the wild, violating specs, so we remove
-		// them if present.
 		i := bytes.IndexByte(kv, ':')
 		if i < 0 {
 			return m, ProtocolError("malformed MIME header line: " + string(kv))
 		}
-		endKey := i
+		key := canonicalMIMEHeaderKey(kv[:i])
-		for endKey > 0 && kv[endKey-1] == ' ' {
-			endKey--
-		}
-		key := canonicalMIMEHeaderKey(kv[:endKey])
 
 		// As per RFC 7230 field-name is a token, tokens consist of one or more chars.
 		// We could return a ProtocolError here, but better to be liberal in what we

src/net/textproto/reader_test.go

@@ -188,11 +188,10 @@ func TestLargeReadMIMEHeader(t *testing.T) {
 	}
 }
 
-// Test that we read slightly-bogus MIME headers seen in the wild,
+// TestReadMIMEHeaderNonCompliant checks that we don't normalize headers
-// with spaces before colons, and spaces in keys.
+// with spaces before colons, and accept spaces in keys.
 func TestReadMIMEHeaderNonCompliant(t *testing.T) {
-	// Invalid HTTP response header as sent by an Axis security
+	// These invalid headers will be rejected by net/http according to RFC 7230.
-	// camera: (this is handled by IE, Firefox, Chrome, curl, etc.)
 	r := reader("Foo: bar\r\n" +
 		"Content-Language: en\r\n" +
 		"SID : 0\r\n" +
@@ -202,9 +201,9 @@ func TestReadMIMEHeaderNonCompliant(t *testing.T) {
 	want := MIMEHeader{
 		"Foo":              {"bar"},
 		"Content-Language": {"en"},
-		"Sid":              {"0"},
+		"SID ":             {"0"},
-		"Audio Mode":       {"None"},
+		"Audio Mode ":      {"None"},
-		"Privilege":        {"127"},
+		"Privilege ":       {"127"},
 	}
 	if !reflect.DeepEqual(m, want) || err != nil {
 		t.Fatalf("ReadMIMEHeader =\n%v, %v; want:\n%v", m, err, want)