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>

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) {
@@ -474,6 +454,14 @@ 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`)},
 }
 
 func TestReadRequest_Bad(t *testing.T) {
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)
 		}
 	}
 }