[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/14802
Reviewed-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>
diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 5e0a005..c118375 100644
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -1166,6 +1166,207 @@
}
}
+// 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)
diff --git a/src/net/http/server.go b/src/net/http/server.go
index 008d5aa..500fe29 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -796,8 +796,8 @@
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"
diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go
index 3887604..39bf174 100644
--- a/src/net/http/transfer.go
+++ b/src/net/http/transfer.go
@@ -616,6 +616,7 @@
if b.hdr != nil {
if e := b.readTrailer(); e != nil {
err = e
+ b.closed = true
}
b.hdr = nil
} else {
