This is an automated email from the ASF dual-hosted git repository.
traeak pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new cee0b3e52d Validate m_frag_offset_count and offset during cache
unmarshal (#13165)
cee0b3e52d is described below
commit cee0b3e52d4932072545cf9d2575af6c4f5b03ae
Author: Brian Olsen <[email protected]>
AuthorDate: Wed May 20 06:33:54 2026 -0600
Validate m_frag_offset_count and offset during cache unmarshal (#13165)
* Validate m_frag_offset_count and offset during cache unmarshal
* HTTP.cc: remove unecessary <0 check, add unit tests
* change cache corruption ink_asserts into run time Warnings
* for test warning helper add indicator that variable may be unused
---
src/proxy/hdrs/HTTP.cc | 27 ++++++++--
src/proxy/hdrs/unit_tests/test_Hdrs.cc | 95 ++++++++++++++++++++++++++++++++++
2 files changed, 117 insertions(+), 5 deletions(-)
diff --git a/src/proxy/hdrs/HTTP.cc b/src/proxy/hdrs/HTTP.cc
index b67534d6de..f7f80d7117 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<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 {
@@ -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
diff --git a/src/proxy/hdrs/unit_tests/test_Hdrs.cc
b/src/proxy/hdrs/unit_tests/test_Hdrs.cc
index 4e90846edb..093e0fb7b3 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<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);
+ }
+}
