upstream/haproxy
1
0
Fork 0
mirror of https://github.com/haproxy/haproxy.git synced 2026-03-21 10:00:32 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 8

BUG/MINOR: qpack: fix 62-bit overflow and 1-byte OOB reads in decoding
Some checks are pending
Contrib / build (push) Waiting to run
Details
alpine/musl / gcc (push) Waiting to run
Details
VTest / Generate Build Matrix (push) Waiting to run
Details
VTest / (push) Blocked by required conditions
Details
Windows / Windows, gcc, all features (push) Waiting to run
Details

Browse source 
This patch improves the robustness of the QPACK varint decoder and fixes
potential 1-byte out-of-bounds reads in qpack_decode_fs().

In qpack_decode_fs(), two 1-byte OOB reads were possible on truncated
streams between two varint decoding. These occurred when trying to read
the byte containing the Huffman bit <h> and the Value Length prefix
immediately following an Index or a Name Length.

Note that these OOB are limited to a single byte because
qpack_get_varint() already ensures that its input length is non-zero
before consuming any data.

The fixes in qpack_decode_fs() are:
- When decoding an index, we now verify that at least one byte remains
  to safely access the following <h> bit and value length.
- When decoding a literal, we now check len < name_len + 1 to ensure
  the byte starting the header value is reachable.

In qpack_get_varint(), the maximum value is now strictly capped at 2^62-1
as per RFC. This is enforced using a budget-based check:

   (v & 127) > (limit - ret) >> shift

This prevents values from  overflowing into the 63rd or 64th bits, which
would otherwise break subsequent signed comparisons (e.g., if (len < name_len))
by interpreting the length as a negative value, leading to false positive
tests.

Thank you to @jming912 for having reported this issue in GH #3302.

Must be backported as far as 2.6
This commit is contained in:
Frederic Lecaille 2026-03-20 19:30:35 +01:00
parent 60c9e2975b
commit 8f6cb8f452
1 changed files with 22 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

36
src/qpack-dec.c
View file

@ -61,10 +61,10 @@
static uint64_t qpack_get_varint(const unsigned char **buf, uint64_t *len_in, int b)
{
uint64_t ret = 0;
int len = *len_in;
uint64_t len = *len_in;
const uint8_t *raw = *buf;
uint64_t v, max = ~0;
uint8_t shift = 0;
uint64_t v, limit = (1ULL << 62) - 1;
int shift = 0;
if (len == 0)
goto too_short;
@ -77,24 +77,26 @@ static uint64_t qpack_get_varint(const unsigned char **buf, uint64_t *len_in, in
do {
if (!len)
goto too_short;
v = *raw++;
len--;
if (v & 127) { // make UBSan happy
if ((v & 127) > max)
goto too_large;
ret += (v & 127) << shift;
}
max >>= 7;
/* This check is sufficient to prevent any overflow
* and implicitly limits shift to 63.
*/
if ((v & 127) > (limit - ret) >> shift)
goto too_large;
ret += (v & 127) << shift;
shift += 7;
} while (v & 128);
end:
end:
*buf = raw;
*len_in = len;
return ret;
too_large:
too_short:
too_large:
too_short:
*len_in = (uint64_t)-1;
return 0;
}
@ -402,7 +404,10 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
n = efl_type & 0x20;
static_tbl = efl_type & 0x10;
index = qpack_get_varint(&raw, &len, 4);
if (len == (uint64_t)-1) {
/* There must be at least one byte available for <h> value after this
* decoding before the next call to qpack_get_varint().
*/
if ((int64_t)len <= 0) {
qpack_debug_printf(stderr, "##ERR@%d\n", __LINE__);
ret = -QPACK_RET_TRUNCATED;
goto out;
@ -474,7 +479,10 @@ int qpack_decode_fs(const unsigned char *raw, uint64_t len, struct buffer *tmp,
n = *raw & 0x10;
hname = *raw & 0x08;
name_len = qpack_get_varint(&raw, &len, 3);
if (len == (uint64_t)-1 || len < name_len) {
/* There must be at least one byte available for <hvalue> after this
* decoding before the next call to qpack_get_varint().
*/
if ((int64_t)len < (int64_t)name_len + 1) {
qpack_debug_printf(stderr, "##ERR@%d\n", __LINE__);
ret = -QPACK_RET_TRUNCATED;
goto out;