code review 5268043: http: DoS protection: cap non-Handler Request.Body reads

http: DoS protection: cap non-Handler Request.Body reads

Previously, if an http.Handler didn't fully consume a
Request.Body before returning and the request and the response
from the handler indicated no reason to close the connection,
the server would read an unbounded amount of the request's
unread body to advance past the request message to find the
next request's header. That was a potential DoS.

With this CL there's a threshold under which we read
(currently 256KB) in order to keep the connection in
keep-alive mode, but once we hit that, we instead
switch into a "Connection: close" response and don't
read the request body.

Fixes issue 2093 (along with number of earlier CLs)

[bradfitz, 2011-10-14 22:42:18]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dsymonds, 2011-10-15 00:11:07]

LGTM

Looks reasonable to me.

http://codereview.appspot.com/5268043/diff/3003/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/5268043/diff/3003/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:151: func (r *response) ReadFrom(src io.Reader) (n int64,
err os.Error) {
want to rename the receiver here too?

http://codereview.appspot.com/5268043/diff/3003/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:290: if err == nil {
I think I would have put the err != nil case first, but not fussed.

[bradfitz, 2011-10-15 00:16:36]

http://codereview.appspot.com/5268043/diff/3003/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/5268043/diff/3003/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:151: func (r *response) ReadFrom(src io.Reader) (n int64,
err os.Error) {
On 2011/10/15 00:11:07, dsymonds wrote:
> want to rename the receiver here too?

will do

http://codereview.appspot.com/5268043/diff/3003/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:290: if err == nil {
On 2011/10/15 00:11:07, dsymonds wrote:
> I think I would have put the err != nil case first, but not fussed.

This isn't new code (just moving up from below) so I'd rather not touch it just
for diff cleanliness.

(true, I did change the receiver name elsewhere, but that was just confusing...)

[bradfitz, 2011-10-15 00:33:46]

*** Submitted as http://code.google.com/p/go/source/detail?r=e67bd0e3dbd4 ***

http: DoS protection: cap non-Handler Request.Body reads

Previously, if an http.Handler didn't fully consume a
Request.Body before returning and the request and the response
from the handler indicated no reason to close the connection,
the server would read an unbounded amount of the request's
unread body to advance past the request message to find the
next request's header. That was a potential DoS.

With this CL there's a threshold under which we read
(currently 256KB) in order to keep the connection in
keep-alive mode, but once we hit that, we instead
switch into a "Connection: close" response and don't
read the request body.

Fixes issue 2093 (along with number of earlier CLs)

R=golang-dev, dsymonds
CC=golang-dev
http://codereview.appspot.com/5268043