diff --git a/src/proxy/hdrs/HTTP.cc b/src/proxy/hdrs/HTTP.cc index b67534d6de6..f7f80d7117e 100644 --- a/src/proxy/hdrs/HTTP.cc +++ b/src/proxy/hdrs/HTTP.cc @@ -2212,9 +2212,16 @@ HTTPInfo::unmarshal(char *buf, int len, RefCountObj *block_ref) len -= HTTP_ALT_MARSHAL_SIZE; if (alt->m_frag_offset_count > HTTPCacheAlt::N_INTEGRAL_FRAG_OFFSETS) { - alt->m_frag_offsets = reinterpret_cast(buf + reinterpret_cast(alt->m_frag_offsets)); - len -= sizeof(FragOffset) * alt->m_frag_offset_count; - ink_assert(len >= 0); + // Validate that m_frag_offset_count is sane: the fragment offset table must fit within the remaining buffer. + int64_t frag_table_size = static_cast(sizeof(FragOffset)) * alt->m_frag_offset_count; + intptr_t frag_offset = reinterpret_cast(alt->m_frag_offsets); + + if (frag_offset < 0 || len < frag_table_size || static_cast(orig_len) - frag_offset < frag_table_size) { + Warning("HTTPInfo::unmarshal: m_frag_offset_count or offset exceeds buffer - corrupt cache entry"); + return -1; + } + alt->m_frag_offsets = reinterpret_cast(buf + frag_offset); + len -= static_cast(frag_table_size); } else if (alt->m_frag_offset_count > 0) { alt->m_frag_offsets = alt->m_integral_frag_offsets; } else { @@ -2279,9 +2286,19 @@ HTTPInfo::unmarshal_v24_1(char *buf, int len, RefCountObj *block_ref) len -= HTTP_ALT_MARSHAL_SIZE; if (alt->m_frag_offset_count > HTTPCacheAlt::N_INTEGRAL_FRAG_OFFSETS) { + // Validate that m_frag_offset_count is sane before computing sizes. + int64_t frag_table_size = static_cast(sizeof(FragOffset)) * alt->m_frag_offset_count; + int64_t extra64 = frag_table_size - static_cast(sizeof(alt->m_integral_frag_offsets)); + intptr_t frag_offset = reinterpret_cast(alt->m_frag_offsets); + + if (frag_offset < 0 || len < extra64 || static_cast(orig_len) - frag_offset < extra64) { + Warning("HTTPInfo::unmarshal_v24_1: m_frag_offset_count or offset exceeds buffer - corrupt cache entry"); + return -1; + } + // stuff that didn't fit in the integral slots. - int extra = sizeof(FragOffset) * alt->m_frag_offset_count - sizeof(alt->m_integral_frag_offsets); - char *extra_src = buf + reinterpret_cast(alt->m_frag_offsets); + int extra = static_cast(extra64); + char *extra_src = buf + frag_offset; // Actual buffer size, which must be a power of two. // Well, technically not, because we never modify an unmarshalled fragment // offset table, but it would be a nasty bug should that be done in the diff --git a/src/proxy/hdrs/unit_tests/test_Hdrs.cc b/src/proxy/hdrs/unit_tests/test_Hdrs.cc index 4e90846edb4..093e0fb7b31 100644 --- a/src/proxy/hdrs/unit_tests/test_Hdrs.cc +++ b/src/proxy/hdrs/unit_tests/test_Hdrs.cc @@ -44,6 +44,7 @@ using namespace std::literals; #include "proxy/hdrs/HTTP.h" #include "proxy/hdrs/HttpCompat.h" +#include "tscore/Diags.h" // replaces test_http_parser_eos_boundary_cases TEST_CASE("HdrTestHttpParse", "[proxy][hdrtest]") @@ -2696,3 +2697,97 @@ TEST_CASE("HdrPromotesOnlyValidHostHeaderMutations", "[proxy][hdrtest]") http_parser_clear(&parser); req_hdr.destroy(); } + +// --------------------------------------------------------------------------- +// Helpers for HTTPInfo::unmarshal frag-offset bounds-check tests. +// Builds a minimal marshalled HTTPCacheAlt buffer. All header-heap pointers +// are left null so unmarshal() returns after the frag-offset check without +// requiring a real heap. +// --------------------------------------------------------------------------- +static std::vector +make_marshalled_alt(int frag_offset_count, intptr_t frag_ptr_value) +{ + // Ensure diags are initialized so Warning() calls in unmarshal() don't crash. + [[maybe_unused]] static bool diags_initialized = []() { + if (diags() == nullptr) { + DiagsPtr::set(new Diags("test_hdrs", nullptr, nullptr, new BaseLogFile("stderr"))); + } + return true; + }(); + std::vector buf(sizeof(HTTPCacheAlt) + 4096, 0); + auto *alt = reinterpret_cast(buf.data()); + alt->m_magic = CacheAltMagic::MARSHALED; + alt->m_writeable = 0; + alt->m_unmarshal_len = -1; + alt->m_frag_offset_count = frag_offset_count; + // Store the raw offset value in the pointer field (mirrors what marshal() does). + *reinterpret_cast(&alt->m_frag_offsets) = frag_ptr_value; + return buf; +} + +TEST_CASE("HTTPInfo::unmarshal frag bounds checks", "[proxy][hdrtest][unmarshal]") +{ + SECTION("huge frag_offset_count rejected") + { + auto buf = make_marshalled_alt(INT_MAX, 0); + int len = static_cast(buf.size()); + CHECK(HTTPInfo::unmarshal(buf.data(), len, nullptr) == -1); + } + + SECTION("negative frag_offset pointer value rejected") + { + // count > N_INTEGRAL_FRAG_OFFSETS but the stored offset is negative + auto buf = make_marshalled_alt(5, -1); + int len = static_cast(buf.size()); + CHECK(HTTPInfo::unmarshal(buf.data(), len, nullptr) == -1); + } + + SECTION("frag pointer near INTPTR_MAX rejected (overflow-safe check)") + { + // Without the subtraction-form check, frag_offset + frag_table_size wraps. + auto buf = make_marshalled_alt(5, std::numeric_limits::max() - 1); + int len = static_cast(buf.size()); + CHECK(HTTPInfo::unmarshal(buf.data(), len, nullptr) == -1); + } + + SECTION("frag offset beyond orig_len rejected") + { + auto buf = make_marshalled_alt(5, 0); + // Set offset to one byte past the end of the buffer. + int buf_len = static_cast(buf.size()); + auto buf2 = make_marshalled_alt(5, static_cast(buf_len + 1)); + CHECK(HTTPInfo::unmarshal(buf2.data(), buf_len, nullptr) == -1); + } +} + +TEST_CASE("HTTPInfo::unmarshal_v24_1 frag bounds checks", "[proxy][hdrtest][unmarshal]") +{ + SECTION("huge frag_offset_count rejected") + { + auto buf = make_marshalled_alt(INT_MAX, 0); + int len = static_cast(buf.size()); + CHECK(HTTPInfo::unmarshal_v24_1(buf.data(), len, nullptr) == -1); + } + + SECTION("negative frag_offset pointer value rejected") + { + auto buf = make_marshalled_alt(5, -1); + int len = static_cast(buf.size()); + CHECK(HTTPInfo::unmarshal_v24_1(buf.data(), len, nullptr) == -1); + } + + SECTION("frag pointer near INTPTR_MAX rejected (overflow-safe check)") + { + auto buf = make_marshalled_alt(5, std::numeric_limits::max() - 1); + int len = static_cast(buf.size()); + CHECK(HTTPInfo::unmarshal_v24_1(buf.data(), len, nullptr) == -1); + } + + SECTION("frag offset beyond orig_len rejected") + { + auto buf = make_marshalled_alt(5, 0); + int buf_len = static_cast(buf.size()); + auto buf2 = make_marshalled_alt(5, static_cast(buf_len + 1)); + CHECK(HTTPInfo::unmarshal_v24_1(buf2.data(), buf_len, nullptr) == -1); + } +}