track remaining length while decoding qpack header block - #13361
Conversation
|
[approve ci] |
|
The AuTests proxy_serve_stale_dns and subcookie spuriously failed. |
|
yeah, those two look unrelated to this change. proxy_serve_stale_dns sits in the cache/dns path and subcookie is a cookie_remap plugin test, neither goes anywhere near the http/3 decode, so I'm fairly sure it's just flake. happy to have ci re-run if that clears it. if it'd help the fix land, I can add a small decode regression test that feeds a crafted header block through the public decode api and checks it's rejected rather than over-reading past the block. let me know if you'd want that alongside this. |
|
It's awesome to have more tests, but it's up to you whether you think the test is worth it. I'm happy to approve either way. We can rerun CI once it checks off the Rocky build. I think if you reapprove individual CI jobs with our |
|
Thanks! Sounds good. I'll wait for the Rocky build to finish and then we can rerun CI if needed. |
|
The Rocky job ran into a network issue and couldn't publish its success. I'll see if we can restart it now. |
|
Thank you for the review and merge. Appreciated. |
(cherry picked from commit 082e875)
The QPACK header decoder takes a header block off an HTTP/3 request stream and walks it field by field. In _decode_header the remaining-length counter is set once to the full block size and then never reduced as the cursor moves forward, so every per-field decoder is handed a buffer end of pos plus the whole block length rather than pos plus what is actually left. After the first field is consumed that end sits past the real end of the buffer, by however many bytes have already been read. A client can lean on this by sending a block whose later field declares a string length that reaches beyond the true end but still fits inside the inflated bound, which slips past the length check and makes the following copy read off the end of the allocation. I noticed it while tracing why the end pointer handed to the sub-decoders did not match the block the frame layer had buffered, and an address-sanitiser run on a crafted block confirmed the over-read. The fix keeps remain_len in step with the cursor by subtracting each consumed length, so the derived end always points at the real end of the block. I kept the change inside _decode_header so the sub-decoders and the rest of the path are left alone.