Copilot commented on code in PR #13165:
URL: https://github.com/apache/trafficserver/pull/13165#discussion_r3243585419
##########
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 (alt->m_frag_offset_count < 0 || len < frag_table_size || frag_offset <
0 || orig_len < frag_offset + frag_table_size) {
Review Comment:
If `m_frag_offset_count` is an unsigned type (e.g. `uint32_t`), the
`alt->m_frag_offset_count < 0` check is a tautology and will be optimized out
(and likely produces a `-Wtype-limits` warning). Either drop that sub-condition
or cap the count against a sane maximum (for example, `frag_table_size`
overflowing `int64_t` is impossible, but `m_frag_offset_count > orig_len /
sizeof(FragOffset)` is the meaningful upper bound). The same applies to the
corresponding check on line 2294 in `unmarshal_v24_1`.
##########
src/proxy/hdrs/HTTP.cc:
##########
@@ -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 (alt->m_frag_offset_count < 0 || len < extra64 || frag_offset < 0 ||
orig_len < frag_offset + extra64) {
Review Comment:
In the v24_1 path, the data located at `buf + frag_offset` is the entire
fragment-offset table (size `frag_table_size`), not just the `extra64` bytes
beyond the integral slots — the integral portion is copied separately below.
Using `extra64` for the `orig_len < frag_offset + extra64` bound check
therefore under-validates and can still allow a read past the end of the buffer
by `sizeof(alt->m_integral_frag_offsets)` bytes. Consider validating against
`frag_table_size` for the buffer-range check while keeping `extra64` for the
`len` adjustment, or confirm via the marshal code that only `extra64` bytes are
actually stored at `frag_offset`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
