Skip to content

Commit

Permalink
http2/hpack: avoid quadratic complexity in hpack decoding
Browse files Browse the repository at this point in the history
When parsing a field literal containing two Huffman-encoded strings,
don't decode the first string until verifying all data is present.
Avoids forced quadratic complexity when repeatedly parsing a partial
field, repeating the Huffman decoding of the string on each iteration.

Thanks to Philippe Antoine (Catena cyber) for reporting this issue.

Fixes golang/go#57855
Fixes CVE-2022-41723

Change-Id: I58a743df450a4a4923dddd5cf6bb0592b0a7bdf3
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1688184
TryBot-Result: Security TryBots <security-trybots@go-security-trybots.iam.gserviceaccount.com>
Reviewed-by: Julie Qiu <julieqiu@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
Reviewed-on: https://go-review.googlesource.com/c/net/+/468135
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
  • Loading branch information
neild authored and prattmic committed Feb 14, 2023
1 parent 547e7ed commit 8e2b117
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 30 deletions.
79 changes: 49 additions & 30 deletions http2/hpack/hpack.go
Original file line number Diff line number Diff line change
Expand Up @@ -359,22 +359,35 @@ func (d *Decoder) parseFieldLiteral(n uint8, it indexType) error {

var hf HeaderField
wantStr := d.emitEnabled || it.indexed()
var undecodedName undecodedString
if nameIdx > 0 {
ihf, ok := d.at(nameIdx)
if !ok {
return DecodingError{InvalidIndexError(nameIdx)}
}
hf.Name = ihf.Name
} else {
hf.Name, buf, err = d.readString(buf, wantStr)
undecodedName, buf, err = d.readString(buf)
if err != nil {
return err
}
}
hf.Value, buf, err = d.readString(buf, wantStr)
undecodedValue, buf, err := d.readString(buf)
if err != nil {
return err
}
if wantStr {
if nameIdx <= 0 {
hf.Name, err = d.decodeString(undecodedName)
if err != nil {
return err
}
}
hf.Value, err = d.decodeString(undecodedValue)
if err != nil {
return err
}
}
d.buf = buf
if it.indexed() {
d.dynTab.add(hf)
Expand Down Expand Up @@ -459,46 +472,52 @@ func readVarInt(n byte, p []byte) (i uint64, remain []byte, err error) {
return 0, origP, errNeedMore
}

// readString decodes an hpack string from p.
// readString reads an hpack string from p.
//
// wantStr is whether s will be used. If false, decompression and
// []byte->string garbage are skipped if s will be ignored
// anyway. This does mean that huffman decoding errors for non-indexed
// strings past the MAX_HEADER_LIST_SIZE are ignored, but the server
// is returning an error anyway, and because they're not indexed, the error
// won't affect the decoding state.
func (d *Decoder) readString(p []byte, wantStr bool) (s string, remain []byte, err error) {
// It returns a reference to the encoded string data to permit deferring decode costs
// until after the caller verifies all data is present.
func (d *Decoder) readString(p []byte) (u undecodedString, remain []byte, err error) {
if len(p) == 0 {
return "", p, errNeedMore
return u, p, errNeedMore
}
isHuff := p[0]&128 != 0
strLen, p, err := readVarInt(7, p)
if err != nil {
return "", p, err
return u, p, err
}
if d.maxStrLen != 0 && strLen > uint64(d.maxStrLen) {
return "", nil, ErrStringLength
// Returning an error here means Huffman decoding errors
// for non-indexed strings past the maximum string length
// are ignored, but the server is returning an error anyway
// and because the string is not indexed the error will not
// affect the decoding state.
return u, nil, ErrStringLength
}
if uint64(len(p)) < strLen {
return "", p, errNeedMore
}
if !isHuff {
if wantStr {
s = string(p[:strLen])
}
return s, p[strLen:], nil
return u, p, errNeedMore
}
u.isHuff = isHuff
u.b = p[:strLen]
return u, p[strLen:], nil
}

if wantStr {
buf := bufPool.Get().(*bytes.Buffer)
buf.Reset() // don't trust others
defer bufPool.Put(buf)
if err := huffmanDecode(buf, d.maxStrLen, p[:strLen]); err != nil {
buf.Reset()
return "", nil, err
}
type undecodedString struct {
isHuff bool
b []byte
}

func (d *Decoder) decodeString(u undecodedString) (string, error) {
if !u.isHuff {
return string(u.b), nil
}
buf := bufPool.Get().(*bytes.Buffer)
buf.Reset() // don't trust others
var s string
err := huffmanDecode(buf, d.maxStrLen, u.b)
if err == nil {
s = buf.String()
buf.Reset() // be nice to GC
}
return s, p[strLen:], nil
buf.Reset() // be nice to GC
bufPool.Put(buf)
return s, err
}
30 changes: 30 additions & 0 deletions http2/hpack/hpack_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,36 @@ func TestEmitEnabled(t *testing.T) {
}
}

func TestSlowIncrementalDecode(t *testing.T) {
// TODO(dneil): Fix for -race mode.
t.Skip("too slow in -race mode")

var buf bytes.Buffer
enc := NewEncoder(&buf)
hf := HeaderField{
Name: strings.Repeat("k", 1<<20),
Value: strings.Repeat("v", 1<<20),
}
enc.WriteField(hf)
hbuf := buf.Bytes()
count := 0
dec := NewDecoder(initialHeaderTableSize, func(got HeaderField) {
count++
if count != 1 {
t.Errorf("decoded %v fields, want 1", count)
}
if got.Name != hf.Name {
t.Errorf("decoded Name does not match input")
}
if got.Value != hf.Value {
t.Errorf("decoded Value does not match input")
}
})
for i := 0; i < len(hbuf); i++ {
dec.Write(hbuf[i : i+1])
}
}

func TestSaveBufLimit(t *testing.T) {
const maxStr = 1 << 10
var got []HeaderField
Expand Down

0 comments on commit 8e2b117

Please sign in to comment.