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: zero-length payload HEADERS frame causes PROTOCOL_ERROR #47851

Closed
zliu5-sc opened this issue Aug 20, 2021 · 5 comments
Closed

x/net/http2: zero-length payload HEADERS frame causes PROTOCOL_ERROR #47851

zliu5-sc opened this issue Aug 20, 2021 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@zliu5-sc
Copy link

zliu5-sc commented Aug 20, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.6 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY="*.sc-corp.net"
GONOSUMDB="*.sc-corp.net"
GOOS="darwin"
GOPRIVATE="*.sc-corp.net"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.16.6/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.16.6/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.6"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/72/p_z4fcg52bb7sg6vwpxckktm0000gp/T/go-build2392873128=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Making an HTTP/2 request to server, which returns a zero-length payload trailer (HEADERS frame) as part of the response.

Zero-length payload HEADERS

We encountered this issue while proxying a gprc-web request to an upstream which uses the grpc-web filter from Envoy. Among other things, grpc-web appends gRPC trailers as part of the response body. When Envoy does it, it clears out the trailers, leaving a zero-length trailer (HEADERS) frame.

What did you expect to see?

Be able to handle the zero-length payload trailer.

For instance, nghttp handles it just fine:

$ echo -ne '\x00\x00\x00\x00\x00' | nghttp -v -H 'Content-Type: application/grpc-web' -d - https://localhost:8043/grpc.health.v1.Health/Check

...
[  1.324] recv DATA frame <length=73, flags=0x00, stream_id=13>
[  1.324] recv HEADERS frame <length=0, flags=0x05, stream_id=13> # <-- HEADERS frame with zero-length payload
          ; END_STREAM | END_HEADERS
          (padlen=0)

What did you see instead?

ERROR_PROTOCOL being returned.

Analysis

We traced it down to this line:

https://github.com/golang/net/blob/60bc85c4be6d32924bcfddb728394cb8713f2c78/http2/frame.go#L1021

if len(p)-int(padLength) <= 0 {

for a HEADERS frame with zero-length payload, p is of size zero. As a result even though padLength is also 0 (as a matter of fact, this frame does not have any padding flag set, this is simply default value of uint8), this check fails due to <= instead of <.

We believe the existing check is incorrect, as per RFC7540 Section 6.2:

Padding that exceeds the size remaining for the header block fragment MUST be treated as a PROTOCOL_ERROR.

(emphasis added)

The existing behavior is inconsistent with the RFC as it incorrectly returns PROTOCOL_ERROR when the padding (None / 0) does NOT exceed the size remaining (0)

Proposed Fix (PR to follow)

This should be a < instead of <= check.

Changing this check to < is also consistent with existing code in methods such as parseDataFrame:

https://github.com/golang/net/blob/60bc85c4be6d32924bcfddb728394cb8713f2c78/http2/frame.go#L611

zliu5-sc added a commit to zliu5-sc/net that referenced this issue Aug 20, 2021
As per [RFC7540 Section 6.2](https://httpwg.org/specs/rfc7540.html#HEADERS):

> Padding that *exceeds* the size remaining for the header block fragment MUST be treated as a PROTOCOL_ERROR.

(emphasis added)

Hence the existing check `if len(p)-int(padLength) <= 0` is incorrect
since `=` would have flagged paddings that is *exactly* the size
remaining. The existing check also triggers false positives when the
HEADERS frame has a payload size of 0 and no padding (0 - 0 <= 0).

Fixes golang/go#47851
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 20, 2021
@mknyszek mknyszek added this to the Backlog milestone Aug 20, 2021
@mknyszek
Copy link
Contributor

CC @neild

@neild
Copy link
Contributor

neild commented Aug 20, 2021

Nice analysis. The fix looks correct to me. If you'd rather not deal with the CLA and code review in Gerrit, let me know and I can make the fix instead.

@zliu5-sc
Copy link
Author

Thanks @mknyszek @neild !

We signed our corp CLA yesterday but I guess it takes time to process. Given the fix is simple I'd be more than happy to have you do the fix.

@gopherbot
Copy link

Change https://golang.org/cl/344029 mentions this issue: http2: accept zero-length block fragments in HEADERS frames

@jroper
Copy link

jroper commented Oct 12, 2021

Thanks @zliu5! This bug was impacting us too when communicating with a proxy we'd written that was receiving http/2 requests and making http/1.1 requests, using akka-http which actually exposes the chunked protocol, and since http/1.1 responses always have an empty chunk at the end of them whether there's trailers or not because that indicates end of stream, our code was forwarding that last chunk message with empty trailers as an empty trailers message downstream to the http/2 connection, which caused go clients to choke.

raboof added a commit to raboof/akka-http that referenced this issue Oct 12, 2021
When ending the stream without any payload, use a DATA frame rather than
a HEADERS frame to work around golang/go#47851.

Tested with https://github.com/raboof/http2-from-go-to-akka-http
raboof added a commit to raboof/akka-http that referenced this issue Oct 13, 2021
When ending the stream without any payload, use a DATA frame rather than
a HEADERS frame to work around golang/go#47851.

Tested with https://github.com/raboof/http2-from-go-to-akka-http
dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
Fix a fencepost error which rejects HEADERS frames with neither
payload nor padding, but accepts ones with padding and no payload.

Fixes golang/go#47851.

Change-Id: Ic6ed65571366e86980a5ec69e7f9e6ab958f3b07
Reviewed-on: https://go-review.googlesource.com/c/net/+/344029
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Oct 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants