Alex Rousskov [Sun, 12 Jul 2026 21:38:04 +0000 (21:38 +0000)]
Fix support for test-suite/squidconf directive parameter files (#2458)
Those files were never found. 2022 commit d37a3744 probably missed this
bug because the corresponding `squid -k parse` failures (quoted below)
do not result in non-zero exit status code. That failure handling
problem deserves a dedicated fix.
ERROR: Can not open file empty.conf for reading
configuration context: ./squidconf/regressions-3.4.0.1.conf(27)
Use String::RawSizeMaxXXX() to limit String length (#2456)
This replacement aligns Ftp::Server with Ftp::Client code after recent
commit 0a261cbf that introduced reusable `String::RawSizeMaxXXX()`
function for limiting the length of input tokens that may be converted
to String objects. This change effectively doubles the previously
anonymous 32KB limit for received FTP command parts (assuming
`request_header_max_size` is using 64KB default or a larger value).
`String::RawSizeMaxXXX()` classifies the increased limit as safe.
Also replaced anonymous 32KB limit in HTTP `X-Forwarded-For` request
header field building code, for the same reason.
No other suitable replacement candidates are known in current code. The
following seemingly similar hard-coded limits serve other purposes; they
would be needed even after the `String` class is gone from Squid code:
* 32KB Ipc::Mem::PageSize(): Strings are not stored in shared memory.
* 64KB BodyPipe::MaxCapacity: BodyPipe uses MemBuf, not String.
* 64KB Store::SwapMetaFieldValueLengthMax: CheckSwapMetaUrl() and
similar Store swap metadata iteration code uses raw memory pointers,
not String. UnpackNewSwapMetaVaryHeaders() uses SBuf, not String.
* 64KB limit in TeChunkedParser::parse(): We use SBuf, not String for
`TeChunkedParser::mimeHeaderBlock_` storage. The hard-coded limit
prevents excessive trailer fields accumulation. Other grabMimeBlock()
callers already use admin-configurable limits that may exceed 64KB.
strtokFile() implements the "read directive parameters from a quoted
file" syntax used when configuration_includes_quoted_values is off (the
documented default). Once the named file's lines were exhausted,
strtokFile() returned nullptr, signalling end-of-directive to callers
such as ACLDomainData::parse(). Any tokens following the quoted filename
on the same configuration line (e.g., "example.com" above) were silently
discarded. A parameter file reference and trailing literal parameters
are valid syntax individually; combining them on one line has always
been allowed by the grammar, so the omission was "invisible" to admins.
This bug dates back to 2008 commit f32cd13e ("Replace cnfig parser gotos
with do-while loop."), which replaced a goto that resumed parsing after
the file's last line with an unconditional "return nullptr". Every Squid
release starting with v3.1.0.1 is affected.
FTP: Reject commands with parameters containing CR (#2453)
FTP command syntax treats CR and LF characters as command terminators.
All RFC 959 sightings of CR and LF either refer to the CRLF terminator
or treat both characters the same way. For example, RFC 959 defines an
FTP command parameter as a sequence of chars, where:
<char> ::= any of the 128 ASCII characters except <CR> and <LF>
Squid should (and now does) restrict CR use to the command terminator
sequence, especially since some FTP servers treat CRs as command
delimiters -- if we continue to allow embedded CRs in FTP command
parameters than our FtpClient::writeCommand() will assert when trying to
forward those commands to the FTP server. Moreover, we already use that
CR treatment when _parsing_ FTP responses (see
Ftp::Client::parseControlReply()).
When it comes to command termination, CRs are still optional.
A new ban on CRs in FTP command parameter values means that Squid starts
treating some FTP commands as syntactically invalid, using
EarlyErrorKind::MalformedCommand for the first time since its inception
in 2014 commit eacfca83. For example, `PWD\rQUIT` input is now invalid.
Since February 2019 commit 3dde9e52, retries of "pinned" requests[^1]
using a new Squid-to-server connections are meant for FTP and
connection-based authentication traffic only. Back then, we agreed that
such re-pinning is wrong[^2], and different mechanisms should be used to
handle forwarding errors in those cases, but we preserved the
corresponding functionality in fear of breaking "working" cases[^3]. On
that preserved code path, `request->pinnedConnection()` is true at
pinnedCanRetry() check time.
Since September 2019 commit daf80700, connectStart() throws if
`request->pinnedConnection()` is true, effectively breaking retries for
nearly all preserved cases mentioned above! The only pinned retries that
could hypothetically continue working after that commit are those where
ConnStateData job had been destroyed between pinnedCanRetry() and
connectStart(), and those rare cases are not interesting since
ConnStateData destruction ought to abort request forwarding anyway.
Both 2019 commits are present in v5.0.1 and beyond. We are not aware of
any associated problems. Thus, our fears were excessive, and we can
remove code that preserves problematic functionality. If we discover a
need to retry pinned FTP or connection-authenticated requests via a new
connection, we will add mechanisms to support that functionality.
Removing this code also helps avoid FATAL errors observed during recent
FTP work.
CONNECT tunnels
---------------
Existing tunnel.cc code ignores `request->flags.pinned` when deciding
whether to retry a failed tunneling attempt. This change does not affect
the corresponding "check pinned connections" TODO in that tunneling
code. FWIW, that TODO is limited to tunnelStart(); switchToTunnel()
cases already do not retry due to a "committed to server" retry ban on
their code path. It is likely that pinned retries should be disabled in
tunneled cases as well, but that adjustment deserves a dedicated
analysis/change.
[^1]: Here, a "pinned" request is, roughly speaking, a request sent by
Squid over a Squid-to-server connection that was kept/provided by a
client-to-Squid connection manager (ConnStateData). See usePinned().
[^2]: "automatically re-opening a PINNED connection from the middleware
is invalid in todays networks" at
https://github.com/squid-cache/squid/pull/351#discussion_r250015937
[^3]: "keep re-pinning as is" at
https://github.com/squid-cache/squid/pull/351#discussion_r253757986
Maintenance: Update OS version matrix for build tests (#2448)
Update the oldest and/or the latest versions of Fedora, Ubuntu, FreeBSD,
and OpenBSD OS matrix used for "slow" build tests. No changes to OS
versions used for Coverity Scan "quick" PR tests.
Alex Rousskov [Mon, 22 Jun 2026 20:10:29 +0000 (20:10 +0000)]
Re-fix parsing of legacy url_rewrite_program responses (#2446)
Legacy helper responses start with a URL instead of `OK rewrite_url=...`
and such. 2016 commit ddc77a2e introduced two bugs when handling legacy
responses:
* Response parsing code triggered MemBuf assertions when 0-terminating
the parsing buffer for certain URLs. The bug affected legacy helper
responses with and without space characters.
* Squid code attempted to accept/use helper-returned URLs with embedded
space character(s), despite a WARNING implying that the post-space
characters are not going to become a part of the new URL.
----
This change resurrects recent commit bb854bb9 that was accidentally
reverted by commit ec328cf16 during pull request merging by Anubis.
Do not send FTP commands with embedded CRs or LFs (#2444)
FTP command syntax prohibits bare CRs and LFs except for command
termination. FTP servers treat embedded CRs inconsistently. FTP does not
have a generally accepted escape mechanism for safely encoding those
message-framing characters. Thus, Squid must not send FTP commands with
parameters containing embedded CRs or LFs.
Squid assembles FTP commands from a variety of information sources that
depend, in part, on whether the master transaction originated on an
`http(s)_port` or `ftp_port`. One can check for (and reject) embedded
CRs and LFs in many code locations. Our attempts to reject invalid
inputs earlier, before the assembly, resulted in significant
functionality changes going beyond this fix scope but still missed an
important injection point.
The approach used here covers all known code paths, has a decent chance
of remaining effective during significant code refactoring, and does not
alter Squid functionality beyond killing transactions that resulted in
problematic FTP command parameters (an in-scope change). This approach
also allows request adaptation services to "fix" problematic requests in
some cases.
Rejected transactions usually result in HTTP 502 ERR_FTP_FAILURE
responses logged as TCP_MISS_ABORTED. More work is needed to provide
better %err_detail for these cases.
Renan Rodrigo [Thu, 18 Jun 2026 15:04:35 +0000 (15:04 +0000)]
Bug 5237: "make check" fails when ${TRUE} is a multicall binary (#2442)
testHeaders copies the 'true' binary to a file named testHeaders relying
on the fact that GNU's true always exits 0. When 'true' implementation
is a multicall binary (e.g., in rust-coreutils and on Alpine Linux), it
cannot be called by other name. Create an empty executable file instead.
squid-conf-tests also copied the 'true' binary, but that Makefile target
is never executed and, hence, should not create an executable file.
Reject excessively large FTP control replies (#2434)
When parsing FTP control replies, `Ftp::Client::parseControlReply()`
stores individual lines in the `ctrl.message` wordlist. The stored
values are later combined, appended, encoded, and/or converted to String
objects, exposing the results to `String::SizeMax_` limitations. Recent
commit 46f3f80 already ensures `reply_header_max_size` limits for
control replies. This change adds checks for cases where
`reply_header_max_size` configuration exceeds the recommended maximum
value. It also protects any sensitive worldlist-manipulating code that
might become reachable before `reply_header_max_size` limit is checked.
Excessively large FTP control replies now lead to ERR_FTP_FAILURE.
Due to ssize_t differences on 32/64 bit platforms, changes
to peerDigestSwapInMask in commit 556b91a8a7 cause
signedness comparison errors.
Refactor to be safe both on 32- and 64-bit platforms
Amos Jeffries [Sun, 31 May 2026 08:29:04 +0000 (08:29 +0000)]
HTTP/1.1: Transfer-Encoding:identity is prohibited (#2427)
HTTP specification deprecated identity encoding but still allows
its use as an unknown encoding when the Transfer-Encoding
header is otherwise correctly used.
Squid compliance update at the time kept accepting identity
encoding as it was being used by agents. The form supported was
Transfer-Encoding:identity.
However, the HTTP specification explicitly requires that any
encoding MUST be wrapped within chunked encoding.
As such the bare Transfer-Encoding:identity form Squid accepted
is explicitly prohibited.
The correct Transfer-Encoding:identity,chunked syntax has been
rejected by Squid for nearly 5 years already without complaint.
So support is being dropped entirely here.
Renaud Métrich [Wed, 20 May 2026 23:02:33 +0000 (23:02 +0000)]
Fix parsing of legacy url_rewrite_program responses (#2420)
Legacy helper responses start with a URL instead of `OK rewrite_url=...`
and such. 2016 commit ddc77a2e introduced two bugs when handling legacy
responses:
* Response parsing code triggered MemBuf assertions when 0-terminating
the parsing buffer for certain URLs. The bug affected legacy helper
responses with and without space characters.
* Squid code attempted to accept/use helper-returned URLs with embedded
space character(s), despite a WARNING implying that the post-space
characters are not going to become a part of the new URL.
Honor reply_header_max_size for received FTP control responses (#2417)
2023 commit 801593a claimed that `reply_header_max_size` applied to "FTP
command responses". However, that misleading claim probably only covered
FTP command replies that were parsed while being loaded from cache[^1],
after being converted to HttpReply objects and written to Store. The
same claim now applies to all received/raw FTP command replies as well.
[^1]: Both store_client::parseHttpHeadersFromDisk() and
MemStore::copyFromShm() called HttpReply::parseTerminatedPrefix().
When a REQMOD service satisfies a CONNECT request (e.g., by sending
Squid an HTTP error response known as a "block page"), Squid should
forward the received HTTP response to the client and then, if various
conditions allow it to do so, resume processing new requests on the
client-to-Squid connection. Instead, connection handling code got stuck
after logging a cryptic "abandoning" notice for the client connection.
Squid ignored client-initiated connection closures and did not close the
abandoned connection for `client_lifetime` that defaults to 24 hours.
Since 2011 commit f35961af, CONNECT turned flags.readMore off. That
change created several bugs because it is difficult for developers to
realize that some flag needs to be turned back on in a given context:
* 2011 commit 83d4cd15 fixed bumped TLS connection handling
* 2011 commit 6b355e5a fixed CONNECT authentication and REQMOD failures
* this commit fixes CONNECT requests satisfied by a REQMOD service
The following enhancements are outside this surgical fix scope: We may
find or add a place where all CONNECT handling is completed (without
passing control to TunnelStateData). We might start monitoring client
connection for closures and prefetch client data while handling CONNECT.
pkg: Repository FreeBSD-ports cannot be opened. pkg update required
pkg: No packages available to install matching autoconf have been
found in the repositories
FreeBSD 15.0 has issues with packages.
Let's stop testing against it for now. We can track
FreeBSD-15 again when 15.1 is released.
Alex Rousskov [Wed, 22 Apr 2026 19:43:44 +0000 (19:43 +0000)]
Fix handling of truncated legacy errorpage %codes (#2411)
When build.input ends with a bare percent character, we must only
copy/consume that character and increment build.input by 1, not 2.
This overread bug existed since errorpage templates were introduced in
1997 commit 9b312a19. 2010 commit 4d16918e significantly broadened the
kinds of use cases that can trigger this bug.
Alex Rousskov [Fri, 17 Apr 2026 21:36:06 +0000 (21:36 +0000)]
Improve parsing of certain FTP directory listing formats (#2408)
This surgical fix restricts parsing to the input buffer when the listing
entry date in "TypeA" or "TypeB" formats is not followed by a filename.
It does not improve rendering of listings with missing filenames or the
overall quality of FTP listing parsing code.
C strchr() always returns a non-nil pointer when given a NUL character,
so its callers must be careful not to supply a NUL character if a
"natural" one-of-the-regular-c-string-characters membership test is
required.
The bug was probably introduced in 1997 commit 3fdadc70 and then
duplicated in 2017 commit 3d872090.
Cover all FreeBSD versions supported by the FreeBSD Project.
Fully rely on packages, not ports, solving the "version mismatch"
problem that had led us to remove FreeBSD 13.5 in commit 99fca3a.
Enable ccache for faster build times.
Restrict testing to the maximus test layer to maximize
signal-to-execution-time ratio: The main purpose of FreeBSD tests is to
quickly identify Linux-isms and portability issues, not internal code
dependencies and misalignments.
Prepare to also support arm64 and riscv64, but leave them disabled for
now as they are respectively too slow and broken at this time.
Amos Jeffries [Mon, 2 Mar 2026 13:56:46 +0000 (13:56 +0000)]
Maintenance: minor COPYING/CREDITS updates (#2383)
The current official GPLv2 text has been modified to
remove the FSF street address. Bring our GPL copy
up to date with the official GPLv2 license document.
Also, libltdl license has similar changes. Update our
CREDITS file to match the v2.5.3+ libltdl/ldtl.h import.
Alex Rousskov [Wed, 18 Feb 2026 21:13:26 +0000 (21:13 +0000)]
ICP: Fix HttpRequest lifetime for ICP v3 queries (#2377)
ACLFilledChecklist correctly locks and unlocks HttpRequest. Thus, when
given an unlocked request object, an on-stack checklist destroys it.
Upon icpAccessAllowed() return, Squid uses the destroyed request object.
This bug was probably introduced in 2003 commit 8000a965 that started
automatically unlocking requests in ACLChecklist destructor. However,
the bug did not affect allowed ICP v3 queries until 2007 commit f72fb56b
started _using_ the request object for them. 2005 commit 319bf5a7 fixed
an equivalent ICP v2 bug for denied queries but missed the ICP v3 case.
The scope, age, and effect of this bug imply that Squid v3+ deployments
receive no ICP v3 queries since 2007 (or earlier). Squid itself does not
send ICP v3 messages, responding with ICP v2 replies to ICP v3 queries.
TODO: Consider dropping ICP v3 support.
Also moved icpAccessAllowed() inside icpGetRequest() to deduplicate code
and reduce the risk of allowing a request without consulting icp_access.
Joshua Rogers [Thu, 12 Feb 2026 20:28:43 +0000 (20:28 +0000)]
ICP: Fix validation of packet sizes and URLs (#2220)
Fix handling of malformed ICP queries and replies instead of passing
invalid URL pointer to consumers, leading to out-of-bounds memory reads
and other problems. These fixes affect both ICP v2 and ICP v3 traffic.
* Reject packets with URLs that are not NUL-terminated.
* Reject packets with URLs containing embedded NULs or trailing garbage.
The above two restrictions may backfire if popular ICP agents do send
such malformed URLs, and we will need to do more to handle them
correctly, but it is _safe_ to reject them for now.
Also protect icpHandleUdp() from dereferencing a nil icpOutgoingConn
pointer. It is not clear whether icpHandleUdp() can be exposed to nil
icpOutgoingConn in current code. More work is needed to polish this.
Expand %x and %D after bumped SQUID_X509_V_ERR_DOMAIN_MISMATCH (#2373)
Squid detects SQUID_X509_V_ERR_DOMAIN_MISMATCH errors during various
processing stages, including when receiving an HTTP request on a
successfully bumped TLS connection. If that request targets a domain not
covered by the server certificate, and sslproxy_cert_error prohibits a
mismatch (it does by default), then Squid terminates the transaction
with an ERR_SECURE_CONNECT_FAIL response. That generated error response
body lacked %x and %D error details:
- [No Error Detail]
+ Certificate does not match domainname: /L=.../O=.../CN=example.com
```
The first `[No Error]` expansion of %E remains unchanged because this
particular error does not set `errno`.
ConnStateData::serveDelayedError() changes fix the above problem but %x
expansion in error pages and %err_detail in access log get a misleading
`+broken_cert` detail. To address that flaw, we changed the default for
broken certificate in Security::ErrorDetail constructor API from peer
certificate to nil. When broken certificate is nil, ErrorDetail now uses
valid certificate to expand %ssl_cn and similar certificate-inspecting
error page %codes.
All Security::ErrorDetail creators were checked and adjusted if needed:
* ConnStateData::serveDelayedError(): No caller changes. Using the new
ErrorDetail creation API fixes this code by supplying nil broken
certificate (because the certificate is _valid_ in this context).
* ssl_verify_cb(): No caller changes. We already use peer certificate as
the default broken certificate because doing so is "reasonable" here.
* Security::PeerConnector::sslCrtvdCheckForErrors(): Adjusted to keep
the original "if there was no error_cert_ID, then use peerCert"
behavior while using new Security::ErrorDetail creation API.
Thus, the last two contexts are not affected by this error reporting API
change. The exceptional serveDelayedError() caller is affected, but
Squid did not report any certificate detail in that case until this
branch fixes, so this branch does not change one "reporting certificate"
to another; it only starts reporting (important) information when none
was available before.
Joshua Rogers [Tue, 10 Feb 2026 19:58:49 +0000 (19:58 +0000)]
Do not escape malformed URI twice when sending ICP errors (#2374)
In this context, escaping escaped URI always produces incorrect URI
because `%` character in the escaped URI gets escaped again. Feeding the
result of the first rfc1738_escape() call to the second call is also
dangerously wrong because the result of the first call gets invalidated
during the second call.
No other cases of such "chained" rfc1738_escape() calls were found.
2023 commit 4e143970 accidentally removed code that was setting
`detailEntry` data member, breaking `%ssl_error_descr` expansion:
`Security::ErrorDetail::printErrorDescription()` would always print
`[Not available]`.
Squid still printed non-configurable request-independent error code
_name_ correctly because the corresponding `printErrorCode()` method
only uses `detailEntry` as a performance optimization.
The effects of this fix are visible, for example, in generated
ERR_SECURE_CONNECT_FAIL error responses:
```diff
- <p>[Not available]: /CN=...</p>
+ <p>Certificate does not match domainname: /CN=...</p>
```
Set SSL_OP_LEGACY_SERVER_CONNECT when peeking at servers (#2354)
Our TLS Server Hello parser does not treat legacy servers specially, but
enabling legacy server support in OpenSSL allows OpenSSL to advance
enough in its Server Hello processing to provide our SslBump code with
the server certificate (that we then validate). Successful certificate
validation, in turn, may result in Squid splicing the connection, even
if OpenSSL detected other errors:
noteNegotiationError: hold TLS write on FD 15 despite
SQUID_TLS_ERR_CONNECT+TLS_LIB_ERR=2000068+TLS_IO_ERR=1
A spliced connection allows the TLS client to handle a legacy server the
way the client needs to handle it, without unwanted Squid meddling.
When a unit test fails during CI checks, the corresponding GitHub
Actions reports and collected btlayer-*.log files do not contain
any failure details. For example, we see
FAIL: tests/testRock
# FAIL: 1
but are missing critical details like
stub time| FATAL: Ipc::Mem::Segment::create failed to
shm_open(/squid-0-tr_rebuild_versions.shm): (63) File name too long
Now, GitHub Actions collect all log files, including unit test logs. For
the `ubuntu-24.04,gcc,default` build target, adding more logs increases
artifacts zip archive size by about 100 KB (from ~200KB to ~300KB).
If `test-builds.sh` succeeds, there are no unit test logs to collect
because all unit test logs are erased when `make distcheck` (initiated
by `test-builds.sh`) reaches its `make distclean` step. If a unit test
fails, then that cleaning step is not reached, (successful and failed)
unit test logs are preserved and are now added to CI artifacts.
Amos Jeffries [Thu, 8 Jan 2026 14:09:08 +0000 (14:09 +0000)]
Drop obsolete AM_PROG_CC_C_O (#2342)
From automake manual:
"
This is an obsolescent macro that checks that the C compiler
supports the -c and -o options together. Note that, since
Automake 1.14, the AC_PROG_CC is rewritten to implement such
checks itself, and thus the explicit use of AM_PROG_CC_C_O
should no longer be required.
"
This also changes an implicit build requirement of automake 1.5
to an explicit 1.14 or later requirement.
Pavel Timofeev [Wed, 7 Jan 2026 07:16:41 +0000 (07:16 +0000)]
Fix SQUID_AUTO_LIB portability (#2337)
POSIX shell does not recognize `+=` as variable append operator.
./configure --with-heimdal-krb5=...
./configure: LIBHEIMDAL_KRB5_PATH+=-L/usr/lib: not found
./configure: LIBHEIMDAL_KRB5_CFLAGS+=-I/usr/include: not found
checking for LIBHEIMDAL_KRB5... no
configure: error: Required library 'heimdal-krb5' not found
Alex Rousskov [Thu, 25 Dec 2025 11:28:42 +0000 (11:28 +0000)]
Bug 5528: tests/testRock fails on Solaris (#2324)
with stub time| FATAL: Ipc::Mem::Segment::create failed to
shm_open(squid-0-tr_rebuild_stats.shm): (22) Invalid argument
Instance::NamePrefix() stub implementation ignored `head` and `tail`
arguments, resulting in malformed shared memory segment names on
Solaris. Other tested OSes tolerate the lack of a leading "/" character.
Linux shm_open(3) recommends "/somename" format "for portable use".
Simply adding `head` and `tail` to `NamePrefix()` result fixes tests on
Solaris but breaks tests on MacOS. We shortened the result (by removing
pid_filename hash component mimicking) to avoid that breakage and
detailed the problem in a C++ comment. More work is needed to replace
human-friendly name components with shorter hashes [on MacOS].
Squid C code does not need `libmiscencoding` anymore.
These changes are limited to making renamed files compile cleanly using
a C++ compiler. Library code still needs to be updated to follow best
C++ practices.
This was added for FreeBSD 7 issues which can not be replicated
using the current FreeBSD 14.
Heimdal package still lacks C++ wrappers, but this does not seem
to be affecting any currently supported OS. If necessary this
check can be restored with better details about how to replicate.
Amos Jeffries [Thu, 18 Dec 2025 07:22:37 +0000 (07:22 +0000)]
CI: Enforce Squid Project review process voting (#2067)
Implement the Squid Project merge procedure voting criteria
documented at https://wiki.squid-cache.org/MergeProcedure#votes
as a github action that can be applied as a merge requirement.
This will make the Squid merge procedure voting apply to prevent
Anubis bot attempting to start merge checks before approval limits,
as well as disabling github UI buttons.
Also, currently we rely on Anubis bot to enforce a 48hr hold. This
hold is added to the voting criteria checked.
Amos Jeffries [Thu, 18 Dec 2025 05:45:25 +0000 (05:45 +0000)]
CI: enable slow build tests for github merge queue (#2068)
Add the workflow piece(s) to make github run our "slow" tests
only on merge queue entries.
Github merge queues are now able to perform all the required
checks and details Squid Project applies to code additions.
The manual "Add to merge queue" button on github UI is an
identical procedural step replacing the current manual
"M-cleared-for-merge" label.
The "Add to merge queue" is not available to PRs until they pass
review approval and the "quick" code tests. The same criteria
Anubis uses to determine whether to start these "slow" tests.
For now Anubis and 'auto' branch are left as-is. They can be
dropped later when the merge queue is formally accepted for
use by Squid Project.
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.
projects / thirdparty / squid.git / log
