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 <[email protected]>
Reviewed-by: Julie Qiu <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/net/+/468135
Run-TryBot: Michael Pratt <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Reviewed-by: Than McIntosh <[email protected]>
Auto-Submit: Michael Pratt <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

http2/hpack/hpack.go

@@ -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)
@@ -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
 }

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