From 8e2b117aee74f6b86c207a808b0255de45c0a18a Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 22 Dec 2022 09:33:10 -0800 Subject: [PATCH] http2/hpack: avoid quadratic complexity in hpack decoding 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 Reviewed-by: Julie Qiu Run-TryBot: Damien Neil Reviewed-by: Roland Shoemaker Reviewed-on: https://go-review.googlesource.com/c/net/+/468135 Run-TryBot: Michael Pratt Reviewed-by: Roland Shoemaker Reviewed-by: Than McIntosh Auto-Submit: Michael Pratt TryBot-Result: Gopher Robot --- http2/hpack/hpack.go | 79 ++++++++++++++++++++++++--------------- http2/hpack/hpack_test.go | 30 +++++++++++++++ 2 files changed, 79 insertions(+), 30 deletions(-) diff --git a/http2/hpack/hpack.go b/http2/hpack/hpack.go index b184a2771a..7a1d976696 100644 --- a/http2/hpack/hpack.go +++ b/http2/hpack/hpack.go @@ -359,6 +359,7 @@ 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 { @@ -366,15 +367,27 @@ func (d *Decoder) parseFieldLiteral(n uint8, it indexType) error { } 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) @@ -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 } diff --git a/http2/hpack/hpack_test.go b/http2/hpack/hpack_test.go index f9092e8bb9..b4b2a5d666 100644 --- a/http2/hpack/hpack_test.go +++ b/http2/hpack/hpack_test.go @@ -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