Skip to content

Validate m_frag_offset_count and offset during cache unmarshal #13165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
traeak wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Validate m_frag_offset_count and offset during cache unmarshal #13165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39053a6
Validate m_frag_offset_count and offset during cache unmarshal
traeak May 14, 2026
61acdaf
HTTP.cc: remove unecessary <0 check, add unit tests
traeak May 15, 2026
01a1625
change cache corruption ink_asserts into run time Warnings
traeak May 15, 2026
d39215a
for test warning helper add indicator that variable may be unused
traeak May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions src/proxy/hdrs/HTTP.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<FragOffset *>(buf + reinterpret_cast<intptr_t>(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<int64_t>(sizeof(FragOffset)) * alt->m_frag_offset_count;
intptr_t frag_offset = reinterpret_cast<intptr_t>(alt->m_frag_offsets);

if (frag_offset < 0 || len < frag_table_size || static_cast<int64_t>(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<FragOffset *>(buf + frag_offset);
len -= static_cast<int>(frag_table_size);
} else if (alt->m_frag_offset_count > 0) {
alt->m_frag_offsets = alt->m_integral_frag_offsets;
} else {
Expand Down Expand Up @@ -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<int64_t>(sizeof(FragOffset)) * alt->m_frag_offset_count;
int64_t extra64 = frag_table_size - static_cast<int64_t>(sizeof(alt->m_integral_frag_offsets));
intptr_t frag_offset = reinterpret_cast<intptr_t>(alt->m_frag_offsets);

if (frag_offset < 0 || len < extra64 || static_cast<int64_t>(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<intptr_t>(alt->m_frag_offsets);
int extra = static_cast<int>(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
Expand Down
95 changes: 95 additions & 0 deletions src/proxy/hdrs/unit_tests/test_Hdrs.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]")
Expand Down Expand Up @@ -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<char>
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<char> buf(sizeof(HTTPCacheAlt) + 4096, 0);
auto *alt = reinterpret_cast<HTTPCacheAlt *>(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<intptr_t *>(&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<int>(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<int>(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<intptr_t>::max() - 1);
int len = static_cast<int>(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<int>(buf.size());
auto buf2 = make_marshalled_alt(5, static_cast<intptr_t>(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<int>(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<int>(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<intptr_t>::max() - 1);
int len = static_cast<int>(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<int>(buf.size());
auto buf2 = make_marshalled_alt(5, static_cast<intptr_t>(buf_len + 1));
CHECK(HTTPInfo::unmarshal_v24_1(buf2.data(), buf_len, nullptr) == -1);
}
}