Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/http2: limits on header block size #12843

Closed
abursavich opened this issue Oct 5, 2015 · 5 comments
Closed

x/net/http2: limits on header block size #12843

abursavich opened this issue Oct 5, 2015 · 5 comments
Assignees
Milestone

Comments

@abursavich
Copy link

@abursavich abursavich commented Oct 5, 2015

The spec provides a way to enforce limits on header block size. Without this feature, clients may cause a server to buffer arbitrarily large headers in memory before servicing the request. Through careful but simple crafting of HPACK encoded cookie headers, a malicious client is able to achieve an over 4000x amplification from bytes on the wire to server memory used with quadratic string garbage generation.

I would expect the http2 server to advertise a reasonable SETTINGS_MAX_HEADER_LIST_SIZE by default (possibly lifting the MaxHeaderBytes field from the http.Server) and enforce it. Potentially, although maybe deserving a separate tracking issue, the cookie header should not use string concatenation as it would still remain a target for massive garbage generation with the existing defaults.

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Oct 5, 2015
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 8, 2015

CL https://golang.org/cl/15601 mentions this issue.

bradfitz added a commit to golang/net that referenced this issue Oct 8, 2015
There are already tests which cover the behavior. This is simply
moving where it happens. I'm not adding new tests to cover the very
bad behavior because once the rest of the bug is fixed, it won't be
possible to observe anyway. But even for smallish inputs, this is
faster.

Part of golang/go#12843

Change-Id: I7d69f2409e2adb4a01d101a915e09cf887b03f21
Reviewed-on: https://go-review.googlesource.com/15601
Reviewed-by: Andrew Gerrand <adg@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 11, 2015

CL https://golang.org/cl/15751 mentions this issue.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 11, 2015

Re-opening per review comments.

@bradfitz bradfitz reopened this Oct 11, 2015
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 12, 2015

CL https://golang.org/cl/15732 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 12, 2015

CL https://golang.org/cl/15733 mentions this issue.

bradfitz added a commit that referenced this issue Oct 12, 2015
These were proposed in the RFC over three years ago, then proposed to
be added to Go in https://codereview.appspot.com/7678043/ 2 years and
7 months ago, and the spec hasn't been updated or retracted the whole
time.

Time to export them.

Of note, HTTP/2 uses code 431 (Request Header Fields Too Large).

Updates #12843

Change-Id: I78c2fed5fab9540a98e845ace73f21c430a48809
Reviewed-on: https://go-review.googlesource.com/15732
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Oct 12, 2016
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
Thanks to Andy Bursavich for the example attack.
Pair programmed with Dmitri Shuralyov.

Fixes golang/go#12843

Change-Id: Ic412c9364b37a10e5164232aa809b956874fae08
Reviewed-on: https://go-review.googlesource.com/15751
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
In the first attempt to enforce the SETTINGS_MAX_HEADER_LIST_SIZE
(https://go-review.googlesource.com/15751), the enforcement happened
in the hpack decoder and the hpack decoder returned errors on Write
and Close if the limit was violated. This was incorrect because the
decoder is used over the life of the connection and all subsequent
requests and could therefore get out of sync.

Instead, this moves the counting of the limit up to the http2 package
in the serverConn type, and replaces the hpack counting mechanism with
a simple on/off switch. When SetEmitEnabled is set false, the header
field emit callbacks will be suppressed and the hpack Decoder will do
less work (less CPU and garbage) if possible, but will still return
nil from Write and Close on valid input, and will still stay in sync
it the stream.

The http2 Server then returns a 431 error if emits were disabled while
processing the HEADER or any CONTINUATION frames.

Fixes golang/go#12843

Change-Id: I3b41aaefc6c6ee6218225f8dc62bba6ae5fe8f2d
Reviewed-on: https://go-review.googlesource.com/15733
Reviewed-by: Andrew Gerrand <adg@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.