| From 265b691ac440bfb711d8de323346f7d72e620efe Mon Sep 17 00:00:00 2001 |
| From: Filippo Valsorda <filippo@golang.org> |
| Date: Thu, 12 Sep 2019 12:37:36 -0400 |
| Subject: [PATCH] [release-branch.go1.12-security] 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 |
| |
| Change-Id: I6d272de827e0870da85d93df770d6a0e161bbcf1 |
| Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/549719 |
| Reviewed-by: Brad Fitzpatrick <bradfitz@google.com> |
| (cherry picked from commit 1280b868e82bf173ea3e988be3092d160ee66082) |
| Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/558776 |
| Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> |
| |
| CVE: CVE-2019-16276 |
| |
| Upstream-Status: Backport [https://github.com/golang/go/commit/6e6f4aaf70c8b1cc81e65a26332aa9409de03ad8] |
| |
| Signed-off-by: Chen Qi <Qi.Chen@windriver.com> |
| --- |
| src/net/http/serve_test.go | 4 ++++ |
| src/net/http/transport_test.go | 27 +++++++++++++++++++++++++++ |
| src/net/textproto/reader.go | 10 ++-------- |
| src/net/textproto/reader_test.go | 13 ++++++------- |
| 4 files changed, 39 insertions(+), 15 deletions(-) |
| |
| diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go |
| index 6eb0088a96..89bfdfbb82 100644 |
| --- a/src/net/http/serve_test.go |
| +++ b/src/net/http/serve_test.go |
| @@ -4748,6 +4748,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 |
| diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go |
| index 5c329543e2..5e5438a708 100644 |
| --- a/src/net/http/transport_test.go |
| +++ b/src/net/http/transport_test.go |
| @@ -5133,3 +5133,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") |
| + } |
| +} |
| diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go |
| index 2c4f25d5ae..1a5e364cf7 100644 |
| --- a/src/net/textproto/reader.go |
| +++ b/src/net/textproto/reader.go |
| @@ -493,18 +493,12 @@ func (r *Reader) ReadMIMEHeader() (MIMEHeader, error) { |
| return m, err |
| } |
| |
| - // Key ends at first colon; should not have trailing spaces |
| - // but they appear in the wild, violating specs, so we remove |
| - // them if present. |
| + // Key ends at first colon. |
| i := bytes.IndexByte(kv, ':') |
| if i < 0 { |
| return m, ProtocolError("malformed MIME header line: " + string(kv)) |
| } |
| - endKey := i |
| - for endKey > 0 && kv[endKey-1] == ' ' { |
| - endKey-- |
| - } |
| - key := canonicalMIMEHeaderKey(kv[:endKey]) |
| + key := canonicalMIMEHeaderKey(kv[:i]) |
| |
| // 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 |
| diff --git a/src/net/textproto/reader_test.go b/src/net/textproto/reader_test.go |
| index f85fbdc36d..b92fdcd3c7 100644 |
| --- a/src/net/textproto/reader_test.go |
| +++ b/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, |
| -// with spaces before colons, and spaces in keys. |
| +// TestReadMIMEHeaderNonCompliant checks that we don't normalize headers |
| +// with spaces before colons, and accept spaces in keys. |
| func TestReadMIMEHeaderNonCompliant(t *testing.T) { |
| - // Invalid HTTP response header as sent by an Axis security |
| - // camera: (this is handled by IE, Firefox, Chrome, curl, etc.) |
| + // These invalid headers will be rejected by net/http according to RFC 7230. |
| 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"}, |
| - "Audio Mode": {"None"}, |
| - "Privilege": {"127"}, |
| + "SID ": {"0"}, |
| + "Audio Mode ": {"None"}, |
| + "Privilege ": {"127"}, |
| } |
| if !reflect.DeepEqual(m, want) || err != nil { |
| t.Fatalf("ReadMIMEHeader =\n%v, %v; want:\n%v", m, err, want) |