BUG/MINOR: qpack: missing shift count check in qpack_get_varint() (UB)

Parsing QPACK varints with excessive continuation bytes increases shift
by 7 on each iteration. When shift reaches 63 or higher, evaluating:

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

executes a bitwise shift on a 64-bit integer with an out-of-range count,
which is Undefined Behavior in C.

Fix this by jumping to too_large label if shift >= 63 before the shift runs.

Thank you to @OneZ3r0 for having reported this issue.

Should be backported to all versions.

diff --git a/src/qpack-dec.c b/src/qpack-dec.c
index 7016553f1ae5b06a09b8269e135566ffec51e0df..e1c14716dc7422f49de89da6b6ddf888bb8eb4aa 100644 (file)
--- a/src/qpack-dec.c
+++ b/src/qpack-dec.c
@@ -70,11 +70,16 @@ static uint64_t qpack_get_varint(const unsigned char **buf, uint64_t *len_in, in
                if (!len)
                        goto too_short;
 
+               /* A valid QPACK varint is at most 62 bits. Zero-payload
+                * continuations must not drive shift past 63, otherwise the
+                * "(limit - ret) >> shift" check below runs with an out-of-range
+                * shift count (C undefined behavior).
+                */
+               if (shift >= 63)
+                       goto too_large;
                v = *raw++;
                len--;
-               /* This check is sufficient to prevent any overflow
-                * and implicitly limits shift to 63.
-                */
+               /* This check is sufficient to prevent any overflow. */
                if ((v & 127) > (limit - ret) >> shift)
                        goto too_large;