Bound the integers HPACK will decode - #181

decodeI accumulated with j + (b .&. 0x7f) * 2 ^ m and stopped only when the sender stopped setting the continuation bit:

    decode m j = do
        b <- fromIntegral <$> read8 rbuf
        let j' = j + (b .&. 0x7f) * 2 ^ m
            m' = m + 7
            cont = b `testBit` 7
        if cont then decode m' j' else return j'

Both the shift and the sum overflow — 2 ^ m is 0 once m passes the word width, and the total wraps well before that — and neither was checked. A long enough encoding therefore decodes to whatever value the sender aims at:

>>> decodeInteger 7 127 [0x83,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x01]
2

Put 0xff in front of those ten octets and decode the lot as a header block:

>>> withDynamicTableForDecoding 4096 4096 $ \dt -> decodeHeader dt block
[(":method","GET")]

which is what the single octet 0x82 means. Eleven bytes and one byte that decode alike is what RFC 7541 section 5.1 asks a decoder to refuse:

Integer encodings that exceed implementation limits -- in value or octet length -- MUST be treated as decoding errors. Different limits can be set for each of the different uses of integers, based on implementation constraints.

Beyond the ambiguity, the same encoding reaches toIndexedEntry and decodeS as an index and as a string length, so what the accumulator wraps to decides which table entry is read and how many bytes are taken.

The limit

2^30 - 1. Far above anything HTTP/2 can ask for — a frame payload is at most 2^24 - 1 octets, so no length or index comes near it — and it still fits in an Int where that is 32 bits wide. Reaching it takes five octets, so the shift is capped at 28 and every octet after that is an overflow by construction.

The value test happens before the shift, comparing the incoming digit against (integerLimit - j) >> m rather than forming the product, so nothing overflows on the way to noticing that something would have. The octet-count test comes first for the same reason: shifting an Int by a word width or more is not defined to give zero.

TooLargeInteger joins DecodeError. That is an added constructor on an exported type, so it wants a minor bump; at the HTTP/2 layer it reaches the peer as COMPRESSION_ERROR along with every other decoding error, via the existing catch-all in hpackDecodeTrailer. Say the word if you would rather reuse an existing constructor than add one.

One behaviour change worth naming: a dynamic table size update above 2^30 - 1 is now a TooLargeInteger rather than a TooLargeTableSize. Both are decoding errors and both become COMPRESSION_ERROR, and a receiver rejects anything above its own advertised table size long before that anyway.

Tests

decodeInteger
  rejects an encoding that runs past the limit [OK]
  rejects an encoding in more octets than the limit can take [OK]
  rejects an encoding that would wrap around [OK]

All three fail against the old arithmetic — checked by reverting just the body of decode and keeping the rest:

rejects an encoding that runs past the limit [FAILED]
rejects an encoding in more octets than the limit can take [FAILED]
rejects an encoding that would wrap around [FAILED]
  did not get expected exception: DecodeError

The existing duality properties and the HPACK test-case corpus are untouched and still pass. Full suite: spec 40 examples / 0 failures, hpack and frame pass, doctest 62 examples / 0 failures, fourmolu clean.

Not included

No ChangeLog entry or version bump, same as the last few.

Still outstanding from the audit: the error call reachable from a padded zero-length DATA frame, and stream errors being escalated to connection errors.

🤖 Generated with Claude Code