Brad Bishop | 64c979e | 2019-11-04 13:55:29 -0500 | [diff] [blame^] | 1 | From 265b691ac440bfb711d8de323346f7d72e620efe Mon Sep 17 00:00:00 2001 |
| 2 | From: Filippo Valsorda <filippo@golang.org> |
| 3 | Date: Thu, 12 Sep 2019 12:37:36 -0400 |
| 4 | Subject: [PATCH] [release-branch.go1.12-security] net/textproto: don't |
| 5 | normalize headers with spaces before the colon |
| 6 | |
| 7 | RFC 7230 is clear about headers with a space before the colon, like |
| 8 | |
| 9 | X-Answer : 42 |
| 10 | |
| 11 | being invalid, but we've been accepting and normalizing them for compatibility |
| 12 | purposes since CL 5690059 in 2012. |
| 13 | |
| 14 | On the client side, this is harmless and indeed most browsers behave the same |
| 15 | to this day. On the server side, this becomes a security issue when the |
| 16 | behavior doesn't match that of a reverse proxy sitting in front of the server. |
| 17 | |
| 18 | For example, if a WAF accepts them without normalizing them, it might be |
| 19 | possible to bypass its filters, because the Go server would interpret the |
| 20 | header differently. Worse, if the reverse proxy coalesces requests onto a |
| 21 | single HTTP/1.1 connection to a Go server, the understanding of the request |
| 22 | boundaries can get out of sync between them, allowing an attacker to tack an |
| 23 | arbitrary method and path onto a request by other clients, including |
| 24 | authentication headers unknown to the attacker. |
| 25 | |
| 26 | This was recently presented at multiple security conferences: |
| 27 | https://portswigger.net/blog/http-desync-attacks-request-smuggling-reborn |
| 28 | |
| 29 | net/http servers already reject header keys with invalid characters. |
| 30 | Simply stop normalizing extra spaces in net/textproto, let it return them |
| 31 | unchanged like it does for other invalid headers, and let net/http enforce |
| 32 | RFC 7230, which is HTTP specific. This loses us normalization on the client |
| 33 | side, but there's no right answer on the client side anyway, and hiding the |
| 34 | issue sounds worse than letting the application decide. |
| 35 | |
| 36 | Fixes CVE-2019-16276 |
| 37 | |
| 38 | Change-Id: I6d272de827e0870da85d93df770d6a0e161bbcf1 |
| 39 | Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/549719 |
| 40 | Reviewed-by: Brad Fitzpatrick <bradfitz@google.com> |
| 41 | (cherry picked from commit 1280b868e82bf173ea3e988be3092d160ee66082) |
| 42 | Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/558776 |
| 43 | Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> |
| 44 | |
| 45 | CVE: CVE-2019-16276 |
| 46 | |
| 47 | Upstream-Status: Backport [https://github.com/golang/go/commit/6e6f4aaf70c8b1cc81e65a26332aa9409de03ad8] |
| 48 | |
| 49 | Signed-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 | |
| 57 | diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go |
| 58 | index 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 |
| 72 | diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go |
| 73 | index 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 | +} |
| 107 | diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go |
| 108 | index 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 |
| 132 | diff --git a/src/net/textproto/reader_test.go b/src/net/textproto/reader_test.go |
| 133 | index 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) |