Amos Jeffries [Mon, 29 May 2017 03:19:47 +0000 (15:19 +1200)]
Add OpenSSL library details to -v output
This is partially to meet the OpenSSL copyright requirement that binaries
mention when they are using the library, and partially for admin to see
which library their Squid is using when multiple are present in the system.
Crashes when server-first bumping mode is used with openSSL-1.1.0 release
When OpenSSL-1.1.0 or later is used:
- The SQUID_USE_SSLGETCERTIFICATE_HACK configure test is false
- The SQUID_SSLGETCERTIFICATE_BUGGY configure test is true
- Squid hits an assert(0) inside Ssl::verifySslCertificate when trying to
retrieve a generated certificate from cache.
Create PID file ASAP, before the shared memory segments.
PID file is created right after configuration finalization, before the
allocation for any shared memory segments.
Late PID file creation allowed N+1 concurrent Squid instances to create
the same set of shared segments (overwriting each other segments),
resulting in extremely confusing havoc because the N instances would
later lose the race for the PID file (or some other critical resource)
creation and remove the segments. If that removal happened before a kid
of the single surviving instance started, that kid would fail to start
with open() errors in Segment.cc because the shared segment it tries to
open would be gone. Otherwise, that kid would fail to _restart_ after
any unrelated failures (possibly many days after the conflict), with
same errors, for the same reason.
Shared state corruption was also possible if different kids (of the
winning instance) opened (and started using) segments created (and
initialized) by different instances.
Situations with N+1 concurrent Squid instances are not uncommon because
many Squid service management scripts (or manual admin commands!)
* do not check whether another Squid is already running and/or
* incorrectly assume that "squid -z" does not daemonize.
This change finally makes starting N+1 Squid instances safe (AFAIK).
Also made daemonized and non-daemonized Squid create the PID file at the
same startup stage, reducing inconsistencies between the two modes.
Make PID file check/creation atomic to avoid associated race conditions.
After this change, if N Squid instances are concurrently started shortly
after time TS, then exactly one Squid instance (X) will run (and have
the corresponding PID file). If another Squid instance has already been
running (with the corresponding PID file) at TS, then X will be that
"old" Squid instance. If no Squid instances were running at TS, then X
will be one of those new N Squids started after TS.
Lack of atomic PID file operations caused unexpected Squid behavior:
* Mismatch between started Squid instance and stored PID file.
* Unexpected crashes due to failed allocation of shared resources,
such as listening TCP ports or shared memory segments.
A new File class guarantees atomic PID file operations using locks. We
tried to generalize/reuse Ssl::Lock from the certificate generation
helper, but that was a bad idea: Helpers cannot use a lot of Squid code
(e.g., debugs(), TextException, SBuf, and enter_suid()), and the old
Ssl::Lock class cannot support shared locking without a major rewrite.
File locks on Solaris cannot work well (see bug #4212 comment #14), but
those problems do not affect PID file management code. Solaris- and
Windows-specific File code has not been tested and may not build.
Failure to write a PID file is now fatal. It used to be fatal only when
Squid was started with the -C command line option. In the increasingly
SMP world, running without a PID file leads to difficult-to-triage
errors. An admin who does not care about PID files should disable them.
Squid now exits with a non-zero error code if another Squid is running.
Also removed PID file rewriting during reconfiguration in non-daemon
mode. Squid daemons do not support PID file reconfiguration since trunk
r13867, but that revision (accidentally?) left behind half-broken
reconfiguration code for non-daemon mode. Fixing that code is difficult,
and supporting PID reconfigure in non-daemons is probably unnecessary.
Also fixed "is Squid running?" check when kill(0) does not have
permissions to signal the other instance. This does happen when Squid is
started (e.g., on the command line) by a different user than the user
Squid normally runs as or, perhaps, when the other Squid instance enters
a privileged section at the time of the check (untested). The bug could
result in undelivered signals or multiple running Squid instances.
These changes do not alter partially broken enter/leave_suid() behavior
of main.cc. That old code will need to be fixed separately!
PID file-related cache.log messages have changed slightly to improve
consistency with other DBG_IMPORTANT messages and to simplify code.
Squid no longer lies about creating a non-configured PID file. TODO:
Consider lowering the importance of these benign/boring messages.
* Terminal errors should throw instead of calling exit()
Squid used to call exit() in many PID-related error cases. Using exit()
as an error handling mechanism creates several problems:
1. exit() does not unwind the stack, possibly executing atexit()
handlers in the wrong (e.g., privileged) context, possibly leaving
some RAII-controller resources in bad state, and complicating triage;
2. Using exit() complicates code by adding a yet another error handling
mechanism to the (appropriate) exceptions and assertions.
3. Spreading exit() calls around the code obscures unreachable code
areas, complicates unifying exit codes, and confuses code checkers.
Long-term, it is best to use exceptions for nearly all error handling.
Reaching that goal will take time, but we can and should move in that
direction: The adjusted SquidMainSafe() treats exceptions as fatal
errors, without dumping core or assuming that no exception can reach
SquidMainSafe() on purpose. This trivial-looking change significantly
simplified (and otherwise improved) PID-file handling code!
The fatal()-related code suffers from similar (and other) problems, but
we did not need to touch it.
TODO: Audit catch(...) and exit() cases [in main.cc] to take advantage
of the new SquidMainSafe() code supporting the throw-on-errors approach.
Alex Rousskov [Mon, 29 May 2017 00:18:24 +0000 (12:18 +1200)]
Do not unconditionally revive dead peers after a DNS refresh.
Every hour, peerRefreshDNS() performs a DNS lookup of all cache_peer
addresses. Before this patch, even if the lookup results did not change,
the associated peerDNSConfigure() code silently cleared dead peer
marking (CachePeer::tcp_up counter), if any. Forcefully reviving dead
peers every hour can lead to transaction delays (and delays may lead to
failures) due to connection timeouts when using a still dead peer.
This patch starts standard TCP probing (instead of pointless dead peer
reviving), correctly refreshing peer state. The primary goal is to
cover a situation where a DNS refresh changes the peer address list.
However, TCP probing may be useful for other situations as well and has
low overhead (that is why it starts unconditionally). For example,
probing may be useful when the DNS refresh changes the order of IP
addresses. It also helps detecting dead idle peers.
Also delay and later resume peer probing if peerDNSConfigure() is
invoked when peers are being probed. Squid should re-probe because the
current probes may use stale IP addresses and produce wrong results.
xstrndup() does not work like strndup(3), and some callers got confused:
1. When n is the str length or less, standard strndup(str,n) copies all
n bytes but our xstrndup(str,n) drops the last one. Thus, all callers
must add one to the desired result length when calling xstrndup().
Most already do, but it is often hard to see due to low code quality
(e.g., one must remember that MAX_URL is not the maximum URL length).
2. xstrndup() also assumes that the source string is 0-terminated. This
dangerous assumption does not contradict many official strndup(3)
descriptions, but that lack of contradiction is actually a recently
fixed POSIX documentation bug (i.e., correct implementations must not
assume 0-termination): http://austingroupbugs.net/view.php?id=1019
The OutOfBoundsException bug led to truncated exception messages.
The ESI bug led to truncated 'literal strings', but I do not know what
that means in terms of user impact. That ESI fix is untested.
cachemgr.cc bug was masked by the fact that the buffer ends with \n
that is unused and stripped by the custom xstrtok() implementation.
TODO. Fix xstrndup() implementation (and rename the function so that
fixed callers do not misbehave if carelessly ported to older Squids).
This ACL detects presence of request, response or ALE transaction
components. Since many ACLs require some of these components, lack of
them in a transaction may spoil the check and confuse admin with
warnings like "... ACL is used in context without an HTTP request".
Using 'has' ACL should help dealing with these problems caused by
component-less transactions.
Also: addressed TODO in item #3 of v4 revision 14752.
Bug 4321: ssl_bump terminate does not terminate at step1
The following trivial configuration should terminate all connections that
are subject to SslBumping:
ssl_bump terminate all
but Squid either splices or bumps instead.
This patch fixes Squid to immediately close the connection.
Also this patch:
- solves wrong use of Ssl::bumpNone in cases where the Ssl::bumpEnd
(do not bump) or Ssl::bumpSplice (splice after peek/stare at step1)
must be used.
- updates %ssl::bump_mode documetation.
- fixes %ssl::bump_mode formating code to print last bumping action
Squid does not forward HTTP transactions to dead peers except when a
dead peer was idle for some time (ten peer connect timeouts or longer).
When the idle peer is still dead, this exception leads to transaction
delays (at best) or client disconnects/errors (at worst), depending on
Squid and client configurations/state. I am removing this exception.
The "use dead idle peer" heuristic was introduced as a small part of a
much bigger bug #14 fix (trunk r6631). AFAICT, the stated goal of the
feature was speeding up failure recovery: The heuristic may result in
HTTP transactions sent to a previously dead (but now alive) idle peer
earlier, before the peer is proven to be alive (using peer revival
mechanisms such as TCP probes). However, the negative side effects of
this heuristic outweigh its accidental benefits. If somebody needs Squid
to detect revived idle peers earlier, they need to add a different
probing mechanism that does not jeopardize HTTP transactions.
Nobody has spoken in defense of this feature on Squid mailing lists:
http://lists.squid-cache.org/pipermail/squid-users/2017-March/014785.html
http://lists.squid-cache.org/pipermail/squid-dev/2017-March/008308.html
The removed functionality was not used to detect revived peers. All peer
revival mechanisms (such as TCP probes) remain intact.
Bug 4711: SubjectAlternativeNames is missing in some generated certificates
Squid may generate certificates which have a Common Name, but do not have
a subjectAltName extension. For example when squid generated certificates
do not mimic an origin certificate or when the certificate adaptation
algorithm sslproxy_cert_adapt/setCommonName is used.
This is causes problems to some browsers, which validates a certificate using
the SubjectAlternativeNames but ignore the CommonName field.
This patch fixes squid to always add a SubjectAlternativeNames extension in
generated certificates which do not mimic an origin certificate.
Squid still will not add a subjectAltName extension when mimicking an origin
server certificate, even if that origin server certificate does not include
the subjectAltName extension. Such origin server may have problems when
talking directly to browsers, and patched Squid is not trying to fix those
problems.
Bug 4682: ignoring http_access deny when client-first bumping mode is used
Squid fails to identify HTTP requests which are tunneled inside an already
established client-first bumped tunnel, and this is results in ignoring
http_access denied for these requests.
Squid does not send CONNECT request to adaptation services
if the "ssl_bump splice" rule matched at step 2. This adaptation
is important because the CONNECT request gains SNI information during
the second SslBump step. This is a regression bug, possibly caused by
the Squid bug 4529 fix (trunk commits r14913 and r14914).
Count failures and use peer-specific connect timeouts when tunneling.
Fixed two bugs with tunneling CONNECT requests (or equivalent traffic)
through a cache_peer:
1. Not detecting dead cache_peers due to missing code to count peer
connect failures. TLS/SSL-level failures were detected (for "tls"
cache_peers) but TCP/IP connect(2) failures were not (for all peers).
2. Origin server connect_timeout used instead of peer_connect_timeout or
a peer-specific connect-timeout=N (where configured).
The regular forwarding code path does not have the above bugs. This
change reduces code duplication across the two code paths (that
duplication probably caused these bugs in the first place), but a lot
more work is needed in that direction.
The 5-second forwarding timeout hack has been in Squid since
forward_timeout inception (r6733). It is not without problems (now
marked with an XXX), but I left it as is to avoid opening another
Pandora box. The hack now applies to the tunneling code path as well.
Bug 4659: sslproxy_foreign_intermediate_certs does not work
The sslproxy_foreign_intermediate_certs directive does not work after r14769.
The bug is caused because of wrong use of X509_check_issued OpenSSL API call.
Amos Jeffries [Thu, 4 May 2017 10:12:39 +0000 (22:12 +1200)]
ext_session_acl: cope with new logformat inputs
Now that Squid is sending an explicit '-' for the trailing %DATA parameter
if there were no acl parameters this helper needs to cope with it on
'active mode' session lookups when login/logout are not being performed.
Amos Jeffries [Thu, 4 May 2017 06:05:54 +0000 (18:05 +1200)]
ext_session_acl: cope with new logformat inputs
Now that Squid is sending an explicit '-' for the trailing %DATA parameter
if there were no acl parameters this helper needs to cope with it on
'active mode' session lookups when login/logout are not being performed.
Fixes squid documentation to correctly describe the squid behavior when the
"bump" action is selected on step SslBump1. In this case squid selects
the client-first bumping mode.
Most of the logic seems to be hangovers from when session helper was
using the BerkleyDB v1.85 compatibility interface. Some of it is
possibly still necessary for the time_quota helper, but that helper has
not been using it so far and needs an upgrade to match what happened to
session helper.
Changes:
* The helpers needing -ldb will not be built unless the library and
headers are available. So we can drop the Makefile LIB_DB substitutions
and always just link -ldb explicitly to these helpers.
NP: Anyone who needs small minimal binaries, can build with the
--as-needed linker flag, or without these helpers. This change has no
effect on other helpers or the main squid binary.
* Since we no longer need to check if -ldb is necessary, we can drop the
configure.ac and acinclude logic detecting that.
* Remove unused AC_CHECK_DECL(dbopen, ...)
- resolves one "FIXME"
* Fix the time_quota helper check to only scan db.h header file contents
if that file is existing, and if the db_185.h file is not being used
instead.
* Fix the session helper check to only try compiling with the db.h
header if that header actually exists.
* De-duplicate the library header file detection shared by configure.ac
and the helpers required.m4 files (after the above two changes).
Amos Jeffries [Fri, 31 Mar 2017 13:52:56 +0000 (02:52 +1300)]
Bug 4671 pt4: refactor Format::assemble()
* replace the String local with an SBuf to get appendf()
* overdue removal of empty lines and '!= NULL' conditions
* reduce scope redux for many out assignments
* use sizeof(tmp) instead of '1024'
* Fixes many GCC 7 compile errors from snprintf() being called with a
too-small buffer.
* update the for-loops in Adaptation::History to C++11 and produce output
in an SBuf. Removing need for iterator typedef's and resolving more GCC 7
warnings about too-small buffers for snprintf().
Amos Jeffries [Fri, 31 Mar 2017 12:53:33 +0000 (01:53 +1300)]
Bug 4671 pt3: remove limit on FTP realm strings
Convert ftpRealm() from generating char* to SBuf. This fixes issues identified
by GCC 7 where the realm string may be longer than the available buffer and
gets truncated.
The size of the buffer was making the occurance rather rare, but it is still
possible.
Squid may fail to load cache entry metadata for several very different
reasons, including the following two relatively common ones:
* A cache_dir entry corruption.
* Huge cache_dir entry metadata that does not fit into the I/O buffer
used for loading entry metadata.
Knowing the exact failure reason may help triage and guide development.
We refactored existing checks to distinguish various error cases,
including the two above. Refactoring also reduced code duplication.
These improvements also uncovered and fixed a null pointer dereference
inside ufsdump.cc (but ufsdump does not even build right now for reasons
unrelated to these changes).
Alex Rousskov [Fri, 31 Mar 2017 12:06:22 +0000 (01:06 +1300)]
Fixed URI scheme case-sensitivity treatment broken since r14802.
A parsed value for the AnyP::UriScheme image constructor parameter was
stored without toLower() canonicalization for known protocols (e.g.,
Squid would store "HTTP" instead of "http" after successfully parsing
"HTTP://EXAMPLE.COM/" in urlParseFinish()). Without that
canonicalization step, Squid violated various HTTP caching rules related
to URI comparison (and served fewer hits) when dealing with absolute
URLs containing non-lowercase HTTP scheme.
According to my limited tests, URL-based ACLs are not affected by this
bug, but I have not investigated how URL-based ACL code differs from
caching code when it comes to stored URL access and whether some ACLs
are actually affected in some environments.
Fix two read-ahead problems related to delay pools (or lack of thereof).
1. Honor EOF on Squid-to-server connections with full read ahead buffers
and no clients when --enable-delay-pools is used without any delay
pools configured in squid.conf.
Since trunk r6150.
Squid delays reading from the server after buffering read_ahead_gap
bytes that are not yet sent to the client. A delayed read is normally
resumed after Squid sends more buffered bytes to the client. See
readAheadPolicyCanRead() and kickReads().
However, Squid was not resuming the delayed read after all Store clients
were gone. If quick_abort prevents Squid from immediately closing the
corresponding Squid-to-server connection, then the connection gets stuck
until read_timeout (15m), even if the server closes much sooner, --
without reading from the server, Squid cannot detect the connection
closure. The affected connections enter the CLOSE_WAIT state.
Kicking delayed read when the last client leaves fixes the problem. The
removal of any client, including the last one, may change
readAheadPolicyCanRead() answer and, hence, deserves a kickReads() call.
Why "without any delay pools configured in squid.conf"? When classic
(i.e., delay_pool_*) delay pools are configured, Squid kicks all delayed
reads every second. That periodic kicking is an old design bug, but it
resumes stuck reads when all Store clients are gone. Without classic
delay pools, there is no periodic kicking. This fix does not address
that old bug but removes Squid hidden dependence on its side effect.
Note that the Squid-to-server connections with full read-ahead buffers
still remain "stuck" if there are non-reading clients. There is nothing
Squid can do about them because we cannot reliably detect EOF without
reading at least one byte and such reading is not allowed by the read
ahead gap. In other words, non-reading clients still stall server
connections.
While fixing this, I moved all CheckQuickAbort() tests into
CheckQuickAbortIsReasonable() because we need a boolean function to
avoid kicking aborted entries and because the old separation was rather
awkward -- CheckQuickAbort() contained "reasonable" tests that were not
in CheckQuickAbortIsReasonable(). All the aborting tests and their order
were preserved during this move. The moved tests gained debugging.
According to the existing test order in CheckQuickAbortIsReasonable(),
the above problem can be caused by:
* non-private responses with a known content length
* non-private responses with unknown content length, having quick_abort_min
set to -1 KB.
2. Honor read_ahead_gap with --disable-delay-pools.
Since trunk r13954.
This fix also addresses "Perhaps these two calls should both live
in MemObject" comment and eliminates existing code duplication.
Amos Jeffries [Thu, 2 Mar 2017 11:52:42 +0000 (00:52 +1300)]
Remove some NULL references from adaptation/icap/ModXact.cc
The issue is that Coverity Scan gets confused by implicit casting of NULL
to a Pointer into thinking that 'true' comparison is possible when NULL
is involved. The != should still compile to the correct checks.
Amos Jeffries [Thu, 23 Feb 2017 13:31:16 +0000 (02:31 +1300)]
Crypto-NG: initial GnuTLS support for encrypted server connections
Make significant changes to how the options= config settings are
handled internally since GnuTLS does not expose the priority_t
implementation details like OpenSSL. They are also applied to the
session object instead of to the context.
The Security::SessionPointer is converted to std::shared_ptr. This is
required because GnuTLS does not expose the locking like OpenSSL. Since
we store the SessionPointer to fde::Table::ssl we can always access it
from there one way or another and there is actually no need for OpenSSL
locking sessions now.
Most of the remaining session lifecycle logic is moved to
security/Session.* and given a generic API. Only some client-connection
and SSL-Bump related setup remains in ssl/.
A fair amount more debug is added along with some text changes doing
s/SSL/TLS/ in code comments and debug outputs.
The Security::ContextPointer is also converted to a std::shared_ptr. As
with SessionPointer these are stored in the long-term global config
objects and referenced from there as needed.
Support for https:// URL outgoing connections is enabled with GnuTLS.
Emmanuel Fuste [Mon, 20 Feb 2017 02:45:32 +0000 (15:45 +1300)]
digest_ldap_auth: Add -r option to clamp the realm to a fixed value
Some historic Digest Auth implementations do not include the realm in the
digest password attribute. The password is effectively stored as "HA1"
instead of "REALM:HA1".
The realm cannot simply be ignored due to:
1) the realm is both the salting value used within the hash and the
scope limitation on what inputs from HTTP are used to compare against
the A1, and
2) Squid does not itself verify the realm received was the one offered
and leaves the comparison to the backend system. There is some
possibility the authentication system is using multiple security realms
and Squids realm string is just an offer.
Not having realm tied to the credentials in the backend storage leaves
this particular helper with no other option but to trust the realm sent
(probably) over clear-text by any client/attacker actually matches the
salting. That allows remote senders to manipulate the realm string they
send to perform a collision attack against the stored password.
They no longer have to find and prove knowledge of the password. But
just find a collision for its hash vs arbitrary realm strings.
Old Digest systems are not the safest things to begin with. They also
tend to use MD5 hashing which was the only one available for many years
and relatively easy to find collisions for.
To resolve all these problems allow the -l parameter to accept an empty
string ('-l "" ') when the -r option provides a fixed realm.
Amos Jeffries [Thu, 9 Feb 2017 02:41:58 +0000 (15:41 +1300)]
GCC7: raise FTP Gateway CTRL channel buffer to 16KB
Fixes
error: %s directive output may be truncated writing up to 8191 bytes
into a region of size 1019
note: snprintf output between 8 and 8199 bytes into a destination of
size 1024
Bump SSL client on [more] errors encountered before ssl_bump evaluation
... such as ERR_ACCESS_DENIED with HTTP/403 Forbidden triggered by an
http_access deny rule match.
The old code allowed ssl_bump step1 rules to be evaluated in the
presence of an error. An ssl_bump splicing decision would then trigger
the useless "send the error to the client now" processing logic instead
of going down the "to serve an error, bump the client first" path.
Furthermore, the ssl_bump evaluation result itself could be surprising
to the admin because ssl_bump (and most other) rules are not meant to be
evaluated for a transaction in an error state. This complicated triage.
Also polished an important comment to clarify that we want to bump on
error if (and only if) the SslBump feature is applicable to the failed
transaction (i.e., if the ssl_bump rules would have been evaluated if
there were no prior errors). The old comment could have been
misinterpreted that ssl_bump rules must be evaluated to allow an
"ssl_bump splice" match to hide the error.
Alex Rousskov [Sun, 5 Feb 2017 20:56:55 +0000 (09:56 +1300)]
Fix crash when configuring with invalid delay_parameters restore value.
... like none/none. Introduced in r13701 which fixed another, much
bigger delay_parameters parsing bug.
TODO: Reject all invalid input, including restore/max of "-/100".
TODO: Fix misleading/wrong associated error messages. For example:
ERROR: invalid delay rate 'none/none'. Expecting restore/max or 'none'
ERROR: restore rate in '1/none' is not a number.
This method was improperly used in contexts where actually
Auth::UserRequest::setDenyMessage() expected. Probably the reason was
that both denyMessage() and getDenyMessage() were not constant,
provoking such 'misuse'.
Also placed some common code into UserRequest::denyMessageFromHelper(),
eliminating code duplication. Though there are still many places
where code is duplicated inside auth/ntlm/UserRequest.cc and
auth/negotiate/UserRequest.cc.
SSLv2 records force SslBump bumping despite a matching step2 peek rule.
If Squid receives a valid TLS Hello encapsulated into ancient SSLv2
records (observed on Solaris 10), the old code ignored the step2 peek
decision and bumped the transaction instead.
Now Squid peeks (or stares) at the origin server as configured, even
after detecting (and parsing) SSLv2 records.
Mitigate DoS attacks that use client-initiated SSL/TLS renegotiation.
There is a well-known DoS attack using client-initiated SSL/TLS
renegotiation. The severety or uniqueness of this attack method
is disputed, but many believe it is serious/real.
There is even a (disputed) CVE 2011-1473:
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2011-1473
The old Squid code tried to disable client-initiated renegotiation, but
it did not work reliably (or at all), depending on Squid version, due
to OpenSSL API changes and conflicting SslBump callbacks. That
code is now removed and client-initiated renegotiations are allowed.
With this change, Squid aborts the TLS connection, with a level-1 ERROR
message if the rate of client-initiated renegotiate requests exceeds
5 requests in 10 seconds (approximately). This protection and the rate
limit are currently hard-coded but the rate is not expected to be
exceeded under normal circumstances.
Update External ACL helpers error handling and caching
The helper protocol for external ACLs [1] defines three possible return values:
OK - Success. ACL test matches.
ERR - Success. ACL test fails to match.
BH - Failure. The helper encountered a problem.
The external acl helpers distributed with squid currently do not follow this
definition. For example, upon connection error, ERR is returned:
$ ext_ldap_group_acl ... -d
ext_ldap_group_acl: WARNING: could not bind to binddn 'Can't contact LDAP server'
ERR
This does not allow to distinguish "no match" and "error" either and
therefore negative caches "ERR", also in the case of an error.
Moreover there are multiple problems inside squid when trying to handle BH
responses:
- Squid-5 and Squid-4 retry requests for BH responses but crashes after the
maximum retry number (currently 2) is reached.
- If an external acl helper return always BH (eg because the LDAP server is
down) squid sends infinitely new request to the helper.
Fix "Source and destination overlap in memcpy" Valgrind errors
Before this patch, source and destination arguments in
log_quoted_string() could point to the same static memory area, causing
multiple Valgrind-reported errors. Fixed by creating another buffer to
store quoted-processed output string.
Reduce crashes due to unexpected ClientHttpRequest termination.
The underlying problem has been known since r13480: If a
ClientHttpRequest job ends without Http::Stream (and ConnStateData)
knowledge, then Squid is likely to segfault or assert. This patch does
not resolve the underlying issue (a proper fix would require
architectural changes in a consensus-lacking area) but makes an
unexpected ClientHttpRequest job destruction less likely.
BodyPipe and Adaptation-related exceptions are the major causes of
unexpected ClientHttpRequest job destruction. This patch handles them by
closing the client connection. Connection closure should trigger an
orderly top-down cleanup, including Http::Stream, ConnStateData, and
ClientHttpRequest destruction.
If there is no connection to close, then the exception is essentially
ignored with a level-1 error message disclosing the problem. The side
effects of ignoring such exceptions are unknown, but without a client
connection, it is our hope that they would be relatively benign.
Amos Jeffries [Sat, 7 Jan 2017 11:01:20 +0000 (00:01 +1300)]
Detect HTTP header ACL issues
rep_header and req_header ACL types cannot match multiple different
headers in one test (unlike Squid-2 appears to have done). Produce
an ERROR and ignore the extra line(s) instead of silently changing
all the previous regex to match the second header name.
Also detect and ERROR when header name is missing entirely. Ignore
these lines instead of asserting.
Bug 4599 pt6: Restrict safe renegotiation disable to OpenSSL 1.0 and older
From OpenSSL 1.1.0 documentation:
"OpenSSL always attempts to use secure renegotiation as described in RFC5746.
This counters the prefix attack described in CVE-2009-3555 and elsewhere."
From OpenSSL changeLog:
Remove support for all 40 and 56 bit ciphers. This includes all the export
ciphers who are no longer supported and drops support the ephemeral RSA key
exchange. The LOW ciphers currently doesn't have any ciphers in it.
[Kurt Roeckx]
OpenSSL 1.1.0 drops EXPORT ciphers and ephemeral RSA. The ssl_temp_rsa_cb used
here. The OpenSSL manual for this function is:
https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_tmp_rsa_callback(3)
Amos Jeffries [Mon, 26 Dec 2016 02:15:22 +0000 (15:15 +1300)]
Bug 3940 pt2: Make 'cache deny' do what is documented
Instead of overriding whatever cacheability was previously set to
(including changing non-cacheables to be cacheable) actually
prevent both cache read and write.
This 'regression' appears after initial bug 3940 fix.
Do not share private responses with collapsed client(s).
This excessive sharing problem with collapsed forwarding code has
several layers. In most cases, the core CF code does not share
uncachable or private response with collapsed clients because of the
refreshCheckHTTP() check. However, some responses might not be subject
to that (or equivalent) check. More importantly, collapsed revalidation
code does not check its responses at all and, hence, easily shares
private responses.
This short-term fix incorrectly assumes that an entry may become private
(KEY_PRIVATE) only when it cannot be shared among multiple clients
(e.g., because of a Cache-Control:private response header). However,
there are a few other cases when an entry becomes private. One of them
is a DISK_NO_SPACE_LEFT error inside storeSwapOutFileClosed() where
StoreEntry::releaseRequest() sets KEY_PRIVATE for a sharable entry [that
may still be perfectly preserved in the memory cache]. Consequently, the
short-term fix reduces CF effectiveness. The extent of this reduction is
probably environment-dependent.
Also: do not re-use SET_COOKIE headers for collapsed revalidation slaves,
i.e., adhere to the same requirement as for regular response HITs.
Garri Djavadyan [Fri, 9 Dec 2016 04:42:34 +0000 (17:42 +1300)]
Bug 3940 (partial): hostHeaderVerify failures MISS when they should be HIT
This fixes the critical condition leading to the HIT. However not all
code is correctly setting flags.noCache and flags.cacheable (see bugzilla).
So there may be other fixes needed after this.
projects / thirdparty / squid.git / log
