Joshua Rogers [Sun, 9 Nov 2025 11:49:11 +0000 (11:49 +0000)]
snmplib: Improve handling of zero-length ASN OCTET STRINGs (#2163)
When converting zero-length objects to c-strings and terminating their
values, do not write to c-string storage buffer at offset `-1`.
Also reject 128-byte SNMP community values because our storage buffer
does not allow for their zero-termination. Such values were probably
silently truncated to 127 bytes.
Also reject SNMP community values with ASCII NUL characters because some
of our code does not support such values.
A Comm::ReadNow() caller or similar high-level reading code that
triggers tls_read_method() calls is often unaware that it is doing TLS.
Comm::ReadNow() and similar TCP I/O APIs return decoded bytes or I/O
errors. This lack of awareness keeps high-level reading code simple, but
it becomes a problem in triage because neither low-level BIO code nor
high-level ReadNow() callers report TLS state/errors. They report
errno-based outcomes that may be faked by translation layers and may not
fully describe what happened at TLS layer.
Translations are highly environment/configuration/transaction specific,
but here are a few examples:
These changes expose TLS state/errors hidden by translation (e.g.,
SSL_ERROR_WANT_READ, SSL_ERROR_ZERO_RETURN, SSL_ERROR_SYSCALL above).
Similar changes were applied to tls_write_method(), for similar reasons.
Debugging and error reporting improvements aside, no functionality
changes are expected. These enhancements were instrumental in tunnel
closure problems triage, but those problems deserve dedicated fixes.
Alex Rousskov [Sun, 2 Nov 2025 13:48:19 +0000 (13:48 +0000)]
Fix and improve debugging of tunneled TCP connections (#2291)
When a to-client write failed, TunnelStateData::writeClientDone()
debugging said "from-client read failed".
TunnelStateData::keepGoingAfterRead() debugging did not differentiate
two key cases: zero-size read and Squid-initiated closure of the other
end of the tunnel. That "other end" was called "client", but it could be
the "server" end. Related `if` statements logic was difficult to follow.
TunnelStateData::Connection state was difficult to reconstruct when
debugging long-lived tunnels. These debugging enhancements were
instrumental in tunnel closure problems triage, but those problems
deserve dedicated fixes.
Debugging improvements aside, no functionality changes are expected.
`ClientRequestContext::httpStateIsValid()` always returns true. We
can prove that by contradiction:
* `ClientRequestContext::httpStateIsValid()` return value is
`cbdataReferenceValid(http)` return value.
* `ClientRequestContext::http` points to a `ClientHttpRequest` object.
* If `cbdataReferenceValid(http)` is false, then the corresponding
`ClientHttpRequest` object pointed to by `http` has been destructed.
* `ClientHttpRequest` destructor deletes its `calloutContext` data
member (i.e. `ClientRequestContext`).
Thus, if an `x->httpStateIsValid()` call returns false, then `x` object
has already been deleted, and the call itself was buggy because it was
dereferencing a pointer to a deleted object!
Moreover, `cbdataReferenceValid(nullptr)` is always true, so calling
`httpStateIsValid()` twice for a deleted `ClientHttpRequest` object does
not actually protect the second caller (i.e. does not detect that `http`
is gone) because the first call makes `ClientRequestContext::http` nil,
making `httpStateIsValid()` true again!
None of the above problems actually happen because
`cbdataReferenceValid(http)` is never false inside
`ClientRequestContext::httpStateIsValid()`. By the time it could have
become false, `ClientRequestContext` object itself is already gone.
Also removed a source code comment that falsely claimed that
`ClientRequestContext` callback methods should check whether
`ClientHttpRequest` object is still "valid". In reality,
`ClientRequestContext` methods are not called after `ClientHttpRequest`
object is destructed because they are protected by cbdata guards, and
`ClientHttpRequest` destruction deletes `ClientRequestContext` object,
resulting in those guards skipping any callbacks.
The same comment also incorrectly implied that `ClientHttpRequest` first
becomes "invalid" and then, _after_ the current callback is done,
`ClientHttpRequest` destructor is called. In reality,
`cbdataReferenceValid(x)` becomes false when `delete x` is called and,
hence, when `x` destructor runs.
Alex Rousskov [Sat, 25 Oct 2025 10:07:58 +0000 (10:07 +0000)]
Maintenance: Drop HttpStateData::handleMoreRequestBodyAvailable (#2286)
The method is unreachable: It was supposed to be a virtual method. It is
declared as such in HttpStateData class, but it should have been
declared virtual in Client (i.e. the _parent_ class). Without that
virtual declaration in Client, the caller calls a Client method with the
same name instead. This code became unreachable in 2007 commit 5f8252d2.
The current method implementation is unusable: There are two explicit
XXXs, excessive (and unclassified) level-1 debugging, and an infinite
recursion added in the same 2007 commit 5f8252d2.
It is possible to refactor this method implementation to address known
concerns. We decided to remove this method instead because proper
refactoring is costly and is likely to result in this method removal.
The infinite recursion concern was raised by Joshua Rogers
https://joshua.hu/
Joshua Rogers [Thu, 23 Oct 2025 10:11:27 +0000 (10:11 +0000)]
ICMP: Harden echo paths, fix overflows, UB, and leaks (#2199)
ICMPv4 send now validates getAddrInfo results before touching
ai_addr, avoiding null-deref, and frees the address on error.
Logging no longer passes nullptr strings, preventing UB.
ICMPv4 recv gained strict bounds checks: reject packets shorter
than IP/ICMP headers, reject bogus iphdrlen, and require enough
bytes for echo meta. Payload length is computed from real data,
clamped to MAX_PAYLOAD, and malformed negatives are dropped. The
result size calculation now uses PINGER_PAYLOAD_SZ instead of the
wrong MAX_PKT4_SZ, fixing excess memory disclosure.
ICMPv6 send added the same getAddrInfo validation and safe log
string handling. The recv path now checks for minimal header
lengths, validates echo meta, fixes ident comparison with ntohs,
and clamps payload length safely before copying into preply.
The pinger handshake no longer uses strlen() on raw buffers;
it now echoes exactly the bytes received, avoiding OOB reads.
Squid Recv was changed to treat pingerReplyData as variable-
length. It validates base header size, available payload, and
non-negative psize, and rejects truncated or oversized datagrams,
fixing past OOB read risks.
Finally, netdbBinaryExchange now flushes its 4K buffer before
writing the next record, fixing a possible overflow at the end of
the buffer.
Alex Rousskov [Thu, 23 Oct 2025 08:05:28 +0000 (08:05 +0000)]
Bug 5520: ERR_INVALID_URL for CONNECT host with leading digit (#2283)
Squid 7.2 commit b8337359 added validation of host names
following RFC 1035 requirements. But those requirements were
outdated by RFC 1123:
One aspect of host name syntax is hereby changed: the
restriction on the first character is relaxed to allow either a
letter or a digit. Host software MUST support this more liberal
syntax.
The commit treated CONNECT host names that start with a decimal digit
as invalid IPv4 addresses and rejected the corresponding requests,
resulting in HTTP 404 errors. Undo that change.
We have considered preserving code that detects valid IPv4 addresses (as
opposed to treating all non-IPv6 input as an "IPv4 address or reg-name"
without disambiguating the two cases) because its pieces may be reused,
but that essentially unused code has non-trivial performance penalty and
final code may look quite different after we complete our "non-CONNECT
uri-host parsing code" migration TODO. Polished source code comments
aside, this change reverts 2025 commit b8337359 and restores 2023
AnyP::Uri::parseHost() implementation (commit 963ff143).
The bug affects url_regex and urllogin ACLs. However, not every use of
those ACLs results in a FATAL exit. The exact preconditions are unknown.
Three pct-encoding (RFC 3986) error handling algorithms were considered:
### Algorithm A: An ACL that cannot decode, mismatches
This algorithm is similar to the algorithm used for handling "ACL is
used in context without ALE" and similar errors, but there is a
significant context difference: Those "without ALE" errors are Squid
misconfigurations or bugs! Decoding failures, on the other hand, are
caused by request properties outside of admin or Squid control.
With this algorithm, a request can easily avoid a "deny urlHasX" rule
match by injecting an invalid pct-encoding (e.g., `X%bad`). Such
injections may not be practical for URLs of most resources outside of
client control because most servers are unlikely to recognize the
malformed URL as something useful for the client. As for resources that
client does control, a urlHasX ACL cannot be effective for those anyway
because the client can change URLs.
Algorithm A does not let Squid admins match problematic URLs!
### Algorithm B: An ACL that cannot decode X, tests raw/encoded X
With this algorithm, a request can trigger some "allow urlHasY" rule
matches by injecting an invalid pct-encoding that looks like Y (e.g., if
an "allow" rule looks for the word `good`, a request may contain a
`%good` or `%XXgood` sequence). Just like with algorithm A, such
injections probably have little practical value, for similar reasons.
Algorithm B lets Squid admins match problematic URLs.
### Algorithm C: An ACL that cannot decode X, tests partially decoded X
With this algorithm, a "partially decoded X" is X where invalid
pct-encoding sequences (or their parts) are left "as is" while valid
pct-encoding triplets are decoded. This is actually a family of similar
algorithms because there are multiple ways to define invalid
pct-encoding sequence boundaries in certain URLs! For example,
`%6Fne%f%6Fo` can be replaced with `one%foo` or `one%f%6Fo`. This
additional complexity/uncertainty aggravates the two concerns below.
Algorithm B notes apply to algorithm C as well.
Algorithm C lets admins match problematic URLs but, again, it requires
that admins know exactly how Squid is going to isolate problematic
pct-encoding triplets (e.g., skip/leave just `%` byte that starts an
invalid pct-encoding sequence or the following two bytes as well).
Algorithm C family includes rfc1738_unescape() behavior. That decoding
function was used for the two ACLs before commit cbb9bf12 and commit 226394f2 started to use AnyP::Uri::Decode() added in commit 26256f28.
For example, rfc1738_unescape() decodes `%%` as `%` and leaves some
other invalid pct-encoding one-, two-, and three-byte sequences in the
decoded result. It is unlikely that many admins know exactly what that
old decoding does, but they could tune their rules to "work" as they
expect for specific cases. Those rules could stop working after the
above commits (v7.0.1+) and this change, to their surprise.
This change implements Algorithm B:
* Unlike Algorithm A, B allows admins to match bad URLs.
* Unlike Algorithm C, B does not force admins to guess how Squid
mangles a bad URL before matching it.
Also updated ACLs documentation to reflect current implementation.
Joshua Rogers [Sun, 19 Oct 2025 17:33:18 +0000 (17:33 +0000)]
Fix libntlmauth string parsing on big-endian machines (#2242)
Prevent off-by-one reads in ntlm_fetch_string(); clamp copies too.
Convert flags with le32toh; validate lengths; ensure NUL-terminate.
cast ntlm string characters to unsigned char explicitly.
Return BlobError on oversized fields to avoid UB.
Combined these changes affect Big-Endian CPU architectures
which will fail to detect ASCII vs UTF encoding properly and
produce invalid strings (including user/password to compare).
Joshua Rogers [Sat, 18 Oct 2025 15:15:07 +0000 (15:15 +0000)]
SNMP: Fix OID truncation and sibling lookup (#2244)
Previously, SNMP OIDs could be truncated (peer_Inst didn't update
the length after appending a subid), snmpTreeSiblingEntry() misused
len and sometimes returned the wrong sibling or stepped past the
end.
Now we update the OID length while building, select siblings
by matching name[len] (returning the right neighbor only if present).
Amos Jeffries [Sat, 11 Oct 2025 03:33:02 +0000 (16:33 +1300)]
Bug 3390: Proxy auth data visible to scripts (#2249)
Original changes to redact credentials from error page %R code
expansion output was incomplete. It missed the parse failure
case where ErrorState::request_hdrs raw buffer contained
sensitive information.
Also missed was the %W case where full request message headers
were generated in a mailto link. This case is especially
problematic as it may be delivered over insecure SMTP even if
the error was secured with HTTPS.
After this change:
* The HttpRequest message packing code for error pages is de-duplicated
and elides authentication headers for both %R and %W code outputs.
* The %R code output includes the CRLF request message terminator.
* The email_err_data directive causing advanced details to be added to
%W mailto links is disabled by default.
Also redact credentials from generated TRACE responses.
---------
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Avoid putenv() problems by switching to setenv() (#2262)
Developers work around putenv() design flaws by writing more complex
code that still leaks memory (e.g., commit b1b2793 and commit 96b9d96).
We should follow putenv(3) manual page advice and use setenv() instead:
The setenv() function is strongly preferred to putenv().
Kerberos builds have been using setenv() since 2009 commit 9ca29d2.
Twenty years ago, setenv() code failed to build on Solaris (see 2005
commit cff61cb), but modern Solaris does have setenv(3).
Since setenv(3) is a standard C library _extension_ unavailable on
Windows, we provide a setenv(3) replacement for MS Windows builds. Our
replacement should work correctly in known current use cases, but acts
differently if given a variable with an empty value. This replacement
does not make things worse because the old macro trick had the same
flaw. We do not know of an easy way to support empty variable values on
Windows the way setenv(3) does.
Also fixed negotiate_kerberos_auth undefined behavior caused by freeing
memory returned by getenv() when the helper was run without `-k keytab`.
Lior Brown [Fri, 3 Oct 2025 08:18:34 +0000 (08:18 +0000)]
Bug 5510: False Cache Digests misses (#2254)
Since 2002 commit add2192d, code receiving fresh Cache Digests from
cache_peers corrupted peer digest bitmask, leading to misses for objects
that were supposed to be present in the digest[^1]. Memory overreads
were probably happening as well. The exact corruption conditions/effects
probably changed when 2023 commit 122a6e3c removed HTTP response headers
from storeClientCopy() API, but the underlying memmove() size
calculation bug predates that 2023 change.
[^1]: Bitmask corruption also ought to trigger some hits for objects
that were not present in peer's cache, although such hits were not
observed in triage, and some excessive hits are endemic to our Bloom
filters.
Flip the default of configure's `--enable-arch-native` option from
enabled to disabled.
GCC's and clang's `-march=native` argument causes issues in some
environments, most notably containers where the compilers' heuristics
about CPU feature set may fail. These issues manifest as
hard-to-reproduce SIGILL errors in binaries such as `squid` or unit test
programs. The new default is safer. Performance-minded administrators
still have a convenient option to optimize via `--enable-arch-native`.
Support no-digest X509 certificate keys like ML-DSA/EdDSA (#2165)
Recent OpenSSL releases (e.g., OpenSSL v3.5) support several private key
types[^1] for which supplying a message digest algorithm is prohibited
when signing a certificate. Prior to this enhancement, Squid was
rejecting https_port and http_port configurations using such key types
(with the above FATAL message) because OpenSSL X509_sign() call made
with a prohibited (for the given key type) non-nil digest algorithm was
failing.
Technically, only listening ports with generate-host-certificates (and
ssl-bump) parameters need to generate X509 certificates and, hence, call
X509_sign(). However, current Squid code generates so called "untrusted"
certificates even for ports that do not support dynamic host certificate
generation or SslBump (XXX). Thus, this enhancement is applicable to
both regular and SslBump configurations.
[^1]: Known no-message-digest key types are ML-DSA-44, ML-DSA-65,
ML-DSA-87, ED25519, and ED448, but others might exist or will be added.
This change was tested against known types, but should support others.
ML-DSA key types are used in post-quantum cryptography.
SNMP: Match Var allocation/deallocation methods (#2183)
Pdu::setVars() and Pdu::unpack() allocate variables with `new Var(...)`,
but clearVars() freed them using snmp_var_free(). That skipped the `Var`
destructor and mismatched the allocator.
We hope that all Pdu::variables are allocated via Pdu class methods
despite the presence of snmp_var_new() and snmp_var_clone() calls in
low-level snmplib code.
Fix parsing of malformed quoted squid.conf strings (#2239)
Iteration of a quoted token that ends with a backslash (escape with no
next char) kept going past the end of the token. That bug as well as
hypothetical 2KB-byte tokens (exceeding CONFIG_LINE_LIMIT) could also
result in a 1-byte NUL overrun.
ext_ldap_group_acl: Require base dn and user search filter (#2167)
Make ext_ldap_group_acl require -B (user base DN) and -F (user search
filter) so the helper finds the user before checking group membership.
Fix LDAP referrals handling and make %v always expand to the requested
group name. Improve logging and errors, and include release notes for
the breaking flag requirements.
HTCP: Check for too-small packed and too-large unpacked fields (#2164)
Harden HTCP parsing by checking HTCP fields
- Check packed field lengths and buffer space before reads.
- Guard CLR "reason" when sz < 2; log invalid messages.
- Support old minor==0 layout with safe prefix copy.
- Use early returns and unique_ptr for safer flows.
Fix off-by-one in helper args count assertion (#2212)
The `nargs` value should now be pointing past both the
`HELPER_MAX_ARGS` and the additional terminator. i.e. outside
the valid array space. This is okay because it is an absolute
counter (1-based) not an offset (0-based) despite how it is
used to fill the array.
Previously, nmasldap_get_simple_pwd() and nmasldap_get_password()
could overrun or return non-terminated strings at length
boundaries. This change adds strict bounds checks, copies at most
len - 1, and ensures explicit NUL termination, aligning both
helpers buffer/length semantics without altering call-site
behavior.
As of 2025 Microsoft has officially removed NTLM support from
Windows machines entirely. The SSPI mechanisms will no
longer provide NTLM authentication for this helper to use.
Fix UDP log module opening and closing code (#2214)
logfile_mod_udp_open() mistreated successful comm_connect_addr() result
as an "Unable to connect" failure (and vice versa), rendering UDP-based
logging unusable. Broken since at least 2010 commit d938215.
Also fixed logfile_mod_udp_close() closing FD 0 after "Invalid UDP
logging address" ERRORs during logfile_mod_udp_open().
ext_file_userip_acl: harden lookups and memory handling (#2198)
Stop mutating getgrnam(3) buffer; iterate gr_mem safely
Zero last node and NULL-check in dict_lookup() to prevent OOB read
Add free_dict() and free dictionary before exit
negotiate_kerberos_auth: Properly align NDR data (#2186)
Resolves sporadic Negotiate/Kerberos auth failures that manifested
as proxy 407 loops or helper errors when decoding PAC data, depending
on ticket layout.
Previously, the parser advanced bpos by the remainder:
(bpos += bpos %n)
instead of padding to the next multiple of n.
For example, n = 4:
bpos=5 (r=1): current: 6 (wrong), correct: 8
bpos=6 (r=2): current: 8 (accidentally right)
bpos=7 (r=3): current: 10 (wrong), correct: 8
This patch NULL-terminates the NMAS Universal Password values
array (values[1] = nullptr) to match ldap_get_values() semantics
and avoid potential out-of-bounds iteration.
ntlm_sspi_auth: memcmp not memcpy, send newline, no uninit mem (#2218)
Previously, memcpy was incorrectly used instead of memcmp. In addition
to this, uninitalized memory could be used, and responses to Squid were
missing a newline.
SNMP: Improve parsing of malformed ASN.1 object identifiers (#2185)
ASN.1 object identifiers are length-delimited, not null-terminated. If
the input encoding omits a terminating byte (MSB clear), then the parser
would walk past the buffer.
Also simplified expressions related to sub-identifier parsing.
Alex Rousskov [Mon, 8 Sep 2025 08:05:00 +0000 (08:05 +0000)]
SNMP: Do not send responses that we fail to encode (#2151)
When snmp_build() fails, its output buffer must not be used:
Syscall param sendto() points to uninitialised byte(s)
by xsendto() (socket.h:118)
by comm_udp_sendto() (comm.cc:923)
by snmpConstructReponse() (snmp_core.cc:449)
Also inform the admin about SNMP encoding failures.
On errors, the helper was reporting DsRoleGetPrimaryDomainInformation()
error info using the wrong/bogus error code and probably freeing an
uninitialized pointer.
FTP: Avoid null dereferences when handling ftp_port traffic (#2172)
`strchr` may return null if a deliminator is not found. Likewise,
if an `Http::HdrType::FTP_REASON` string is not found, nullptr would
be used in the %s formatter, leading to UB.
This drastic reduction in testHttpRange dependencies is made possible by
splitting HeaderTools code into several parts:
* Header-mangling code that pulled in many heavy dependencies but was
unused by testHttpRange code. This high-level code does not belong to
libhttp, so it was left in src/ (see HeaderMangling.cc).
* Simple header manipulation functions without heavy dependencies; some
used by testHttpRange code. This low-level code should eventually be
moved to libhttp, but it was left in HttpHeaderTools.cc for now
because libhttp itself is currently bloated with heavy dependencies --
linking with libhttp requires linking with or stubbing a lot of
code unrelated to testHttpRange.
* Definition of httpHeaderParseQuotedString() and a few other functions
declared in HttpHeader.h were moved to HttpHeader.cc to improve
declaration/definition/stubs affinity and avoid linking errors:
testHttpRange now depends on stub_HttpHeader.cc that has stubs for
these functions.
* httpHeaderMaskInit() and getStringPrefix() are only used by
HttpHeader.cc, so they were moved to HttpHeader.cc and made static.
Moved code was unchanged. A few comments were relocated to better match
Squid coding style. A few STUBs were added/adjusted as needed.
TODO: Check the remaining two tests that still include header mangling
code: testHttpRequest and testCacheManager.
Amos Jeffries [Sat, 30 Aug 2025 04:00:29 +0000 (04:00 +0000)]
Add origin-form and absolute-path to URI API (#2141)
Convert callers of Uri::path() as appropriate to their needs.
These methods provide %-encoded URI components for use in
display or normalized processing. The path() method is now only
guaranteed to provide the URL-path component in un-escaped form
(though query has not yet been moved).
Amos Jeffries [Tue, 26 Aug 2025 02:36:53 +0000 (02:36 +0000)]
Remove DEFAULT_ICP_PORT build variable (#2146)
Since 2008 commit df2eec10, this variable has only been used for
icp_port documentation and only in one location. Just stating the IANA
registered port (i.e. 3130) there is better.
Ben Kallus [Mon, 11 Aug 2025 13:02:17 +0000 (13:02 +0000)]
Fix type mismatch in new/delete of addrinfo::ai_addr (#2136)
new/delete type mismatches are UB. Fix an instance of this
problem that occurs when sockaddr_in6 is allocated, but
sockaddr is deallocated, by always allocating/deallocating
sockaddr_storage.
AddressSanitizer: new-delete-type-mismatch:
object passed to delete has wrong type:
size of the allocated type: 28 bytes;
size of the deallocated type: 16 bytes.
#0 0xaaaad1a8db54 in operator delete(void*, unsigned long)
#1 0xaaaad287a668 in Ip::Address::FreeAddr(addrinfo*&)
src/ip/Address.cc:710:22
Norman Ziert [Tue, 5 Aug 2025 08:31:40 +0000 (08:31 +0000)]
Bug 5407: Support at least 1000 groups per Kerberos user (#2047)
Increase MAX_PAC_GROUP_SIZE to a more reasonable value,
so negotiate_kerberos_auth can report more than approximately
200 groups an authenticated user is member of back to Squid.
Amos Jeffries [Mon, 4 Aug 2025 12:20:02 +0000 (12:20 +0000)]
Make spell-check.sh happy and remove stale spelling exceptions (#2122)
This change fixes a few misspellings identified by Codespell v2.4.1 and
removes all no-longer-used spelling exceptions that we could find. Most
of the fixed misspellings were present but ignored in 2020 commit 2f8abb64 because then-current Codespell v1.16.0 did not flag them.
N.B. GitHub Actions ubuntu-24.04 runner still uses Codespell v1.16.0.
projects / thirdparty / squid.git / log
