From 8f6cb8f45235d166ee35cc6df8750f4be759481e Mon Sep 17 00:00:00 2001 From: Frederic Lecaille Date: Fri, 20 Mar 2026 19:30:35 +0100 Subject: [PATCH] BUG/MINOR: qpack: fix 62-bit overflow and 1-byte OOB reads in decoding 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 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 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 --- src/qpack-dec.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/qpack-dec.c b/src/qpack-dec.c index 5e7a243a6..e36eb75f3 100644 --- a/src/qpack-dec.c +++ b/src/qpack-dec.c @@ -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 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 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;