x/net/http2/hpack: Possible decoding mistake in HPACK decoder #15614
Labels
Milestone
Comments
|
Do you want to fix? |
|
@bradfitz Sure, I just wanted to confirm it was actually a bug and not a misreading of the spec. I think I already have a CLA for Go from a few years back. I'll send out a CL. |
|
Googlers don't need to do a CLA. I don't remember that part of the spec, so it's likely I just missed it the first time. I at least don't see a test for it. Can you also add a test for the EOS case? ("A Huffman-encoded string literal containing the EOS symbol MUST be treated as a decoding error.") |
|
Sure. I'll have a CL by tomorrow probably. |
|
CL https://golang.org/cl/23067 mentions this issue. |
mk0x9
pushed a commit
to mk0x9/go
that referenced
this issue
May 18, 2016
Updates x/net/http2/hpack to rev 6050c111 for: http2/hpack: forbid excess and invalid padding in hpack decoder https://golang.org/cl/23067 Updates golang#15614 Change-Id: I3fbf9b265bfa5e49e6aa97d8c34e08214cfcc49a Reviewed-on: https://go-review.googlesource.com/23208 Reviewed-by: Carl Mastrangelo <notcarl@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
c3mb0
pushed a commit
to c3mb0/net
that referenced
this issue
Apr 2, 2018
This change fixes a few bugs in the HPACK decoder:
* Excess trailing padding is treated as an error per the HPACK Spec
section 5.2
* Non EOS prefix padding is treated as an error
* Max length is now enforced for all decoded symbols
The idea here is to keep track of the decoded symbol length, rather
than the number of unconsumed bits in cur. To this end, nbits has
been renamed cbits (cur bits), and sbits (sym bits) has been
introduced. The main problem with using nbits is that it can easily
be zero, such as when decoding {0xff, 0xff}. Using a clear moniker
makes it easier to see why checking cbits > 0 at the end of the
function is incorrect.
Fixes golang/go#15614
Change-Id: I1ae868caa9c207fcf9c9dec7f10ee9f400211f99
Reviewed-on: https://go-review.googlesource.com/23067
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Please answer these questions before submitting your issue. Thanks!
go version)?go version go1.6 linux/amd64
go env)?GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GORACE=""
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
https://play.golang.org/p/GCCEi-6zBN
An error
No error
I am working on an HTTP/2 compliance test which involves Fuzz testing the HPACK Decoder. The problem was originally found in the Java implementation of Netty. When I looked to see how Go was handling it, it seemed to be different from Netty and also different from the Spec.
Two 0xFF bytes in the input are interpreted as trailing padding in a huffman encoded value. From reading the spec, the last paragraph says that padding in excess of 7 bits is an error (rather than being ignored). I believe this should be a decoding error, rather than being treated as an empty string as the code above behaves.
Note that the string in the example was shortened for brevity. The original suspect bytestring is:
The text was updated successfully, but these errors were encountered: