Fix regex remap on older PCRE2 - #3
Open
bneradt wants to merge 129 commits into
Open
Conversation
strncasecmp was only comparing up to the length of one string, so names of different lengths could incorrectly match. * Fix typo
RFC 9113 Section 8.2.2 lists Transfer-Encoding as a connection-specific header that must be rejected as malformed in HTTP/2 messages. The HeaderValidator already rejected Connection, Keep-Alive, Proxy-Connection, and Upgrade but was missing Transfer-Encoding.
RFC 9112 requires chunked to be the last transfer coding. Requests that violate this are now rejected with 400, and server responses with 502. Also updates check_hdr_implements to use CSV iteration for multi-value Transfer-Encoding headers.
Bounds-check the backward pointer walk in the obs-fold unfold code and use the accumulated line buffer when the preceding bytes are not in the current input fragment.
The existing proxy.config.http.header_field_max_size setting limits individual header field sizes in the HTTP/1.1 parser, but the HPACK and QPACK decode paths had no equivalent check. This adds a max_string_len parameter to xpack_decode_string and passes the configured limit down through the HTTP/2 and HTTP/3 decode chains. Define 32768 as a constant
HPACK indexes decoded into 64-bit values could be silently narrowed when looking up entries in the indexing table. A peer could encode a value above the 32-bit range that truncated to a different valid static table entry. This rejects oversized decoded indexes before any table lookup and keeps the narrowing explicit after validation. This also adds regression coverage for indexed and literal indexed-name representations. Co-authored-by: bneradt <bneradt@yahooinc.com>
Replace hand-rolled digit accumulation loops in mime_parse_int, mime_parse_uint, and mime_parse_int64 with std::from_chars to eliminate undefined behavior from signed integer overflow. Apply RFC-specific overflow policy per header type: Content-Length: reject as parse error (RFC 9112 §6.3) Age: clamp to 2^31 (RFC 9111 §1.2.2) Max-Forwards: clamp to INT32_MAX (RFC 9110 §7.6.2)
Co-authored-by: Serris Santos <lserris@apache.org>
Co-authored-by: Serris Santos <lserris@apache.org>
Co-authored-by: Serris Santos <lserris@apache.org>
When deflate() returns a non-Z_OK status mid-stream, the gzip transform logged a warning and continued the encode loop instead of aborting. Return on error to align with the brotli and zstd transforms, which already exit cleanly on compress-operation failure. Also escalate the log line from warning() (debug-only via Dbg) to error() so operators see the abort without enabling the compress debug tag.
Add config validation to cap round_robin_max_count to [1-1024] and a release assert in HostDBInfo::assign to catch srv_offset values that would silently truncate the 16-bit bitfield. The existing code comment documented the invariant but did not enforce it.
Replace two VLAs sized by DNS response counts with ts::LocalBuffer. The SRV pointer array in dnsEvent and the live-targets array in select_best_srv used stack VLAs guarded only by ink_assert (compiled out in release). Use LocalBuffer with a stack size of 16 matching the default hostdb_round_robin_max_count, with automatic heap fallback for larger configurations.
Bounds-check type parameter in get_received_frame_count() for both Http2ClientSession and Http2ServerSession Clamp out-of-range values to HTTP2_FRAME_TYPE_MAX (the "unknown" bucket), matching the existing pattern in _count_received_frames on the write side
Add cp + RRFIXEDSZ > eom bounds check before NS_GET16/NS_GET32 reads of the fixed RR header fields (type, class, ttl, rdlength) Add dn_skipname error check and cp + SRV_FIXEDSZ > eom bounds check in the SRV record parsing path A truncated DNS UDP response could satisfy the cp < eom loop condition while multi-byte macro reads extend past the buffer end
The remap plugin path in Instance::_initialize iterates argv into a fixed-size std::array<DataType, 16> without bounds checking. When a remap.config rule supplies more than 16 parameters, the write overflows the array. Add a bounds check matching the one already present in the global plugin path. Mitigating factor: parameters come from admin-controlled remap.config, not from attacker input.
Canonicalize the rebased path and verify it remains under the config directory. Previously, ../ sequences in the path could escape the config directory after concatenation.
Cap status code accumulator in http_parse_status to 999 to prevent signed integer overflow and truncation from a malicious origin response. Co-authored-by: Serris Santos <lserris@apache.org>
Client-controlled URL fields can make the WIPE_FIELD_VALUE log filter build a large temporary buffer while masking query parameter values. Keeping that buffer on the thread stack risks exhausting the stack and crashing traffic_server when the filter is configured for matching URL fields. This makes use of ts::LocalBuffer which uses stack space in the common case (header is under 8kb), but allocates memory if it is more than that. This keeps large marshalled URLs from consuming excessive thread stack space during WIPE_FIELD_VALUE processing.
PROXY protocol v1 port fields arrive as text, and values outside the TCP port range could be narrowed into in_port_t before validation. Invalid input could therefore be treated as a different valid port in routing, ACL, and logging decisions. This validates the full decimal token in the parser before narrowing it to the port type. This also adds boundary coverage for accepted maximum ports and rejected zero, overflow, and truncating values.
Large tuple features and Host field rewrites can flow through txn_box from request-controlled input. Several paths used alloca with sizes derived from those values, which could consume a large fraction of the ATS thread stack or overflow it outright. This replaces those temporary alloca buffers with LocalBuffer so the common small cases stay local while larger request-controlled buffers move off the stack. The Host rewrite buffers keep the same ATS host-size local storage, with LocalBuffer providing a heap fallback when the formatted field is larger. This also corrects the Host port buffer sizing to use the integer digit capacity for in_port_t. max_digits10 is a floating-point round-trip trait and is zero for this integer type, so using digits10 + 1 reserves space for the largest decimal port value.
IPv6 address text with an oversized or signed hextet could be accepted by libswoc and narrowed into the 16-bit IPv6 quad storage. That allowed input such as 10000::1 to be interpreted as a different address after truncation. This parses IPv6 hextets as unsigned values and rejects any token that does not fit in one quad. This also adds coverage for the maximum valid hextet, oversized hextets, bracketed input, and signed tokens.
…x UAF The recursive corrupted-doc and collision retry path can free the CacheVC inside the nested handleEvent, after which the per-CacheVC recursion counter was decremented on the freed object. Move the counter to a file-scoped thread_local so it outlives the freed CacheVC; recursion is synchronous on a single thread, so the depth limit still bounds it identically. Drop the unused read_recursive member and add a gold autest behind a test-only build hook.
Co-authored-by: Serris Santos <lserris@apache.org>
Large or malformed URI inputs can exhaust or overflow uri_signing's variable-length stack buffers, and several helpers mixed string length with buffer capacity. That made normalization and token stripping depend on unchecked C-string assumptions around NUL terminators. This moves temporary normalization buffers and test helpers to ts::LocalBuffer, validates pointer and count inputs before using them, and applies NUL-inclusive buffer sizes consistently. This also expands dot-segment coverage for large and degenerate paths.
CONNECT destinations to loopback or private services need an explicit destination policy in addition to port allowlisting to protect them. Operators already have that control through outbound ip_allow rules, but the default files and coverage did not fully show how to protect local resources, alternate unspecified or IPv4-mapped address forms, or how to opt in to intentional local tunnels. This is a default configuration, documentation, and test change only; it does not add new core CONNECT validation code. It adds CONNECT-only outbound denies for unspecified, loopback, private, link-local, and IPv4-mapped IPv6 destination ranges, while keeping intentional deployments overrideable with earlier outbound allow rules.
When C= has no trailing delimiter, strstr() returns nullptr and the (pp - cp) arithmetic was undefined behavior. Add a nullptr check in both the AF_INET and AF_INET6 branches, plus autest cases.
A pre-session inactivity/handshake timer left armed across the TLS handoff could fire while a deferred TS_HTTP_SSN_START_HOOK callout is pending, delivering VC_EVENT_INACTIVITY_TIMEOUT to a session acceptor and release-asserting traffic_server. Cancel the timer before handing off the read VIO and re-arm accept_no_activity_timeout only after the start hooks return. Adds a regression autest.
regex_map compiled its host pattern unanchored, so a rule for "cdn.example.com" also matched "prefix.cdn.example.com" and "cdn.example.com.test". Compile with RE_ANCHORED | RE_ENDANCHORED so the pattern requires a whole-host match; it now matches only the exact host.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The global remap table's load and acquire ran as two unsynchronized steps, while config reload swapped the table and immediately released the old one without a mutex. A reader preempted between load and acquire could revive a table whose refcount the reload had just driven to zero, after the deleter was already scheduled. Retire the bespoke acquire/release refcount on UrlRewrite and let std::atomic<std::shared_ptr<UrlRewrite>> (via the new AtomicSharedPtr helper) own the publish-and-replace. Each transaction snapshots the current table into HttpSM::m_remap on session start; reload exchange()s in a new shared_ptr and drops its ref, so the old table destructs only after the last in-flight HttpSM releases its snapshot. Add shutdown_url_rewrite() to drain and inhibit further drops so plugin doneInstance() runs while this_ethread() is still valid. Co-authored-by: Masaori Koshiba <masaori@apache.org>
Late shutdown can drop the global remap table before all net-thread work
has stopped accepting or initializing transactions. A transaction created in
that window can reach remap with an empty table lease and crash while
dereferencing it.
Addresses the following crash:
```
(gdb) bt
#0 RemapProcessor::setup_for_remap (this=<optimized out>, s=0x7f84ff3a2100, table=0x0) at /src/proxy/http/remap/RemapProcessor.cc:46
Backtrace stopped: Cannot access memory at address 0x7f9ba11f8418
(gdb) l
41 RemapProcessor::setup_for_remap(HttpTransact::State *s, UrlRewrite *table)
42 {
43 Dbg(dbg_ctl_url_rewrite, "setting up for remap: %p", s);
44 URL *request_url = nullptr;
45 bool mapping_found = false;
46 HTTPHdr *request_header = &s->hdr_info.client_request;
47 char **redirect_url = &s->remap_redirect;
48 const char *request_host;
49 int request_host_len;
50 int request_port;
(gdb)
```
This keeps the shutdown-window null table as a quiet defensive remap miss.
The guard runs before setup or finish dereferences the table, leaves
in-flight transactions that already hold a remap lease untouched, and avoids
warning or metric churn for a condition expected only while the process exits.
This intentionally leaves shutdown admission ordering unchanged. A broader
admission-gate fix can be evaluated separately from this cheap consumer
backstop.
ChunkedHandler::read_trailer() ended the trailer section on a bare LF blank line regardless of proxy.config.http.strict_chunk_parsing, while read_size() already required a full CRLF. Per RFC 9112 Section 7.1 the terminating empty line must be CRLF, so a bare LF let two parsers disagree on where the message ends. Reject it under strict parsing, mirroring read_size(); non-strict is unchanged. Adds unit and gold coverage.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The cherry-picked combo_handler changes from master brought in gold-file output paths assuming master's TestRun layout, which contains an additional colon-in-path test (apache PR apache#13143) that does not exist on 10.1.x. With one fewer TestRun preceding the empty-content-type, cache-control aggregation, and max-age-zero checks, their output paths are one index too high on this branch. Realign 4-tr -> 3-tr, 5-tr -> 4-tr, 6-tr -> 5-tr so each gold-file path matches the TestRun that produces it.
Responses answered internally without a server transaction did not all consume an accompanying request body. A DELETE with Max-Forwards: 0 and a PURGE take the cache-delete path, leaving body bytes to be parsed as the next request on a keep-alive connection. This drains the request body at the common internal-response path, closes the connection when the body cannot be safely consumed, and adds gold tests for the DELETE desync, cache-miss drain, and keep-alive behavior.
…railers=0) When trailers are retained, a chunked->chunked passthrough has the user-agent consumer read p->read_buffer directly while the parser's chunked_reader walks that same buffer. The chunked high-water throttle returned before walking the parser, so chunked_reader stayed un-advanced and pinned read_buffer above the high water mark; the re-enable check (also high-water based) never cleared and the transfer deadlocked until the inactivity timeout. Walk the parser before applying the throttle on the passthrough path so chunked_reader stays drained and the high-water check reflects only the consumer's backlog. The throttle still runs afterwards, so the cache-write variant (passthrough + dechunk-to-cache) keeps bounding its separate dechunked_buffer, preserving the CVE-2021-32564 protection. The buffered chunk/dechunk paths are unchanged. Add a Proxy Verifier regression test covering the passthrough flow-control path.
CentOS 7 provides PCRE2 10.23, which predates PCRE2_ENDANCHORED and cannot compile the 10.1.x security rollup. A post-match length check would also lose the regex engine's full-match backtracking semantics. Keep native end anchoring when PCRE2 supports it, and fall back to a grouped absolute-end assertion only on older releases. This preserves alternatives and capture numbering while keeping the newer JIT path allocation-free.
bneradt
force-pushed
the
fix-pcre2-10-23-regex-remap
branch
from
July 27, 2026 20:32
6501b03 to
886328a
Compare
cmcfarlen
force-pushed
the
sec-2026-2-10.1.x
branch
from
July 27, 2026 23:52
7b3bf9c to
a74a0fb
Compare
cmcfarlen
pushed a commit
that referenced
this pull request
Jul 29, 2026
Redirected transactions can retain CACHE_WL_SUCCESS after the concrete cache write VC has been cleared. When a later response reaches cache write setup, we crash dereferencing the missing write VC. This only reuses a redirected cache write when the prepared write VC is still available. Otherwise, this resets the write state so ATS prepares a fresh cache write. The added test simply adds some coverage around the scenario but is not a true regression test. That is, the test still passes without the src/ code change. This addresses a crash in the cache write setup path: ``` #0 HttpSM::setup_cache_write_transfer(...) #1 HttpSM::perform_cache_write_action() #2 HttpSM::handle_api_return() #3 HttpSM::state_api_callout() apache#4 HttpSM::state_api_callback() apache#5 TSHttpTxnReenable() apache#6 EscalateResponse() ```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The 10.1.x security rollup uses PCRE2_ENDANCHORED for regex
remap, but CentOS 7 provides PCRE2 10.23, which predates that
flag and fails to compile the release branch.
Keep the native end-anchor flag when PCRE2 supports it and
emulate it with a grouped absolute-end assertion only on older
releases. This preserves full-host backtracking and capture
numbering while keeping the newer JIT path allocation-free.