brbzull0 opened a new pull request, #13655:
URL: https://github.com/apache/trafficserver/pull/13655
Ten guards in `src/proxy/http3/QPACK.cc` are written
```c
if ((ret = xpack_decode_integer(tmp, ...)) < 0 && tmp > 0xFFFF) {
return -1;
}
```
With `&&`, neither condition rejects on its own: a decode failure falls
through
because the value check does not hold, and an in-range failure-free decode
of an
oversized varint falls through because the error check does not hold. The
value
is then narrowed into the surrounding `uint16_t`. The `delta_base_index`
guard in
`_decode_header()` additionally had its comparison inverted
(`delta_base_index < 0xFFFF` where `>` was meant).
Lines: 287, 925, 934, 1517, 1524, 1548, 1555, 1578, 1600, 1622.
### One bound change, and why it is required
The value guard in `_read_insert_with_name_ref()` (line 1525 on master) is
the
only one of the ten bounded at `0xFF`; the other nine use `0xFFFF`. This
raises
it to `0xFFFF` as well, and that is load bearing rather than tidying:
`value_len` is a `size_t &`, so `0xFF` is not protecting against any
narrowing --
it is a bare sanity limit on a decoded header value length. Under `&&` it
never
fired, so it did not matter. Under `||` it fires, and a `0xFF` bound would
abort
the decode for **any header value longer than 255 bytes** -- ordinary
cookies and
tokens included. At `0xFFFF` the check is unreachable, because
`xpack_decode_string()` is already capped by `_header_field_max_size` (32768
by
default), which matches the behaviour of the other nine sites.
Flipping that operator without raising the bound would be a regression, so
the
two changes belong together.
### The unchecked failure is the more interesting half
On failure `xpack_decode_integer()` returns -1. Callers then do
`read_len += ret`, and `read_len` is `size_t`, so it becomes `SIZE_MAX`.
`IOBufferReader::consume()` receives that as `int64_t` -1:
```c
ink_release_assert(n == 0 || is_read_avail_more_than(n - 1));
start_offset += n;
```
`is_read_avail_more_than(-2)` is true, so the release-assert passes and
`start_offset` moves *backwards* by one. The helper returns 0 rather than a
negative, so `_on_encoder_stream_read_ready()` does not call
`_abort_decode()`,
and its `while (reader.is_read_avail_more_than(0))` loop reads the same byte
again on the next iteration. `_read_duplicate()` (line 1578) has exactly this
shape.
Widening the destination types would address the truncation but not this
half --
the `< 0` check is what is missing.
### Relationship to #13621
#13621 also touches `_read_insert_with_name_ref()` (line 1517), widening its
`index` parameter to `uint64_t` so the narrowing disappears rather than being
rejected. That is a better fix for that one site.
This change flips the operator there too, so the ten sites are corrected
consistently and none is left with a known-broken guard while #13621 is in
review. If #13621 lands afterwards its approach supersedes the operator flip
at
that line -- a one-line resolution. Line 1524, the value guard in the same
function, is not covered by #13621 at all.
### Test
Adds `decode() rejects oversized Required Insert Count at entry` to
`test_QPACK.cc`, tagged `[qpack-decode-entry-bounds]`. It drives
`QPACK::decode()` with a 4-byte 8-bit-prefix varint encoding `0x10000` and
asserts the decoder reports failure rather than accepting it.
Confirmed to be a regression test: with the guards restored to `&&` it fails
on
both assertions with `sync_ret := 0` and
`handler->last_event() := 2700` (`QPACK_EVENT_DECODE_COMPLETE`) -- the
oversized
count is accepted and truncated to 0. With the change it passes, 3
assertions.
Also run, 5/5 pass: `h3_proxy_verifier`, `h3_python_client`,
`h3_stream_lifetime`, `h3_flow_control`, `h3_sni_check`.
The remaining eight guards have no direct test. Reaching `_decode_header()`
requires getting past the `QPACK::decode()` check this test exercises, and
the
encoder-stream helpers need a QPACK encoder stream driven with crafted
instructions, which the current test harness does not set up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
