Alex Rousskov [Fri, 19 Feb 2016 21:26:00 +0000 (14:26 -0700)]
Fix propagation of response status line parsing error details.
This is a follow-up patch to trunk r14548 (Bug 4432). Now that the
calling code is using the right field to get the parsing error details
(parseStatusCode), we need to fix the code that sets those parsing error
details [in case of response status line parsing errors].
TODO: To minimize chances of similar "I forgot to set parseStatusCode"
bugs slipping through, hide that data member behind a method that
returns scInvalidHeader (or a new scInternalSquidError) if parseError_
is still zero. Rename parseStatusCode to parseError_ and stop confusing
it with the response status code.
Alex Rousskov [Fri, 19 Feb 2016 21:23:08 +0000 (14:23 -0700)]
Throw instead of asserting on some String overflows.
Note that Client-caught exceptions result in HTTP 500 (Internal Server
Error) responses with X-Squid-Error set to "ERR_CANNOT_FORWARD 0".
Also avoid stuck Client jobs on exceptions. See trunk r8266 for a
similar fix with a detailed discussion. Here, I added doneWithFwd
instead of setting fwd to NULL because we dereference fwd (and store
pointers to things stored in fwd!) in many places. I think it is too
risky to just clear refcounted FwdState pointer (except in the
destructor where doing so is pointless).
Using doneWithFwd correctly is difficult because there are many ways we
can be "done" with FwdState, including:
* calling fwd->complete(),
* calling fwd->handleUnregisteredServerEnd(), and
* closing the connection that FwdState monitors for closures.
The latter is especially tricky case because the closing is initiated in
many places, the process is asynchronous, and not all control
connections are monitored by FwdState.
For example, the updated control connection closure handler assumes that
it is being used for either external closures or internal closures
incorrectly used instead of mustStop()/abortAll(). In both cases, either
FwdState is still monitoring the connection (OK) or we forgot to call
one of its "done" methods listed above before closing. The latter would
be a bug, but I did not find any signs of it and fixing it would be
outside this change scope anyway.
Also unified String size limit checks [that I could find].
external_acl parameters separated by %20 instead of space
If an external ACL is configured with more than one parameter as shown
in the example below, then Squid sends those parameters to the
external_acl helper separated by %20 characters instead of spaces:
acl TEST external ACLTYPE param1=val1 param2=val2
This change fixes regression introduced in trunk r14351 (Support
logformat %macros in external_acl_type format) but more work may
be needed to make Squid behave as squid.conf.documented promises.
Amos Jeffries [Fri, 19 Feb 2016 15:06:42 +0000 (04:06 +1300)]
Revert r14303: Migrate StoreEntry to using MEMPROXY_CLASS
This change has been identified as the trigger for several object caching
errors. The real cause is not yet known, but reverting this optimisation
avoids it, so is being done for stability.
This resolves bugs 4370 and maybe also 4354 and 4355
William Lima [Thu, 18 Feb 2016 12:48:08 +0000 (01:48 +1300)]
Bug 3870: assertion failed: String.cc: 'len_ + len <65536' in ESI::CustomParser
The custom ESI parser used in absence of libxml2 or libexpat parsers was
restricted to handling 64KB buffers but under some conditions could expand
to over 64KB during the parse process. Hitting this assertion.
TODO: the parser can now be redesigned to make use of Tokenizer and
CharacterSet parsing tools. But that is left for later work.
* Do not use parsing leftovers, such as HTTP response status code. Doing
so screws up error detection logic in continueAfterParsingHeader() and
leads to stuck transactions instead of error responses.
* Do not store the fake half-baked response (via replaceHttpReply).
Doing so leads to assertions. The fake response is only meant for
continueAfterParsingHeader().
I also removed a misleading XXX about connection closure. Our
continueAfterParsingHeader() handles errors, not processReplyHeader().
TODO: The error detection/propagation code is ugly and should be
rewritten [using C++ exceptions].
tangqinghao [Thu, 18 Feb 2016 02:48:41 +0000 (15:48 +1300)]
Bug 4111: leave_suid() does not properly handle error codes returned by setuid
... this will cause privilege escalation in the rare case that setuid fails.
So far there are no known cases of this happening when downgrading from root.
Also fixes several incorrect uses of errno which may have been obscuring
error message details if it did happen.
Amos Jeffries [Wed, 17 Feb 2016 21:03:29 +0000 (10:03 +1300)]
SourceLayout: Move the Runner which manages SSL SessionCache to libsecurity
Unfortunately the OpenSSL session cache callbacks cannot also be moved
due to circular dependency issues. However, when those are resolved by
later libsecurity API additions the callbacks will be much easier to
shift. For now the three symbols shared between the two libraries are
exposed by libsslsquid in the Ssl:: namespace.
Cache initialization is now moved into the Runner. Binding its state
initialization more tightly to the memory allocation and initialization.
Which also removes the need for explicit main.cc dependency.
One issue was uncovered during this:
* While ssl/support.h was defining a destruct_session_cache() function
that appeared to release the cache memory, it was not actually being
used anywhere. Which unless a fortuitous sequence of events is happening
means that the memory for the cache entries may not be released properly.
On the other hand the cache should only be erased on shutdown so the
effects of this are minor.
The unused function has been removed and the issue is now expicitly
noted in the Runner shutdown handling method for future investigation.
In the case SSL errors detected by certificate validator helper the objects
stored in Ssl::ServerBump::sslErrors member and will never released.
This member normally points to an Ssl::CertErrors list attached to the related
SSL object which is responsible to release this list.
When the cert validator detects errors a new errors list created and attached
to the related Ssl::ServerBump::sslErrors member but the SSL objects still
hold the old one. The old list released but not the new one.
This patch also fixes the case the cbdata protected Ssl::CertErrors list,
still is used through the related Ssl::ServerBump object but it is not valid
any more, because the SSL object which hold it gone.
This patch instead of storing the Ssl::CertErrors list to Ssl::ServerBump
object stores the SSL object and increases its reference to avoid be released
Bug 4437: Fix Segfault on Certain SSL Handshake Errors
Squid after an unsuccesfull try to connect to the remote server may make two
concurrent retries to connect to the remote SSL server, calling twice the
FwdState::retryOrBail() method, which may result to unexpected behaviour.
Prevent this by just closing the connection to the remote SSL server inside
FwdState::connectedToPeer method on error and instead of calling the
FwdState::retryOrBail method, just allow comm_close handler to retry the
connection if required.
This bug investigated after the r14528 patch.
The (forgotten?) Ssl::PeekingPeerConnector::callback member is always NULL and
hides the callback member of the parent Ssl::PeerConnector class. This is has
as result the failure of "Must(callback != NULL)" clause inside the
Ssl::PeekingPeerConnector::tunnelInsteadOfNegotiating method investigated with
the r14528 patch.
Amos Jeffries [Wed, 10 Feb 2016 09:36:47 +0000 (22:36 +1300)]
Cleanup: const correctness for SBuf iterators
The SBuf iterator has almost all properties of a const_iterator but the
begin/end methods producing it had the syntax for returning a non-const
iterator. This leads to several const related problems despite the
SBufIterator providing a const API:
1) range-for loops cannot be used on const SBuf
2) begin/end/rbegin/rend operators cannot be used on const SBuf
Also, the SBufIterator API for operator*() used a temporary char
which, wile const, allows the compiler to implicitly use move semantics
on the return value and allows some (thankfully unused) &*itr() syntax
to compile when it should not be possible.
To avoid even the potential of that causing hidden issues in future we
convert the operator*() to the API definition used widely within the STL
const_iterator's (const char &operator*() const).
Amos Jeffries [Tue, 9 Feb 2016 08:57:33 +0000 (21:57 +1300)]
Cleanup: convenience library renaming
I have been trying to automate graphing of the Squid internal
dependencies. One of the major issues that has encountered is that some
of our convenience libraries use the '-' hyphen character which is a
reserved character in DOT graph format.
To make the scripts much simpler and the visual output reflect exactly
what the library names are this patch cleans up the libraries to follow
our pre-existing policy, and now also to remove punctuation from library
names. Which condition has been added to the guidelines documentation.
The information about PeekingPeerConnector splicing the connections
was lost in some cases, resulting in two different bugs:
- With a certificate validator, the PeekingPeerConnector class calls
back FwdState, which calls the ConnStateData class, which then tries
secure the connection with the already tunneled SSL client and
closes the connection on negotiating errors.
- Without a certificate validator, the PeekingPeerConnector class
never calls FwdState class, and both PeekingPeerConnector and
FwdState objects stall until finishing tunnelState closes server
and client connections.
Now, PeerConnector always calls FwdState back, marking spliced
connections as such. This has the following positive side-effects:
- When FwdState learns about spliced connections, it does not call
ConnStateData back. Instead, it terminates and gets destroyed.
The tunnel continues uninterrupted.
- The PeekingPeerConnector job ends and is destroyed instead of
waiting to call FwdState.
Fix external_acl problems after trunk r14351
(Support logformat %macros in external_acl_type format).
The above changes created the following problems:
- external_acl requires AccessLogEntry but ALE is not available
in many cases such as ssl_bump ACLs.
- The %<cert_subject stopped working because it was supported by
external_acl code and not by logformat code.
This patch:
- Passes AccessLogEntry in most cases.
For example, PeerConnector-related classes are now covered.
- Implements the %<cert_subject formating code for logformat.
Amos Jeffries [Sun, 31 Jan 2016 12:05:30 +0000 (01:05 +1300)]
SourceLayout: rename ClientSocketContext to Http::Stream
... and provided through http/libsquid-http.la.
The name is chosen to match the RFC7540 HTTP/2 "stream" terminology.
Which defines a stream as a bi-directional transaction, including request,
reply and all related 1xx informational and/or control messages.
That same word "stream" is also used in RFC7230 briefly to describe the
same "transaction" scope and details. But not formalized until RFC7540.
Http::Stream's may be initiated by a client HTTP request, Squid internally,
or in HTTP/2 a server PUSH_PROMISE frame.
There are no logic changes in this. Just symbol renaming and move.
Amos Jeffries [Fri, 29 Jan 2016 12:49:28 +0000 (01:49 +1300)]
SourceLayout: rename cert_valid.pl to security_fake_certverify
* creates src/security/cert_validators/ for certificate validation helpers.
To distinguish from certificate generator helpers which would be in
src/security/cert_generators/.
* renames cert_valid.pl to securiy_fake_certverify inline with the helper
naming schema.
* moves helpers/ssl/ to src/security/cert_validators/fake/ as it is the fake
helper.
- building the man(8) documentation that was missing previously.
* adds a ./configure option --enable-security-cert-validator-helpers= to allow
the bundled certverify helper(s) to be managed at build time just like any other.
- this involves addition of the modules.m4, requires.m4 and Makefile.am
infrastructire that helpers/ssl/ was previously lacking.
Alex Rousskov [Thu, 28 Jan 2016 01:49:55 +0000 (18:49 -0700)]
Fixed cleanup of a shared memory segment in an unusual configuration.
Squid was not removing squid-squid-page-pool.shm at exit when started
in non-daemon mode (-N). That segment is used in non-SMP mode if
memory_cache_shared is explicitly set to "on" (an unusual
configuration primarily useful for testing).
Markus Mayer [Thu, 28 Jan 2016 01:30:37 +0000 (18:30 -0700)]
Fixed handling of shared memory left over by Squid crashes or bugs.
A Squid instance may inherit an old shared memory segment from the
previous instance as the result of either a Squid crash or an at-exit
cleanup bug. This change fixes two problems triggered by old segments:
1. After an earlier OS X fix (bug 3805; trunk r13947), Squid stopped
initializing previously used shared memory. Uninitialzed memory
resulted in subtle bugs and crashes.
2. When called for an old Squid shared memory segment, OS X
ftruncate() fails with EINVAL, preventing Squid from starting when
the old segment is still around.
More specifically: Darwin ftruncate() calls pshm_truncate().
pshm_truncate() checks if the PSHM_ALLOCATED flag is already set on
the memory region. If the flag is set, the call fails with EINVAL.
Otherwise, pshm_truncate() sets PSHM_ALLOCATED for the region.
Since Squid must call ftruncate() to size every new segment, all
old Squid segments have that flag set, preventing ftruncate() calls
for old segments in newer Sqid instances from succeeding.
Amos Jeffries [Tue, 26 Jan 2016 21:02:00 +0000 (10:02 +1300)]
Cleanup: update TLS session pointer types
* rename SSL* pointer to Security::SessionPtr and SSL_Pointer to
SessionPointer as the smart pointer variant. Matching the model
designed for TLS context storage.
* update fd_table .ssl member to a SessionPointer for safer session
pointer deallocation.
* migrate most uses of SSL* to Securit::SessionPtr or auto
Invalid FTP connection handling on blocked content.
FTP client gets stuck after the following chain of events:
* Client requests a file that will be blocked by ICAP.
* Squid starts downloading the file from the FTP server
and sends "150 Opening..." to the FTP client.
* Squid aborts the data connection with the FTP server
as soon as the ICAP service blocks it.
* Squid sends "451 Forbidden" to the FTP client.
* The FTP server sends "500 OOPS: setsockopt: linger" to Squid.
* Squid terminates the control connection to the FTP server.
* Squid establishes a new control connection to the FTP server
but does not authenticate itself.
* Further commands from the FTP client do not work any more.
The above and many similar problems exist because Squid handles
FTP client-to-squid and squid-to-FTP server data connections
independently from each other. In many cases, one connection does
not get notified about the problems with the other connection.
This patch:
- Add Ftp::MasterState::userDataDone to record received
the FTP client final response status code to sent (or to be send)
to the client.
- The Ftp::MasterState::waitForOriginData flag to hold status of the
squid-to-server side. If the squid-to-server side is not finishes
yet this is true.
- Send a control reply to the FTP client only after the data transfered
on both server and client sides.
- Split Client::abortTransaction to Client::abortOnData and to
Client::abortAll()
- Implement the Ftp::Relay::abortOnData() and Ftp::Relay::Abort()
(i.e., StoreEntry abort handler) to avoid closing the control
connection when the data connection is closed unexpectedly.
Make %<a and %<p details available to [eCAP] RESPMOD services
... via adaptation_meta.
This patch fills access log entry with server connection
details as soon as possible. Previously, ALE was updated only
in prepareLogWithRequestDetails() when adaptation process has
already finished.
Name-only note ACL stopped matching after trunk r14465 (note -m).
Despite this fix, empty ACLStringData values (also used for myportname,
peername, tag, and other ACLs) must [continue to] match nothing because
Squid ORs acl values. In math, empty disjunction is false.
The note ACL matches name:value entries, not bare values. A valueless
name has a different semantics than an empty list of bare values. It is
(name and value1) or (name and value2)
rather than
name and (value1 or value2)
where an empty value disjunction would have falsified the whole
condition.
Amos Jeffries [Mon, 18 Jan 2016 13:40:42 +0000 (02:40 +1300)]
Tests: Add --action parameter for testing script
This command line option allows automake target to be provided as an
override to the target specified in the test build scripts. With this a
partial test can be run for all specified layers rather than a full
distcheck which can be quite slow.
* disable the use of system CA by default to verify client connection
certificates. Since the use of client certificates is rare.
* no change to verification of upstream server or peer certificates.
Since the use of system CA to sign server certificates is common.
* the new "default-ca" configuration option and its documentation are
updated to make the situation more obvious amongst the other TLS options
changes in Squid-4.
* the action of the sslflags=NO_DEFAULT_CA is already deprecated, so no
change when it is used. On port lines it now merely sets the default.
It may be a good idea to also disable system CA use for cache_peer and
ICAPS connections. For now they are left unchanged.
The new connections_encrypted ACL matches transactions where all HTTP
messages were received over TLS transport connections, including messages
received from ICAP servers.
Some ICAP/eCAP services receive data from unencrypted sources. Some ICAP/eCAP
services are "secure". By default we assume that all eCAP services and all
ICAP services on TLS transport connections are "secure" unless the user
uses the "connection_encryption" option in service configuration line.
Tls flags listed using the [tls-|tls|ssl]flags option are not configured
correctly. In practice always the SSL_FLAG_NO_DEFAULT_CA even if no
selected and all other flags are ingored
projects / thirdparty / squid.git / log
