From 39053a65e35c5ae9877ec1c85714b835a4ddb881 Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Thu, 14 May 2026 12:46:32 -0600 Subject: [PATCH 1/4] Validate m_frag_offset_count and offset during cache unmarshal --- src/proxy/hdrs/HTTP.cc | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/proxy/hdrs/HTTP.cc b/src/proxy/hdrs/HTTP.cc index b67534d6de6..b66688b3c27 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 (alt->m_frag_offset_count < 0 || len < frag_table_size || frag_offset < 0 || orig_len < frag_offset + frag_table_size) { + ink_assert(!"HTTPInfo::unmarshal m_frag_offset_count or offset exceeds buffer"); + 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 (alt->m_frag_offset_count < 0 || len < extra64 || frag_offset < 0 || orig_len < frag_offset + extra64) { + ink_assert(!"HTTPInfo::unmarshal_v24_1 m_frag_offset_count or offset exceeds buffer"); + 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 From 61acdaf1e21be023156a6f134b96ec2b2b212b71 Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Fri, 15 May 2026 07:28:00 -0600 Subject: [PATCH 2/4] HTTP.cc: remove unecessary <0 check, add unit tests --- src/proxy/hdrs/HTTP.cc | 4 +- src/proxy/hdrs/unit_tests/test_Hdrs.cc | 88 ++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/src/proxy/hdrs/HTTP.cc b/src/proxy/hdrs/HTTP.cc index b66688b3c27..deafc257ef3 100644 --- a/src/proxy/hdrs/HTTP.cc +++ b/src/proxy/hdrs/HTTP.cc @@ -2216,7 +2216,7 @@ HTTPInfo::unmarshal(char *buf, int len, RefCountObj *block_ref) 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 (alt->m_frag_offset_count < 0 || len < frag_table_size || frag_offset < 0 || orig_len < frag_offset + frag_table_size) { + if (frag_offset < 0 || len < frag_table_size || static_cast(orig_len) - frag_offset < frag_table_size) { ink_assert(!"HTTPInfo::unmarshal m_frag_offset_count or offset exceeds buffer"); return -1; } @@ -2291,7 +2291,7 @@ HTTPInfo::unmarshal_v24_1(char *buf, int len, RefCountObj *block_ref) 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 (alt->m_frag_offset_count < 0 || len < extra64 || frag_offset < 0 || orig_len < frag_offset + extra64) { + if (frag_offset < 0 || len < extra64 || static_cast(orig_len) - frag_offset < extra64) { ink_assert(!"HTTPInfo::unmarshal_v24_1 m_frag_offset_count or offset exceeds buffer"); return -1; } diff --git a/src/proxy/hdrs/unit_tests/test_Hdrs.cc b/src/proxy/hdrs/unit_tests/test_Hdrs.cc index 4e90846edb4..e4511e01b9c 100644 --- a/src/proxy/hdrs/unit_tests/test_Hdrs.cc +++ b/src/proxy/hdrs/unit_tests/test_Hdrs.cc @@ -2696,3 +2696,91 @@ 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) +{ + // Allocate generously so the struct itself always fits. + 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); + } +} From 01a1625a9dc64cf721563b04e59cdcbc7c9a4a1e Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Fri, 15 May 2026 08:05:24 -0600 Subject: [PATCH 3/4] change cache corruption ink_asserts into run time Warnings --- src/proxy/hdrs/HTTP.cc | 4 ++-- src/proxy/hdrs/unit_tests/test_Hdrs.cc | 9 ++++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/proxy/hdrs/HTTP.cc b/src/proxy/hdrs/HTTP.cc index deafc257ef3..f7f80d7117e 100644 --- a/src/proxy/hdrs/HTTP.cc +++ b/src/proxy/hdrs/HTTP.cc @@ -2217,7 +2217,7 @@ HTTPInfo::unmarshal(char *buf, int len, RefCountObj *block_ref) 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) { - ink_assert(!"HTTPInfo::unmarshal m_frag_offset_count or offset exceeds buffer"); + 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); @@ -2292,7 +2292,7 @@ HTTPInfo::unmarshal_v24_1(char *buf, int len, RefCountObj *block_ref) intptr_t frag_offset = reinterpret_cast(alt->m_frag_offsets); if (frag_offset < 0 || len < extra64 || static_cast(orig_len) - frag_offset < extra64) { - ink_assert(!"HTTPInfo::unmarshal_v24_1 m_frag_offset_count or offset exceeds buffer"); + Warning("HTTPInfo::unmarshal_v24_1: m_frag_offset_count or offset exceeds buffer - corrupt cache entry"); return -1; } diff --git a/src/proxy/hdrs/unit_tests/test_Hdrs.cc b/src/proxy/hdrs/unit_tests/test_Hdrs.cc index e4511e01b9c..c10d71e70ed 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]") @@ -2706,7 +2707,13 @@ TEST_CASE("HdrPromotesOnlyValidHostHeaderMutations", "[proxy][hdrtest]") static std::vector make_marshalled_alt(int frag_offset_count, intptr_t frag_ptr_value) { - // Allocate generously so the struct itself always fits. + // Ensure diags are initialized so Warning() calls in unmarshal() don't crash. + 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; From d39215aa629ca8ee6cb16aef56df253405969e0f Mon Sep 17 00:00:00 2001 From: Brian Olsen Date: Fri, 15 May 2026 08:23:48 -0600 Subject: [PATCH 4/4] for test warning helper add indicator that variable may be unused --- src/proxy/hdrs/unit_tests/test_Hdrs.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/proxy/hdrs/unit_tests/test_Hdrs.cc b/src/proxy/hdrs/unit_tests/test_Hdrs.cc index c10d71e70ed..093e0fb7b31 100644 --- a/src/proxy/hdrs/unit_tests/test_Hdrs.cc +++ b/src/proxy/hdrs/unit_tests/test_Hdrs.cc @@ -2708,7 +2708,7 @@ 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. - static bool diags_initialized = []() { + [[maybe_unused]] static bool diags_initialized = []() { if (diags() == nullptr) { DiagsPtr::set(new Diags("test_hdrs", nullptr, nullptr, new BaseLogFile("stderr"))); }