From: Brad Fitzpatrick Date: Mon, 21 Sep 2015 14:09:47 +0000 (+0200) Subject: [release-branch.go1.4] net/http: backport some potential request smuggling vectors... X-Git-Tag: go1.4.3~2 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=cb65428710d70abdaf101defa9cd7eaddff9d925;p=gostls13.git [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 Run-TryBot: Brad Fitzpatrick Reviewed-by: Andrew Gerrand Reviewed-by: Chris Broadfoot Run-TryBot: Chris Broadfoot --- diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 5e0a0053c0..c118375481 100644 --- a/src/net/http/serve_test.go +++ b/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) diff --git a/src/net/http/server.go b/src/net/http/server.go index 008d5aa7a7..500fe29b27 100644 --- a/src/net/http/server.go +++ b/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" diff --git a/src/net/http/transfer.go b/src/net/http/transfer.go index 388760445f..39bf1746e2 100644 --- a/src/net/http/transfer.go +++ b/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 {