[release-branch.go1.4] net/http: backport some potential request smuggling vectors from Go 1.5

This CL contains the verbatim tests from these two changes, but with
alternate minimal fixes against the 1.4 tree:

https://go-review.googlesource.com/#/c/12865/
https://go-review.googlesource.com/#/c/13148/

Change-Id: If98c2198e24e30e14a3b7b5e954b504d1f18db89
Reviewed-on: https://go-review.googlesource.com/14802Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Run-TryBot: Chris Broadfoot <cbro@golang.org>

src/net/http/serve_test.go

@@ -1166,6 +1166,207 @@ func TestServerUnreadRequestBodyLarge(t *testing.T) {
 	}
 }
 
+// testHandlerBodyConsumer represents a function injected into a test handler to
+// vary work done on a request Body.
+type testHandlerBodyConsumer struct {
+	name string
+	f    func(io.ReadCloser)
+}
+
+var testHandlerBodyConsumers = []testHandlerBodyConsumer{
+	{"nil", func(io.ReadCloser) {}},
+	{"close", func(r io.ReadCloser) { r.Close() }},
+	{"discard", func(r io.ReadCloser) { io.Copy(ioutil.Discard, r) }},
+}
+
+func TestRequestBodyReadErrorClosesConnection(t *testing.T) {
+	defer afterTest(t)
+	for _, handler := range testHandlerBodyConsumers {
+		conn := new(testConn)
+		conn.readBuf.WriteString("POST /public HTTP/1.1\r\n" +
+			"Host: test\r\n" +
+			"Transfer-Encoding: chunked\r\n" +
+			"\r\n" +
+			"hax\r\n" + // Invalid chunked encoding
+			"GET /secret HTTP/1.1\r\n" +
+			"Host: test\r\n" +
+			"\r\n")
+
+		conn.closec = make(chan bool, 1)
+		ls := &oneConnListener{conn}
+		var numReqs int
+		go Serve(ls, HandlerFunc(func(_ ResponseWriter, req *Request) {
+			numReqs++
+			if strings.Contains(req.URL.Path, "secret") {
+				t.Error("Request for /secret encountered, should not have happened.")
+			}
+			handler.f(req.Body)
+		}))
+		<-conn.closec
+		if numReqs != 1 {
+			t.Errorf("Handler %v: got %d reqs; want 1", handler.name, numReqs)
+		}
+	}
+}
+
+func TestInvalidTrailerClosesConnection(t *testing.T) {
+	defer afterTest(t)
+	for _, handler := range testHandlerBodyConsumers {
+		conn := new(testConn)
+		conn.readBuf.WriteString("POST /public HTTP/1.1\r\n" +
+			"Host: test\r\n" +
+			"Trailer: hack\r\n" +
+			"Transfer-Encoding: chunked\r\n" +
+			"\r\n" +
+			"3\r\n" +
+			"hax\r\n" +
+			"0\r\n" +
+			"I'm not a valid trailer\r\n" +
+			"GET /secret HTTP/1.1\r\n" +
+			"Host: test\r\n" +
+			"\r\n")
+
+		conn.closec = make(chan bool, 1)
+		ln := &oneConnListener{conn}
+		var numReqs int
+		go Serve(ln, HandlerFunc(func(_ ResponseWriter, req *Request) {
+			numReqs++
+			if strings.Contains(req.URL.Path, "secret") {
+				t.Errorf("Handler %s, Request for /secret encountered, should not have happened.", handler.name)
+			}
+			handler.f(req.Body)
+		}))
+		<-conn.closec
+		if numReqs != 1 {
+			t.Errorf("Handler %s: got %d reqs; want 1", handler.name, numReqs)
+		}
+	}
+}
+
+// slowTestConn is a net.Conn that provides a means to simulate parts of a
+// request being received piecemeal. Deadlines can be set and enforced in both
+// Read and Write.
+type slowTestConn struct {
+	// over multiple calls to Read, time.Durations are slept, strings are read.
+	script []interface{}
+	closec chan bool
+	rd, wd time.Time // read, write deadline
+	noopConn
+}
+
+func (c *slowTestConn) SetDeadline(t time.Time) error {
+	c.SetReadDeadline(t)
+	c.SetWriteDeadline(t)
+	return nil
+}
+
+func (c *slowTestConn) SetReadDeadline(t time.Time) error {
+	c.rd = t
+	return nil
+}
+
+func (c *slowTestConn) SetWriteDeadline(t time.Time) error {
+	c.wd = t
+	return nil
+}
+
+func (c *slowTestConn) Read(b []byte) (n int, err error) {
+restart:
+	if !c.rd.IsZero() && time.Now().After(c.rd) {
+		return 0, syscall.ETIMEDOUT
+	}
+	if len(c.script) == 0 {
+		return 0, io.EOF
+	}
+
+	switch cue := c.script[0].(type) {
+	case time.Duration:
+		if !c.rd.IsZero() {
+			// If the deadline falls in the middle of our sleep window, deduct
+			// part of the sleep, then return a timeout.
+			if remaining := c.rd.Sub(time.Now()); remaining < cue {
+				c.script[0] = cue - remaining
+				time.Sleep(remaining)
+				return 0, syscall.ETIMEDOUT
+			}
+		}
+		c.script = c.script[1:]
+		time.Sleep(cue)
+		goto restart
+
+	case string:
+		n = copy(b, cue)
+		// If cue is too big for the buffer, leave the end for the next Read.
+		if len(cue) > n {
+			c.script[0] = cue[n:]
+		} else {
+			c.script = c.script[1:]
+		}
+
+	default:
+		panic("unknown cue in slowTestConn script")
+	}
+
+	return
+}
+
+func (c *slowTestConn) Close() error {
+	select {
+	case c.closec <- true:
+	default:
+	}
+	return nil
+}
+
+func (c *slowTestConn) Write(b []byte) (int, error) {
+	if !c.wd.IsZero() && time.Now().After(c.wd) {
+		return 0, syscall.ETIMEDOUT
+	}
+	return len(b), nil
+}
+
+func TestRequestBodyTimeoutClosesConnection(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping in -short mode")
+	}
+	defer afterTest(t)
+	for _, handler := range testHandlerBodyConsumers {
+		conn := &slowTestConn{
+			script: []interface{}{
+				"POST /public HTTP/1.1\r\n" +
+					"Host: test\r\n" +
+					"Content-Length: 10000\r\n" +
+					"\r\n",
+				"foo bar baz",
+				600 * time.Millisecond, // Request deadline should hit here
+				"GET /secret HTTP/1.1\r\n" +
+					"Host: test\r\n" +
+					"\r\n",
+			},
+			closec: make(chan bool, 1),
+		}
+		ls := &oneConnListener{conn}
+
+		var numReqs int
+		s := Server{
+			Handler: HandlerFunc(func(_ ResponseWriter, req *Request) {
+				numReqs++
+				if strings.Contains(req.URL.Path, "secret") {
+					t.Error("Request for /secret encountered, should not have happened.")
+				}
+				handler.f(req.Body)
+			}),
+			ReadTimeout: 400 * time.Millisecond,
+		}
+		go s.Serve(ls)
+		<-conn.closec
+
+		if numReqs != 1 {
+			t.Errorf("Handler %v: got %d reqs; want 1", handler.name, numReqs)
+		}
+	}
+}
+
 func TestTimeoutHandler(t *testing.T) {
 	defer afterTest(t)
 	sendHi := make(chan bool, 1)

src/net/http/server.go

@@ -796,8 +796,8 @@ func (cw *chunkWriter) writeHeader(p []byte) {
 	if w.req.ContentLength != 0 && !w.closeAfterReply {
 		ecr, isExpecter := w.req.Body.(*expectContinueReader)
 		if !isExpecter || ecr.resp.wroteContinue {
-			n, _ := io.CopyN(ioutil.Discard, w.req.Body, maxPostHandlerReadBytes+1)
-			if n >= maxPostHandlerReadBytes {
+			n, err := io.CopyN(ioutil.Discard, w.req.Body, maxPostHandlerReadBytes+1)
+			if n >= maxPostHandlerReadBytes || (err != nil && err != io.EOF) {
 				w.requestTooLarge()
 				delHeader("Connection")
 				setHeader.connection = "close"

src/net/http/transfer.go

@@ -616,6 +616,7 @@ func (b *body) readLocked(p []byte) (n int, err error) {
 		if b.hdr != nil {
 			if e := b.readTrailer(); e != nil {
 				err = e
+				b.closed = true
 			}
 			b.hdr = nil
 		} else {