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

[brad...@golang.org]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com (cc: golan...@googlegroups.com),

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

Description:
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)

Please review this at http://codereview.appspot.com/4921049/

Affected files:
M src/pkg/http/request.go
M src/pkg/http/serve_test.go
M src/pkg/http/server.go
M src/pkg/http/transfer.go

[a...@golang.org]

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#newcode611
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#newcode612
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#newcode613
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#newcode615
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#newcode616
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#newcode634
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#newcode642
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#newcode677
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#newcode899
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#newcode128
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#newcode551
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#newcode552
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#newcode482
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#newcode515
src/pkg/http/transfer.go:515: // in the strea is not necessary.
s/strea/stream/

http://codereview.appspot.com/4921049/

[brad...@golang.org]

Hello golan...@googlegroups.com, a...@golang.org (cc:
golan...@googlegroups.com),

Please take another look.

http://codereview.appspot.com/4921049/

[brad...@golang.org]

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#newcode611
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#newcode612
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#newcode613
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#newcode615
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#newcode616
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#newcode634
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#newcode642
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#newcode677
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#newcode899
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#newcode128
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#newcode482
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#newcode515
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.

http://codereview.appspot.com/4921049/

[a...@golang.org]

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
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#newcode136
src/pkg/http/server.go:136: // requestTooLarge is called by
maxBytesReader, when too much input has
s/, //

http://codereview.appspot.com/4921049/

[Andrew Gerrand]

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

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

[Brad Fitzpatrick]

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

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
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.

[Andrew Gerrand]

LGTM

[brad...@golang.org]

*** 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

http://codereview.appspot.com/4921049/

[r...@golang.org]

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#newcode615
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#newcode634
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#newcode676
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.

http://codereview.appspot.com/4921049/