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 <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

diff --git a/src/qpack-dec.c b/src/qpack-dec.c
index 5e7a243a6ccfe493029422ae1f46c67ac95edc30..e36eb75f31da5dc3d091d9c59a63d10ec8758d6e 100644 (file)
--- 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 <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;