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

[zliu5-sc]

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

[mknyszek]

CC @neild

[neild]

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]

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]

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

[jroper]

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.