]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: close server conn after request body error
authorJed Denlea <jed@fastly.com>
Thu, 30 Jul 2015 01:10:32 +0000 (18:10 -0700)
committerBrad Fitzpatrick <bradfitz@golang.org>
Sun, 2 Aug 2015 09:34:59 +0000 (09:34 +0000)
HTTP servers attempt to entirely consume a request body before sending a
response.  However, when doing so, it previously would ignore any errors
encountered.

Unfortunately, the errors triggered at this stage are indicative of at
least a couple problems: read timeouts and chunked encoding errors.
This means properly crafted and/or timed requests could lead to a
"smuggled" request.

The fix is to inspect the errors created by the response body Reader,
and treat anything other than io.EOF or ErrBodyReadAfterClose as
fatal to the connection.

Fixes #11930

Change-Id: I0bf18006d7d8f6537529823fc450f2e2bdb7c18e
Reviewed-on: https://go-review.googlesource.com/12865
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/net/http/serve_test.go
src/net/http/server.go

index 61bbeb8f5397409aa47dfc6115c5da60f2f4bda0..7e499810717fb914dd8574a9f54911d0ec0137f5 100644 (file)
@@ -1110,6 +1110,7 @@ func TestServerExpect(t *testing.T) {
 // Under a ~256KB (maxPostHandlerReadBytes) threshold, the server
 // should consume client request bodies that a handler didn't read.
 func TestServerUnreadRequestBodyLittle(t *testing.T) {
+       defer afterTest(t)
        conn := new(testConn)
        body := strings.Repeat("x", 100<<10)
        conn.readBuf.Write([]byte(fmt.Sprintf(
@@ -1329,6 +1330,173 @@ func testHandlerBodyClose(t *testing.T, i int, tt handlerBodyCloseTest) {
        }
 }
 
+// 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)
+               }
+       }
+}
+
+// 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)
index d12f696eafad91c6aa8daa4c4f08def975ad636b..905a8b9ad8dac76901186b1eff1b1b048c386ea0 100644 (file)
@@ -872,23 +872,56 @@ func (cw *chunkWriter) writeHeader(p []byte) {
        // don't want to do an unbounded amount of reading here for
        // DoS reasons, so we only try up to a threshold.
        if w.req.ContentLength != 0 && !w.closeAfterReply {
-               ecr, isExpecter := w.req.Body.(*expectContinueReader)
-               if !isExpecter || ecr.resp.wroteContinue {
-                       var tooBig bool
-                       if reqBody, ok := w.req.Body.(*body); ok && reqBody.unreadDataSize() >= maxPostHandlerReadBytes {
+               var discard, tooBig bool
+
+               switch bdy := w.req.Body.(type) {
+               case *expectContinueReader:
+                       if bdy.resp.wroteContinue {
+                               discard = true
+                       }
+               case *body:
+                       switch {
+                       case bdy.closed:
+                               if !bdy.sawEOF {
+                                       // Body was closed in handler with non-EOF error.
+                                       w.closeAfterReply = true
+                               }
+                       case bdy.unreadDataSize() >= maxPostHandlerReadBytes:
                                tooBig = true
-                       } else {
-                               n, _ := io.CopyN(ioutil.Discard, w.req.Body, maxPostHandlerReadBytes+1)
-                               tooBig = n >= maxPostHandlerReadBytes
+                       default:
+                               discard = true
                        }
-                       if tooBig {
-                               w.requestTooLarge()
-                               delHeader("Connection")
-                               setHeader.connection = "close"
-                       } else {
-                               w.req.Body.Close()
+               default:
+                       discard = true
+               }
+
+               if discard {
+                       _, err := io.CopyN(ioutil.Discard, w.req.Body, maxPostHandlerReadBytes+1)
+                       switch err {
+                       case nil:
+                               // There must be even more data left over.
+                               tooBig = true
+                       case ErrBodyReadAfterClose:
+                               // Body was already consumed and closed.
+                       case io.EOF:
+                               // The remaining body was just consumed, close it.
+                               err = w.req.Body.Close()
+                               if err != nil {
+                                       w.closeAfterReply = true
+                               }
+                       default:
+                               // Some other kind of error occured, like a read timeout, or
+                               // corrupt chunked encoding. In any case, whatever remains
+                               // on the wire must not be parsed as another HTTP request.
+                               w.closeAfterReply = true
                        }
                }
+
+               if tooBig {
+                       w.requestTooLarge()
+                       delHeader("Connection")
+                       setHeader.connection = "close"
+               }
        }
 
        code := w.status