blob: 7b39dbd734b6249e2998cf84ba7f7a3be1f7a7fc [file] [log] [blame]
Brad Bishop64c979e2019-11-04 13:55:29 -05001From 265b691ac440bfb711d8de323346f7d72e620efe Mon Sep 17 00:00:00 2001
2From: Filippo Valsorda <filippo@golang.org>
3Date: Thu, 12 Sep 2019 12:37:36 -0400
4Subject: [PATCH] [release-branch.go1.12-security] net/textproto: don't
5 normalize headers with spaces before the colon
6
7RFC 7230 is clear about headers with a space before the colon, like
8
9X-Answer : 42
10
11being invalid, but we've been accepting and normalizing them for compatibility
12purposes since CL 5690059 in 2012.
13
14On the client side, this is harmless and indeed most browsers behave the same
15to this day. On the server side, this becomes a security issue when the
16behavior doesn't match that of a reverse proxy sitting in front of the server.
17
18For example, if a WAF accepts them without normalizing them, it might be
19possible to bypass its filters, because the Go server would interpret the
20header differently. Worse, if the reverse proxy coalesces requests onto a
21single HTTP/1.1 connection to a Go server, the understanding of the request
22boundaries can get out of sync between them, allowing an attacker to tack an
23arbitrary method and path onto a request by other clients, including
24authentication headers unknown to the attacker.
25
26This was recently presented at multiple security conferences:
27https://portswigger.net/blog/http-desync-attacks-request-smuggling-reborn
28
29net/http servers already reject header keys with invalid characters.
30Simply stop normalizing extra spaces in net/textproto, let it return them
31unchanged like it does for other invalid headers, and let net/http enforce
32RFC 7230, which is HTTP specific. This loses us normalization on the client
33side, but there's no right answer on the client side anyway, and hiding the
34issue sounds worse than letting the application decide.
35
36Fixes CVE-2019-16276
37
38Change-Id: I6d272de827e0870da85d93df770d6a0e161bbcf1
39Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/549719
40Reviewed-by: Brad Fitzpatrick <bradfitz@google.com>
41(cherry picked from commit 1280b868e82bf173ea3e988be3092d160ee66082)
42Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/558776
43Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
44
45CVE: CVE-2019-16276
46
47Upstream-Status: Backport [https://github.com/golang/go/commit/6e6f4aaf70c8b1cc81e65a26332aa9409de03ad8]
48
49Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
50---
51 src/net/http/serve_test.go | 4 ++++
52 src/net/http/transport_test.go | 27 +++++++++++++++++++++++++++
53 src/net/textproto/reader.go | 10 ++--------
54 src/net/textproto/reader_test.go | 13 ++++++-------
55 4 files changed, 39 insertions(+), 15 deletions(-)
56
57diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
58index 6eb0088a96..89bfdfbb82 100644
59--- a/src/net/http/serve_test.go
60+++ b/src/net/http/serve_test.go
61@@ -4748,6 +4748,10 @@ func TestServerValidatesHeaders(t *testing.T) {
62 {"foo\xffbar: foo\r\n", 400}, // binary in header
63 {"foo\x00bar: foo\r\n", 400}, // binary in header
64 {"Foo: " + strings.Repeat("x", 1<<21) + "\r\n", 431}, // header too large
65+ // Spaces between the header key and colon are not allowed.
66+ // See RFC 7230, Section 3.2.4.
67+ {"Foo : bar\r\n", 400},
68+ {"Foo\t: bar\r\n", 400},
69
70 {"foo: foo foo\r\n", 200}, // LWS space is okay
71 {"foo: foo\tfoo\r\n", 200}, // LWS tab is okay
72diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
73index 5c329543e2..5e5438a708 100644
74--- a/src/net/http/transport_test.go
75+++ b/src/net/http/transport_test.go
76@@ -5133,3 +5133,30 @@ func TestTransportIgnores408(t *testing.T) {
77 }
78 t.Fatalf("timeout after %v waiting for Transport connections to die off", time.Since(t0))
79 }
80+
81+func TestInvalidHeaderResponse(t *testing.T) {
82+ setParallel(t)
83+ defer afterTest(t)
84+ cst := newClientServerTest(t, h1Mode, HandlerFunc(func(w ResponseWriter, r *Request) {
85+ conn, buf, _ := w.(Hijacker).Hijack()
86+ buf.Write([]byte("HTTP/1.1 200 OK\r\n" +
87+ "Date: Wed, 30 Aug 2017 19:09:27 GMT\r\n" +
88+ "Content-Type: text/html; charset=utf-8\r\n" +
89+ "Content-Length: 0\r\n" +
90+ "Foo : bar\r\n\r\n"))
91+ buf.Flush()
92+ conn.Close()
93+ }))
94+ defer cst.close()
95+ res, err := cst.c.Get(cst.ts.URL)
96+ if err != nil {
97+ t.Fatal(err)
98+ }
99+ defer res.Body.Close()
100+ if v := res.Header.Get("Foo"); v != "" {
101+ t.Errorf(`unexpected "Foo" header: %q`, v)
102+ }
103+ if v := res.Header.Get("Foo "); v != "bar" {
104+ t.Errorf(`bad "Foo " header value: %q, want %q`, v, "bar")
105+ }
106+}
107diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go
108index 2c4f25d5ae..1a5e364cf7 100644
109--- a/src/net/textproto/reader.go
110+++ b/src/net/textproto/reader.go
111@@ -493,18 +493,12 @@ func (r *Reader) ReadMIMEHeader() (MIMEHeader, error) {
112 return m, err
113 }
114
115- // Key ends at first colon; should not have trailing spaces
116- // but they appear in the wild, violating specs, so we remove
117- // them if present.
118+ // Key ends at first colon.
119 i := bytes.IndexByte(kv, ':')
120 if i < 0 {
121 return m, ProtocolError("malformed MIME header line: " + string(kv))
122 }
123- endKey := i
124- for endKey > 0 && kv[endKey-1] == ' ' {
125- endKey--
126- }
127- key := canonicalMIMEHeaderKey(kv[:endKey])
128+ key := canonicalMIMEHeaderKey(kv[:i])
129
130 // As per RFC 7230 field-name is a token, tokens consist of one or more chars.
131 // We could return a ProtocolError here, but better to be liberal in what we
132diff --git a/src/net/textproto/reader_test.go b/src/net/textproto/reader_test.go
133index f85fbdc36d..b92fdcd3c7 100644
134--- a/src/net/textproto/reader_test.go
135+++ b/src/net/textproto/reader_test.go
136@@ -188,11 +188,10 @@ func TestLargeReadMIMEHeader(t *testing.T) {
137 }
138 }
139
140-// Test that we read slightly-bogus MIME headers seen in the wild,
141-// with spaces before colons, and spaces in keys.
142+// TestReadMIMEHeaderNonCompliant checks that we don't normalize headers
143+// with spaces before colons, and accept spaces in keys.
144 func TestReadMIMEHeaderNonCompliant(t *testing.T) {
145- // Invalid HTTP response header as sent by an Axis security
146- // camera: (this is handled by IE, Firefox, Chrome, curl, etc.)
147+ // These invalid headers will be rejected by net/http according to RFC 7230.
148 r := reader("Foo: bar\r\n" +
149 "Content-Language: en\r\n" +
150 "SID : 0\r\n" +
151@@ -202,9 +201,9 @@ func TestReadMIMEHeaderNonCompliant(t *testing.T) {
152 want := MIMEHeader{
153 "Foo": {"bar"},
154 "Content-Language": {"en"},
155- "Sid": {"0"},
156- "Audio Mode": {"None"},
157- "Privilege": {"127"},
158+ "SID ": {"0"},
159+ "Audio Mode ": {"None"},
160+ "Privilege ": {"127"},
161 }
162 if !reflect.DeepEqual(m, want) || err != nil {
163 t.Fatalf("ReadMIMEHeader =\n%v, %v; want:\n%v", m, err, want)