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.
The following sequence of events triggers this assertion:
- The server sends an 1xx control message.
- http.cc schedules ConnStateData::sendControlMsg call.
- Before sendControlMsg is fired, http.cc detects an error (e.g., I/O
error or timeout) and starts writing the reply to the user.
- The ConnStateData::sendControlMsg is fired, starts writing 1xx, and
hits the "no concurrent writes" assertion.
We could only reproduce this sequence in the lab after changing Squid
code to trigger a timeout at the right moment, but the sequence looks
plausible. Other event sequences might result in the same outcome.
To avoid concurrent writes, Squid now drops the control message if
Http::One::Server detects that a reply is already being written. Also,
ConnStateData delays reply writing until a pending control message write
has been completed.
Amos Jeffries [Sat, 19 Nov 2016 16:06:10 +0000 (05:06 +1300)]
Fix Null pointer dereferences after rev.14913
on_unsupported_protocol with non-HTTP requests occurs without
an HTTP request object. Some ACLs will not be possible to check
but that is not a reason to crash.
Added nil dereference checks for Ftp::Client::ctrl.conn, including:
- Ftp::Client::handlePasvReply() and handleEpsvReply() that dereference
ctrl.conn in DBG_IMPORTANT messages.
- Many functions inside FtpClient.cc and FtpGateway.cc files.
TODO: We need to find a better way to handle nil ctrl.conn. It is only
a matter of time when we forget to add another dereference check or
discover a place we missed during this change.
Also disabled forwarding of EPRT and PORT commands to origin servers.
Squid support for those commands is broken and their forwarding may
cause segfaults (bug #4004). Active FTP is still supported, of course.
Amos Jeffries [Mon, 7 Nov 2016 10:49:37 +0000 (23:49 +1300)]
Bug 4599 pt1: initial support for OpenSSL v1.1
Converts some CRYPTO_add(..., CRYPTO_LOCK_X509) calls with portability
wrapper for X509_up_ref(). Just the calls which are in code not yet using
Security::CertPointer.
Alex Rousskov [Fri, 4 Nov 2016 22:04:34 +0000 (16:04 -0600)]
Improved testAppendSBuf to actually test SBuf::append() optimization.
... rather than to test SBuf::id preservation across assignments
[currently used by that optimization].
Trunk r14911 talks about bitwise object copy but making sure that SBuf
has a working assignment operator is an issue unrelated to append() and
its optimization. Besides, it is not really the prohibited by C++
bitwise copy that we might worry about but the default member-wise
assignment that we may forget to customize.
Alex Rousskov [Fri, 4 Nov 2016 21:14:54 +0000 (15:14 -0600)]
Honor SBufReservationRequirements::minSize regardless of idealSize.
In a fully specified SBufReservationRequirements, idealSize would
naturally match or exceed minSize. However, the idealSize default value
(zero) may not. We should honor minSize regardless of idealSize, just as
the API documentation promises to do.
No runtime changes expected right now because the only existing user of
SBufReservationRequirements sets .idealSize to CLIENT_REQ_BUF_SZ (4096)
and .minSize to 1024.
The pinned_connection->stopPinnedConnectionMonitoring() call does not needed,
it is already called from inside pinned_connection->borrowPinnedConnection
called two lines before.
Support tunneling of bumped non-HTTP traffic. Other SslBump fixes.
Use case: Skype groups appear to use TLS-encrypted MSNP protocol instead
of HTTPS. This change allows Squid admins using SslBump to tunnel Skype
groups and similar non-HTTP traffic bytes via "on_unsupported_protocol
tunnel all". Previously, the combination resulted in encrypted HTTP 400
(Bad Request) messages sent to the client (that does not speak HTTP).
Also this patch:
* fixes bug 4529: !EBIT_TEST(entry->flags, ENTRY_FWD_HDR_WAIT)
assertion in FwdState.cc.
* when splicing transparent connections during SslBump step1, avoid
access-logging an extra record and log %ssl::bump_mode as the expected
"splice" not "none".
* handles an XXX comment inside clientTunnelOnError for possible memory
leak of client streams related objects
* fixes TunnelStateData logging in the case of splicing after peek.
Alex Rousskov [Thu, 3 Nov 2016 13:57:02 +0000 (02:57 +1300)]
Optimize appending to empty SBufs, a common use case.
Most appends start with an empty buffer. Some append only once. There
is no reason to allocate new memory for that first append or force the
appender to write code to optimize that first append.
Why also check that SBuf has never been configured or altered then? To
preserve pre-allocated SBufs, such as those [ab]used as I/O buffers.
Amos Jeffries [Fri, 28 Oct 2016 08:41:57 +0000 (21:41 +1300)]
Fix GCC 4.9 build after rev.14893
GCC 4.9 does not support some corner cases of C++11 syntax. In this case
when it is presented with default nulptr initialization of function
pointers it gets confused with 'pure virtual' initialization:
error: invalid pure specifier (only '= 0' is allowed) before ';' token
ssl::server_name ACL badly broken since inception (trunk r14008).
The original server_name code mishandled all SNI checks and some rare
host checks:
* The SNI-derived value was pointing to an already freed memory storage.
* Missing host-derived values were not detected (host() is never nil).
* Mismatches were re-checked with an undocumented "none" value
instead of being treated as mismatches.
Same for ssl::server_name_regex.
Also set SNI for more server-first and client-first transactions.
Amos Jeffries [Fri, 28 Oct 2016 07:56:00 +0000 (20:56 +1300)]
HTTP: initial support for Cache-Control:immutable
The 'immutable' cache control is a proposed standardization of
behaviour equivalent to the refresh_pattern 'ignore-reload' option.
Reducing latency delays from revalidation when an object is known not
to be updated until it expires.
When a server emits it on responses a recipient cache is expected to
treat the response as not requiring any revalidation until it becomes
stale. For the duration of its freshness period it may be used as-is
to respond to client requests - including reload (CC:max-age=0)
requests.
This initial code does not yet support preventing IMS conditional
requests behaviour when the 'immutable' signal is received.
Note that CC:no-cache in requests, and If-None-Match ETag based
conditionals can still make an 'immutable' response be irrelevant to
a particular client request. So server contact is not completely
prevented.
Use 'immutable' option in preference over the refresh_pattern
'ignore-reload' in order to track whether that override is still
relevant.
Amos Jeffries [Tue, 25 Oct 2016 08:03:32 +0000 (21:03 +1300)]
HTTP/1.1: make Vary:* objects cacheable
Under new clauses from RFC 7231 section 7.1.4 and HTTP response
containing header Vary:* (wifcard variant) can be cached, but
requires revalidation with server before each use.
Use the new mandatory revalidation flags to allow storing of any
wildcard Vary:* response.
Note that responses with headers like Vary:A,B,C,* are equivalent
to Vary:*. The cache key string for these objects is normalized.
Amos Jeffries [Mon, 24 Oct 2016 12:46:35 +0000 (01:46 +1300)]
Cleanup: convert AclDenyInfoList and AclNameList to MEMPROXY_CLASS
Use the C++ new/delete operators provided by MEMPROXY_CLASS and
associated as-needed pool creation instead of C Alloc/Free
functions and pre-initialized pool.
Remove now unused MEM_ACL_DENY_INFO_LIST and MEM_ACL_NAME_LIST
memory pools.
Remove apparently unused DenyInfoList global.
Fixes a memory leak on shutdown/reconfigure when
aclDestroyDenyInfoList() frees a list of AclDenyInfoList object
but not the AclNameList objects attached to them.
Fixes a memory leak when reconfigure frees a list of AclDenyInfoList
objects but not the URL/page-name strings attached to them.
Squid crashed because HttpMsg::body_pipe was used without check that it
was initialized. The message lacks body pipe when it has no body or
empty body.
projects / thirdparty / squid.git / log
