code review 4921049: http: add MaxBytesReader to limit request body size

http: add MaxBytesReader to limit request body size

This adds http.MaxBytesReader, similar to io.LimitReader,
but specific to http, and for preventing a class of DoS
attacks.

This also makes the 10MB ParseForm limit optional (if
not already set by a MaxBytesReader), documents it,
and also adds "PUT" as a valid verb for parsing forms
in the request body.

Improves issue 2093 (DoS protection)
Fixes issue 2165 (PUT form parsing)

[bradfitz, 2011-08-22 08:48:01]

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

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

[adg, 2011-08-23 01:06:21]

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:611: // MaxBytesReader is similar to io.LimitReader, but
is intended for us
s/ us//

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:612: // on limiting the size of incoming request bodies.
In contrast to
s/on //

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:613: // io.LimitReader, MaxBytesReader on a ReadCloser,
returns a non-EOF
s/on a/is a/

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:615: // HTTP TCP connection (if applicable) when the
limit is hit, to
s/HTTP TCP connection/underlying io.ReadCloser/
s/, to/./

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:616: // prevent a client from accidentally or
maliciously sending a large
s/prevent a client/This prevents clients/

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:634: res.closeAfterReply = true
I'm not that happy about the intermingling between these two types here.

Perhaps add a method 
func (*request) requestTooLarge()
that sets the flags and the header.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:642: p = p[0:l.n]
p = p[:l.n]

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:677: var maxFormSize = int64((1 << 63) - 1)
maxFormSize := ...

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/serve_test.go
File src/pkg/http/serve_test.go (right):

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/serve_test.go#ne...
src/pkg/http/serve_test.go:899: type neverEnding byte
This is great.

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

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:128: // don't consume the
don't consume the... ? THE SUSPENSE IS KILLING ME!

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:551: // Close the body, unless we're about to close the
whole TCP connection
s/whole TCP connection/underlying net.Conn./

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:552: // anyway.
dd

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/transfer.go
File src/pkg/http/transfer.go (right):

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/transfer.go#newc...
src/pkg/http/transfer.go:482: res *response // if this is a server request, this
is our response writer
// response writer for server requests

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/transfer.go#newc...
src/pkg/http/transfer.go:515: // in the strea is not necessary.
s/strea/stream/

[bradfitz, 2011-08-23 06:28:55]

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

Please take another look.

[bradfitz, 2011-08-23 06:29:01]

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:611: // MaxBytesReader is similar to io.LimitReader, but
is intended for us
On 2011/08/23 01:06:21, adg wrote:
> s/ us//

Done.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:612: // on limiting the size of incoming request bodies.
In contrast to
On 2011/08/23 01:06:21, adg wrote:
> s/on //

Done.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:613: // io.LimitReader, MaxBytesReader on a ReadCloser,
returns a non-EOF
On 2011/08/23 01:06:21, adg wrote:
> s/on a/is a/

Done.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:615: // HTTP TCP connection (if applicable) when the
limit is hit, to
On 2011/08/23 01:06:21, adg wrote:
> s/HTTP TCP connection/underlying io.ReadCloser/
> s/, to/./

Done.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:616: // prevent a client from accidentally or
maliciously sending a large
On 2011/08/23 01:06:21, adg wrote:
> s/prevent a client/This prevents clients/

Done.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:634: res.closeAfterReply = true
On 2011/08/23 01:06:21, adg wrote:
> I'm not that happy about the intermingling between these two types here.
> 
> Perhaps add a method 
> func (*request) requestTooLarge()
> that sets the flags and the header.

Done.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:642: p = p[0:l.n]
On 2011/08/23 01:06:21, adg wrote:
> p = p[:l.n]

Done.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:677: var maxFormSize = int64((1 << 63) - 1)
On 2011/08/23 01:06:21, adg wrote:
> maxFormSize := ...

Done.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/serve_test.go
File src/pkg/http/serve_test.go (right):

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/serve_test.go#ne...
src/pkg/http/serve_test.go:899: type neverEnding byte
On 2011/08/23 01:06:21, adg wrote:
> This is great.

:)

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

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:128: // don't consume the
On 2011/08/23 01:06:21, adg wrote:
> don't consume the... ? THE SUSPENSE IS KILLING ME!

Done.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/transfer.go
File src/pkg/http/transfer.go (right):

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/transfer.go#newc...
src/pkg/http/transfer.go:482: res *response // if this is a server request, this
is our response writer
On 2011/08/23 01:06:21, adg wrote:
> // response writer for server requests

Done.

http://codereview.appspot.com/4921049/diff/4001/src/pkg/http/transfer.go#newc...
src/pkg/http/transfer.go:515: // in the strea is not necessary.
On 2011/08/23 01:06:21, adg wrote:
> s/strea/stream/

Done.

[adg, 2011-08-23 06:44:27]

http://codereview.appspot.com/4921049/diff/11001/src/pkg/http/serve_test.go
File src/pkg/http/serve_test.go (right):

http://codereview.appspot.com/4921049/diff/11001/src/pkg/http/serve_test.go#n...
src/pkg/http/serve_test.go:922: r.Body = MaxBytesReader(w, r.Body, limit)
Should we install this by default, and allow users to override it?

To customize they could do something like:

r.Body.(*http.MaxBytesReader).SetLimit(1e9) // lower to 1mb limit

And to bypass it, they could just access the underlying reader:

body := r.Body.(*http.MaxBytesReader).ReadCloser

I think it's important to be secure by default.

What do you think?

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

http://codereview.appspot.com/4921049/diff/11001/src/pkg/http/server.go#newco...
src/pkg/http/server.go:136: // requestTooLarge is called by maxBytesReader, when
too much input has
s/, //

[adg, 2011-08-23 06:45:16]

On 23 August 2011 16:44,  <adg@golang.org> wrote:
> To customize they could do something like:

That's not to say there aren't better ways of doing it entirely...

[bradfitz, 2011-08-23 06:52:02]

On Tue, Aug 23, 2011 at 10:44 AM, <adg@golang.org> wrote:

>
> http://codereview.appspot.com/**4921049/diff/11001/src/pkg/**
>
http/serve_test.go<http://codereview.appspot.com/4921049/diff/11001/src/pkg/http/serve_test.go>
>
> File src/pkg/http/serve_test.go (right):
>
> http://codereview.appspot.com/**4921049/diff/11001/src/pkg/**
>
http/serve_test.go#newcode922<http://codereview.appspot.com/4921049/diff/11001/src/pkg/http/serve_test.go#newcode922>
> src/pkg/http/serve_test.go:**922: r.Body = MaxBytesReader(w, r.Body,
> limit)
> Should we install this by default, and allow users to override it?
>

They can install a root handler to do it if they want.  Or we can add a new
optional field on Server to do that for them, if they set it to non-zero.

I don't want to do it by default.



> To customize they could do something like:
>
> r.Body.(*http.MaxBytesReader).**SetLimit(1e9) // lower to 1mb limit
>
> And to bypass it, they could just access the underlying reader:
>
> body := r.Body.(*http.MaxBytesReader).**ReadCloser
>

Gross.

The http package already has lots of optional and composed stuff.  I don't
want do more of it by default and ingrain it as The Way to do it.


> I think it's important to be secure by default.
>
> What do you think?
>

Let's get mechanism in first, and then deal with defaults later.

For people who don't set it, though, what I do want to do is bound the
ioutil.Discard copy at the end of a request.  If you get an HTTP request and
reply with a 200 or 401 or something and don't read in the body, I don't
want the http framework to silently and automatically
io.Copy(ioutil.Discard, an unbounded amount of data).  That's a separate
upcoming fix, and related to being secure by default.

[adg, 2011-08-23 07:00:33]

LGTM

[bradfitz, 2011-08-23 08:17:29]

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

http: add MaxBytesReader to limit request body size

This adds http.MaxBytesReader, similar to io.LimitReader,
but specific to http, and for preventing a class of DoS
attacks.

This also makes the 10MB ParseForm limit optional (if
not already set by a MaxBytesReader), documents it,
and also adds "PUT" as a valid verb for parsing forms
in the request body.

Improves issue 2093 (DoS protection)
Fixes issue 2165 (PUT form parsing)

R=golang-dev, adg
CC=golang-dev
http://codereview.appspot.com/4921049

[rsc, 2011-08-23 20:11:24]

http://codereview.appspot.com/4921049/diff/14001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4921049/diff/14001/src/pkg/http/request.go#newc...
src/pkg/http/request.go:615: // underlying io.ReadCloser connection (if
applicable, usually a TCP
I don't understand the 'if applicable'.  It's a ReadCloser, so Close is always
applicable.  Suggestion:

In contrast to io.LimitReader, MaxBytesReader's result is a ReadCloser,
returns a non-EOF error for a Read beyond the limit, and Closes the
underlying reader when its Close method is called.

http://codereview.appspot.com/4921049/diff/14001/src/pkg/http/request.go#newc...
src/pkg/http/request.go:634: if res, ok := l.w.(*response); ok {
This is magic.  Can we avoid it?

http://codereview.appspot.com/4921049/diff/14001/src/pkg/http/request.go#newc...
src/pkg/http/request.go:676: maxFormSize := int64((1 << 63) - 1)
Drop the inner ().  Gofmt will format as 1<<63 - 1 to make the precedence clear.