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 <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/readrequest_test.go b/src/net/http/readrequest_test.go
index 21c0e098bf79ef0360b0cecebd800f7c207ea10f..22a9c2ef4b29dd49638274d34b9dec1b7fcc2e40 100644 (file)
--- 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 484a89e46dbd14419772a74b3ec9f43be3059c85..1ea19619fee32c32449a48b26e23911b80e54372 100644 (file)
--- 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 dc55816ab6531841cdcdba93b4c129f0c7839de1..5588077425694868b14f87151d47ef86f53fb4cd 100644 (file)
--- 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 c5e0b7591e84c9ed0a72a94f1c92db2b4774a229..8c3a05264a4fd3057eb635c4742be0fb1fa7c9ba 100644 (file)
--- 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 b26765e3cd691ebf60ccb348795cadeb38203565..f1c56b4608197c8b01f73b08eb6180a5f5c9ecb8 100644 (file)
--- 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{}},
-       }
-       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)
+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 _, 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)
                }
        }
 }