diff --git a/src/proxy/http3/QPACK.cc b/src/proxy/http3/QPACK.cc index d3186902289..11aea57e8a7 100644 --- a/src/proxy/http3/QPACK.cc +++ b/src/proxy/http3/QPACK.cc @@ -284,7 +284,7 @@ QPACK::decode(uint64_t stream_id, const uint8_t *header_block, size_t header_blo uint64_t tmp = 0; int64_t ret = xpack_decode_integer(tmp, header_block, header_block + header_block_len, 8); - if (ret < 0 && tmp > 0xFFFF) { + if (ret < 0 || tmp > 0xFFFF) { return -1; } uint16_t largest_reference = tmp; @@ -922,7 +922,7 @@ QPACK::_decode_header(const uint8_t *header_block, size_t header_block_len, HTTP // Decode Header Data Prefix uint64_t tmp; - if ((ret = xpack_decode_integer(tmp, pos, pos + remain_len, 8)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_integer(tmp, pos, pos + remain_len, 8)) < 0 || tmp > 0xFFFF) { return -1; } pos += ret; @@ -931,7 +931,7 @@ QPACK::_decode_header(const uint8_t *header_block, size_t header_block_len, HTTP uint64_t delta_base_index; uint16_t base_index; - if ((ret = xpack_decode_integer(delta_base_index, pos, pos + remain_len, 7)) < 0 && delta_base_index < 0xFFFF) { + if ((ret = xpack_decode_integer(delta_base_index, pos, pos + remain_len, 7)) < 0 || delta_base_index > 0xFFFF) { return -2; } @@ -1514,15 +1514,15 @@ QPACK::_read_insert_with_name_ref(IOBufferReader &reader, bool &is_static, uint1 // Name Index uint64_t tmp; - if ((ret = xpack_decode_integer(tmp, input, input + input_len, 6)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_integer(tmp, input, input + input_len, 6)) < 0 || tmp > 0xFFFF) { return -1; } index = tmp; read_len += ret; // Value - if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 && - tmp > 0xFF) { + if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 || + tmp > 0xFFFF) { return -1; } value_len = tmp; @@ -1545,14 +1545,14 @@ QPACK::_read_insert_without_name_ref(IOBufferReader &reader, Arena &arena, char // Name uint64_t tmp; - if ((ret = xpack_decode_string(arena, name, tmp, input, input + input_len, _header_field_max_size, 5)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_string(arena, name, tmp, input, input + input_len, _header_field_max_size, 5)) < 0 || tmp > 0xFFFF) { return -1; } name_len = tmp; read_len += ret; // Value - if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 && + if ((ret = xpack_decode_string(arena, value, tmp, input + read_len, input + input_len, _header_field_max_size, 7)) < 0 || tmp > 0xFFFF) { return -1; } @@ -1575,7 +1575,7 @@ QPACK::_read_duplicate(IOBufferReader &reader, uint16_t &index) // Index uint64_t tmp; - if ((ret = xpack_decode_integer(tmp, input, input + input_len, 5)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_integer(tmp, input, input + input_len, 5)) < 0 || tmp > 0xFFFF) { return -1; } index = tmp; @@ -1597,7 +1597,7 @@ QPACK::_read_dynamic_table_size_update(IOBufferReader &reader, uint16_t &max_siz uint64_t tmp; // Max Size - if ((ret = xpack_decode_integer(tmp, input, input + input_len, 5)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_integer(tmp, input, input + input_len, 5)) < 0 || tmp > 0xFFFF) { return -1; } max_size = tmp; @@ -1619,7 +1619,7 @@ QPACK::_read_table_state_synchronize(IOBufferReader &reader, uint16_t &insert_co uint64_t tmp; // Insert Count - if ((ret = xpack_decode_integer(tmp, input, input + input_len, 6)) < 0 && tmp > 0xFFFF) { + if ((ret = xpack_decode_integer(tmp, input, input + input_len, 6)) < 0 || tmp > 0xFFFF) { return -1; } insert_count = tmp; diff --git a/src/proxy/http3/test/test_QPACK.cc b/src/proxy/http3/test/test_QPACK.cc index 7059c773f94..f9c1d193d27 100644 --- a/src/proxy/http3/test/test_QPACK.cc +++ b/src/proxy/http3/test/test_QPACK.cc @@ -439,6 +439,46 @@ TEST_CASE("Encoding", "[qpack-encode]") } } +// QPACK::decode() parses the Required Insert Count varint from the header +// block prefix and stores the decoded value in a uint16_t local; the call site +// must reject a varint whose value exceeds the uint16_t range, otherwise the +// value silently truncates and the decoder proceeds with corrupted state. +// Drive QPACK::decode() with a 4-byte 8-bit-prefix varint encoding 0x10000 +// and assert that the decoder reports failure rather than accepting it. +TEST_CASE("decode() rejects oversized Required Insert Count at entry", "[qpack-decode-entry-bounds]") +{ + QUICApplicationDriver driver; + auto qpack = std::make_unique(driver.get_connection(), UINT32_MAX, 4096, 100, MAX_FIELD_SIZE); + auto handler = std::make_unique(); + + HTTPHdr hdr; + hdr.create(HTTPType::REQUEST); + + uint8_t block[16] = {0}; + uint8_t *p = block; + int enc_len = xpack_encode_integer(p, p + sizeof(block), 0x10000, 8); + REQUIRE(enc_len > 0); + p += enc_len; + *p++ = 0x00; + size_t block_len = static_cast(p - block); + + int sync_ret = qpack->decode(1, block, block_len, hdr, handler.get(), eventProcessor.all_ethreads[0]); + + // decode() schedules its result asynchronously when it returns >= 0; only + // wait in that case. A synchronous failure (sync_ret < 0) means no event + // will be delivered and there is nothing to wait for. + if (sync_ret >= 0) { + sleep(1); + } + + CAPTURE(sync_ret); + CAPTURE(handler->last_event()); + CHECK_FALSE((sync_ret == 0 && handler->last_event() == QPACK_EVENT_DECODE_COMPLETE)); + CHECK((sync_ret < 0 || handler->last_event() == QPACK_EVENT_DECODE_FAILED)); + + hdr.destroy(); +} + TEST_CASE("Decoding", "[qpack-decode]") { char app_dir[PATH_MAX + 1] = "";