From 1c69384da4fb4a1323e011941c101189247fea67 Mon Sep 17 00:00:00 2001 From: Tom Bergan Date: Mon, 27 Nov 2017 11:25:14 -0800 Subject: [PATCH] net/textproto: reject all headers with a leading space Previously, golang.org/cl/75350 updated ReadMIMEHeader to ignore the first header line when it begins with a leading space, as in the following example: GET / HTTP/1.1 Host: foo.com Accept-Encoding: gzip However, golang.org/cl/75350 changed ReadMIMEHeader's behavior for the following example: before the CL it returned an error, but after the CL it ignored the first line. GET / HTTP/1.1 Host foo.com Accept-Encoding: gzip This change updates ReadMIMEHeader to always fail when the first header line starts with a space. During the discussion for golang.org/cl/75350, we realized we had three competing needs: 1. HTTP clients should accept malformed response headers when possible (ignoring the malformed lines). 2. HTTP servers should reject all malformed request headers. 3. The net/textproto package is used by multiple protocols (most notably, HTTP and SMTP) which have slightly different parsing semantics. This complicates changes to net/textproto. We weren't sure how to best fix net/textproto without an API change, but it is too late for API changes in Go 1.10. We decided to ignore initial lines that begin with spaces, thinking that would have the least impact on existing users -- malformed headers would continue to parse, but the initial lines would be ignored. Instead, golang.org/cl/75350 actually changed ReadMIMEHeader to succeed in cases where it previously failed (as in the above example). Reconsidering the above two examples, there does not seem to be a good argument to silently ignore ` Host: foo.com` but fail on ` Host foo.com`. Hence, this change fails for *all* headers where the initial line begins with a space. Updates #22464 Change-Id: I68d3d190489c350b0bc1549735bf6593fe11a94c Reviewed-on: https://go-review.googlesource.com/80055 Run-TryBot: Tom Bergan TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- src/net/http/readrequest_test.go | 28 +++++---------- src/net/http/response_test.go | 58 +++++++++++--------------------- src/net/http/transport_test.go | 13 +++---- src/net/textproto/reader.go | 12 ++++--- src/net/textproto/reader_test.go | 27 +++++++-------- 5 files changed, 54 insertions(+), 84 deletions(-) diff --git a/src/net/http/readrequest_test.go b/src/net/http/readrequest_test.go index 21c0e098bf..22a9c2ef4b 100644 --- a/src/net/http/readrequest_test.go +++ b/src/net/http/readrequest_test.go @@ -401,26 +401,6 @@ var reqTests = []reqTest{ noTrailer, noError, }, - - // leading whitespace in the first header. golang.org/issue/22464 - { - "GET / HTTP/1.1\r\n Foobar: ignored\r\nConnection: close\r\n\r\n", - &Request{ - Method: "GET", - URL: &url.URL{ - Path: "/", - }, - Header: Header{"Connection": {"close"}}, - Proto: "HTTP/1.1", - ProtoMajor: 1, - ProtoMinor: 1, - RequestURI: "/", - Close: true, - }, - noBodyStr, - noTrailer, - noError, - }, } func TestReadRequest(t *testing.T) { @@ -473,6 +453,14 @@ Content-Length: 4 abc`)}, {"smuggle_content_len_head", reqBytes(`HEAD / HTTP/1.1 Host: foo +Content-Length: 5`)}, + + // golang.org/issue/22464 + {"leading_space_in_header", reqBytes(`HEAD / HTTP/1.1 + Host: foo +Content-Length: 5`)}, + {"leading_tab_in_header", reqBytes(`HEAD / HTTP/1.1 +\tHost: foo Content-Length: 5`)}, } diff --git a/src/net/http/response_test.go b/src/net/http/response_test.go index 484a89e46d..1ea19619fe 100644 --- a/src/net/http/response_test.go +++ b/src/net/http/response_test.go @@ -555,28 +555,6 @@ some body`, }, "Your Authentication failed.\r\n", }, - - // leading whitespace in the first header. golang.org/issue/22464 - { - "HTTP/1.1 200 OK\r\n" + - " Content-type: text/html\r\n" + - "\tIgnore: foobar\r\n" + - "Foo: bar\r\n\r\n", - Response{ - Status: "200 OK", - StatusCode: 200, - Proto: "HTTP/1.1", - ProtoMajor: 1, - ProtoMinor: 1, - Request: dummyReq("GET"), - Header: Header{ - "Foo": {"bar"}, - }, - Close: true, - ContentLength: -1, - }, - "", - }, } // tests successful calls to ReadResponse, and inspects the returned Response. @@ -838,7 +816,6 @@ func TestReadResponseErrors(t *testing.T) { type testCase struct { name string // optional, defaults to in in string - header Header wantErr interface{} // nil, err value, or string substring } @@ -864,22 +841,21 @@ func TestReadResponseErrors(t *testing.T) { } } - contentLength := func(status, body string, wantErr interface{}, header Header) testCase { + contentLength := func(status, body string, wantErr interface{}) testCase { return testCase{ name: fmt.Sprintf("status %q %q", status, body), in: fmt.Sprintf("HTTP/1.1 %s\r\n%s", status, body), wantErr: wantErr, - header: header, } } errMultiCL := "message cannot contain multiple Content-Length headers" tests := []testCase{ - {"", "", nil, io.ErrUnexpectedEOF}, - {"", "HTTP/1.1 301 Moved Permanently\r\nFoo: bar", nil, io.ErrUnexpectedEOF}, - {"", "HTTP/1.1", nil, "malformed HTTP response"}, - {"", "HTTP/2.0", nil, "malformed HTTP response"}, + {"", "", io.ErrUnexpectedEOF}, + {"", "HTTP/1.1 301 Moved Permanently\r\nFoo: bar", io.ErrUnexpectedEOF}, + {"", "HTTP/1.1", "malformed HTTP response"}, + {"", "HTTP/2.0", "malformed HTTP response"}, status("20X Unknown", true), status("abcd Unknown", true), status("二百/两百 OK", true), @@ -905,18 +881,22 @@ func TestReadResponseErrors(t *testing.T) { version("HTTP/1", true), version("http/1.1", true), - contentLength("200 OK", "Content-Length: 10\r\nContent-Length: 7\r\n\r\nGopher hey\r\n", errMultiCL, nil), - contentLength("200 OK", "Content-Length: 7\r\nContent-Length: 7\r\n\r\nGophers\r\n", nil, Header{"Content-Length": {"7"}}), - contentLength("201 OK", "Content-Length: 0\r\nContent-Length: 7\r\n\r\nGophers\r\n", errMultiCL, nil), - contentLength("300 OK", "Content-Length: 0\r\nContent-Length: 0 \r\n\r\nGophers\r\n", nil, Header{"Content-Length": {"0"}}), - contentLength("200 OK", "Content-Length:\r\nContent-Length:\r\n\r\nGophers\r\n", nil, nil), - contentLength("206 OK", "Content-Length:\r\nContent-Length: 0 \r\nConnection: close\r\n\r\nGophers\r\n", errMultiCL, nil), + contentLength("200 OK", "Content-Length: 10\r\nContent-Length: 7\r\n\r\nGopher hey\r\n", errMultiCL), + contentLength("200 OK", "Content-Length: 7\r\nContent-Length: 7\r\n\r\nGophers\r\n", nil), + contentLength("201 OK", "Content-Length: 0\r\nContent-Length: 7\r\n\r\nGophers\r\n", errMultiCL), + contentLength("300 OK", "Content-Length: 0\r\nContent-Length: 0 \r\n\r\nGophers\r\n", nil), + contentLength("200 OK", "Content-Length:\r\nContent-Length:\r\n\r\nGophers\r\n", nil), + contentLength("206 OK", "Content-Length:\r\nContent-Length: 0 \r\nConnection: close\r\n\r\nGophers\r\n", errMultiCL), // multiple content-length headers for 204 and 304 should still be checked - contentLength("204 OK", "Content-Length: 7\r\nContent-Length: 8\r\n\r\n", errMultiCL, nil), - contentLength("204 OK", "Content-Length: 3\r\nContent-Length: 3\r\n\r\n", nil, nil), - contentLength("304 OK", "Content-Length: 880\r\nContent-Length: 1\r\n\r\n", errMultiCL, nil), - contentLength("304 OK", "Content-Length: 961\r\nContent-Length: 961\r\n\r\n", nil, nil), + contentLength("204 OK", "Content-Length: 7\r\nContent-Length: 8\r\n\r\n", errMultiCL), + contentLength("204 OK", "Content-Length: 3\r\nContent-Length: 3\r\n\r\n", nil), + contentLength("304 OK", "Content-Length: 880\r\nContent-Length: 1\r\n\r\n", errMultiCL), + contentLength("304 OK", "Content-Length: 961\r\nContent-Length: 961\r\n\r\n", nil), + + // golang.org/issue/22464 + {"leading space in header", "HTTP/1.1 200 OK\r\n Content-type: text/html\r\nFoo: bar\r\n\r\n", "malformed MIME"}, + {"leading tab in header", "HTTP/1.1 200 OK\r\n\tContent-type: text/html\r\nFoo: bar\r\n\r\n", "malformed MIME"}, } for i, tt := range tests { diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go index dc55816ab6..5588077425 100644 --- a/src/net/http/transport_test.go +++ b/src/net/http/transport_test.go @@ -4281,12 +4281,13 @@ func TestMissingStatusNoPanic(t *testing.T) { shutdown := make(chan bool, 1) done := make(chan bool) fullAddrURL := fmt.Sprintf("http://%s", addr) - raw := `HTTP/1.1 400 - Date: Wed, 30 Aug 2017 19:09:27 GMT - Content-Type: text/html; charset=utf-8 - Content-Length: 10 - Last-Modified: Wed, 30 Aug 2017 19:02:02 GMT - Vary: Accept-Encoding` + "\r\n\r\nAloha Olaa" + raw := "HTTP/1.1 400\r\n" + + "Date: Wed, 30 Aug 2017 19:09:27 GMT\r\n" + + "Content-Type: text/html; charset=utf-8\r\n" + + "Content-Length: 10\r\n" + + "Last-Modified: Wed, 30 Aug 2017 19:02:02 GMT\r\n" + + "Vary: Accept-Encoding\r\n\r\n" + + "Aloha Olaa" go func() { defer func() { diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go index c5e0b7591e..8c3a05264a 100644 --- a/src/net/textproto/reader.go +++ b/src/net/textproto/reader.go @@ -477,11 +477,13 @@ func (r *Reader) ReadMIMEHeader() (MIMEHeader, error) { m := make(MIMEHeader, hint) - for r.skipSpace() > 0 { + // The first line cannot start with a leading space. + if buf, err := r.R.Peek(1); err == nil && (buf[0] == ' ' || buf[0] == '\t') { line, err := r.readLineSlice() - if len(line) == 0 || err != nil { + if err != nil { return m, err } + return m, ProtocolError("malformed MIME header initial line: " + string(line)) } for { @@ -490,9 +492,9 @@ func (r *Reader) ReadMIMEHeader() (MIMEHeader, error) { return m, err } - // Key ends at first colon; should not have spaces but - // they appear in the wild, violating specs, so we - // remove them if present. + // Key ends at first colon; should not have trailing spaces + // 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)) diff --git a/src/net/textproto/reader_test.go b/src/net/textproto/reader_test.go index b26765e3cd..f1c56b4608 100644 --- a/src/net/textproto/reader_test.go +++ b/src/net/textproto/reader_test.go @@ -211,21 +211,20 @@ func TestReadMIMEHeaderNonCompliant(t *testing.T) { } } -func TestReadMIMEHeaderLeadingSpace(t *testing.T) { - tests := []struct { - input string - want MIMEHeader - }{ - {" Ignore: ignore\r\nFoo: foo\r\n\r\n", MIMEHeader{"Foo": {"foo"}}}, - {"\tIgnore: ignore\r\nFoo: foo\r\n\r\n", MIMEHeader{"Foo": {"foo"}}}, - {" Ignore1: ignore\r\n Ignore2: ignore\r\nFoo: foo\r\n\r\n", MIMEHeader{"Foo": {"foo"}}}, - {" Ignore1: ignore\r\n\r\n", MIMEHeader{}}, +func TestReadMIMEHeaderMalformed(t *testing.T) { + inputs := []string{ + "No colon first line\r\nFoo: foo\r\n\r\n", + " No colon first line with leading space\r\nFoo: foo\r\n\r\n", + "\tNo colon first line with leading tab\r\nFoo: foo\r\n\r\n", + " First: line with leading space\r\nFoo: foo\r\n\r\n", + "\tFirst: line with leading tab\r\nFoo: foo\r\n\r\n", + "Foo: foo\r\nNo colon second line\r\n\r\n", } - for _, tt := range tests { - r := reader(tt.input) - m, err := r.ReadMIMEHeader() - if !reflect.DeepEqual(m, tt.want) || err != nil { - t.Errorf("ReadMIMEHeader(%q) = %v, %v; want %v", tt.input, m, err, tt.want) + + for _, input := range inputs { + r := reader(input) + if m, err := r.ReadMIMEHeader(); err == nil { + t.Errorf("ReadMIMEHeader(%q) = %v, %v; want nil, err", input, m, err) } } }