Bug 2821: Ignore Content-Range in non-206 responses (#250)
Squid used to honor Content-Range header in HTTP 200 OK (and possibly
other non-206) responses, truncating (and possibly enlarging) some
response bodies. RFC 7233 declares Content-Range meaningless for
standard HTTP status codes other than 206 and 416. Squid now relays
meaningless Content-Range as is, without using its value.
Why not just strip a meaningless Content-Range header? Squid does not
really know whether it is the status code or the header that is "wrong".
Let the client figure it out while the server remains responsible.
Also ignore Content-Range in 416 (Range Not Satisfiable) responses
because that header does not apply to the response body.
Also fixed body corruption of (unlikely) multipart 206 responses to
single-part Range requests. Valid multipart responses carry no
Content-Range (in the primary header), which confused Squid.
The fixed leak was accompanied by these cache.log errors:
ERROR: worker I/O push queue for ... overflow: ...
I/O queue overflows during disk read requests log the same error but do
not leak memory. Repeated overflows during disk write requests could
eventually exhaust IPC shared memory:
ERROR: ... exception: run out of shared memory pages for IPC I/O
With IPC memory exhausted due to leaks, rock disk I/O stops forever.
Amos Jeffries [Sat, 20 Jan 2018 04:54:16 +0000 (17:54 +1300)]
Fixed Ip::Address copying (#126)
Explicit copy construction was slow and unnecessary.
Explicit copy assignment mishandled self copying and was unnecessary.
The remaining memcpy() calls mishandled self copying.
There are no known cases of Ip::Address self copying.
Fix 889fc47 for SSL bumping with an authentication type other than the Basic (#104)
Commit 889fc47 was made to fix issue with Basic authentication and SSL bumping. But after this commit we can no longer properly use http_access with proxy_auth/proxy_auth_regex ACL because that type of ACL always return 1(match) regardless of the conditions in the rules.
Use the caches authentication results (if any) instead of a fixed 1(match) result.
Alexander Gozman [Fri, 16 Feb 2018 10:52:58 +0000 (13:52 +0300)]
Fix clientside_mark and client port logging in TPROXY mode (#150)
The clientside_mark ACL was not working with TPROXY because a
conntrack query could not find connmark without a true client port.
Ip::Intercept::Lookup() must return true client address, but its
TproxyTransparent() component was reseting the client port. We should
use zero port when we compute the source address for the Squid-to-peer
connection instead.
squidadm [Sat, 19 Aug 2017 18:48:49 +0000 (06:48 +1200)]
3.5.27 (#49)
* Prep for 3.5.27 (#48)
* Maintenance: update snapshot script for git (#24)
* Update snapshot script after git migration
- Remove unused BZRROOT environment variable
- Replace tag with branch name
* Update source-maintenance script for git (#26)
* replace bzr calls with git equivalent
* remove obsolete ROOT and PWD variables (git does not support non-recursive file listing)
* add exceptions to ignore more files caught by git than bzr
* Protect Squid Client classes from new requests that compete with
ongoing pinned connection use and
* resume dealing with new requests when those Client classes are done
using the pinned connection.
Replaced primary ConnStateData::pinConnection() calls with a pair of
pinBusyConnection() and notePinnedConnectionBecameIdle() calls,
depending on the pinned connection state ("busy" or "idle").
Removed pinConnection() parameters that were not longer used or could be computed from the remaining parameters.
Removed ConnStateData::httpsPeeked() code "hiding" the originating
request and connection peer details while entering the first "idle"
state. The old (trunk r11880.1.6) bump-server-first code used a pair of
NULLs because "Intercepted connections do not have requests at the
connection pinning stage", but that limitation no longer applicable
because Squid always fakes (when intercepting) or parses (a CONNECT)
request now, even during SslBump step1.
The added XXX and TODOs are not directly related to this fix. They
were added to document problems discovered while working on this fix.
In v3.5 code, the same problems manifest as Read.cc
"fd_table[conn->fd].halfClosedReader != NULL" assertions.
Alex Rousskov [Sat, 1 Jul 2017 09:59:16 +0000 (21:59 +1200)]
Fix mgr query handoff from the original recipient to Coordinator.
This bug has already been fixed once, in trunk r11164.1.61, but that fix
was accidentally undone shortly after, during significant cross-branch
merging activity combined with the Forwarder class split. The final
merge importing the associated code (trunk r11730) was buggy.
The bug (explained in r11164.1.61) leads to a race condition between
* Store notifying Server classes about the entry completion (which might
trigger a bogus error message sent to the cache manager client while
Coordinator sends its own valid response on the same connection!) and
* post-cleanup() connection closure handlers of Server classes silently
closing everything (and leaving Coordinator the only responding
process on that shared connection).
The bug probably was not noticed for so long because, evidently, the
latter actions tend to win in the current code.
Amos Jeffries [Thu, 22 Jun 2017 15:31:46 +0000 (03:31 +1200)]
Bug 4671 pt3: various GCC 7 compile errors
Also, 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.
Alex Rousskov [Wed, 21 Jun 2017 20:12:48 +0000 (08:12 +1200)]
Replace new/delete operators using modern C++ rules.
This change was motivated by "Mismatched free()/delete/delete[]" errors
reported by valgrind and mused about in Squid source code.
I speculate that the old new/delete replacement code was the result of
slow accumulation of working hacks to accomodate various environments,
as compiler support for the feature evolved. The cumulative result does
not actually work well (see the above paragraph), and the replacement
functions had the following visible coding problems according to [1,2]:
a) Declared with non-standard profiles that included throw specifiers.
b) Declared inline. C++ says that the results of inline declarations
have unspecified effects. In Squid, they probably necessitated
complex compiler-specific "extern inline" workarounds.
c) Defined in the header file. C++ says that defining replacements "in
any source file" is enough and that multiple replacements per
program (which is what a header file definition produces) result in
"undefined behavior".
d) Declared inconsistently (only 2 out of 4 flavors). Declaring one base
flavor should be sufficient, but if we declare more, we should
declare all of them.
The replacements were not provided to clang (trunk r13219), but there
was no explanation why. This patch does not change that exclusion.
I have no idea whether any of the old hacks are still necessary in some
cases. However, I suspect that either we do not care much if the
replacements are not enabled on some poorly supported platforms OR we
can disable them (or make them work) using much simpler hacks for the
platforms we do care about.
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
Bug 2833 pt3: Do not respond with HTTP/304 to unconditional requests
... after internal revalidation. The original unconditional HttpRequest
was still marked (and processed) as conditional after internal
revalidation because the original (clear) Last-Modified and ETag values
were not restored (cleared) after the internal revalidation abused them.
TODO: Isolate the code converting the request into conditional one _and_
the code that undoes that conversion, to keep both actions in sync.
The security fix in v5 r14979 had a negative effect on collapsed
forwarding. All "private" entries were considered automatically
non-shareable among collapsed clients. However this is not true: there
are many situations when collapsed forwarding should work despite of
"private" entry status: 304/5xx responses are good examples of that.
This patch fixes that by means of a new StoreEntry::shareableWhenPrivate
flag.
The suggested fix is not complete: To cover all possible situations, we
need to decide whether StoreEntry::shareableWhenPrivate is true or not
for all contexts where StoreEntry::setPrivateKey() is used. This patch
fixes only few important cases inside http.cc, making CF (as well
collapsed revalidation) work for some [non-cacheable] response status
codes, including 3xx, 5xx and some others.
The original support for internal revalidation requests collapsing
was in trink r14755 and referred to Squid bugs 2833, 4311, and 4471.
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.
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).
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.
Amos Jeffries [Mon, 29 May 2017 04:38:52 +0000 (16:38 +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.
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 SslBump splicing happens after getting SNI. SNI goes into the
second fake CONNECT request, where it may fail the host forgery check.
A failed check triggers an HTTP error response from Squid. When
attempting to send that response to the TLS client, Squid checks whether
all previously pipelined HTTP requests on the connection have finished.
Prior to this fix, Squid left the first fake CONNECT request in the
connection pipeline despite adding the second fake CONNECT. That first
CONNECT stalled the error response described above, with Squid waiting,
in vain, for that already handled [fake] transaction to finish.
Also call quitAfterError() to force Squid to close the connection (after
writing the discussed error response) instead of just logging a
[misleading] "kick abandoning [connection]" message in cache.log.
TODO: Always pop the first CONNECT when generating a second one.
Unifying CONNECT treatment is difficult because code like tunnel.cc
wants that CONNECT to be in the pipeline. Polishing that would probably
require disassociating ConnStateData from tunnel.cc (at least).
TODO: Apply the existing "delayed error" logic (that optionally bumps
TLS connections to deliver [some] errors to [some] SSL/TLS clients) to
host forgery errors. Otherwise, the plain HTTP error message cannot be
understood by the intercepted TLS client.
Alex Rousskov [Sun, 26 Feb 2017 08:46:24 +0000 (21:46 +1300)]
Fix crash when configuring with invalid delay_parameters restore value.
... like none/none. Introduced in rev 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.
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.
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.
Amos Jeffries [Fri, 27 Jan 2017 15:00:05 +0000 (04:00 +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.
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 [Mon, 26 Dec 2016 02:22:00 +0000 (15:22 +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.
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 [Thu, 15 Dec 2016 09:36:34 +0000 (22:36 +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.
Bug 4004 partial: Fix segfault via Ftp::Client::readControlReply
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.
Alex Rousskov [Mon, 14 Nov 2016 12:40:51 +0000 (01:40 +1300)]
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.
Fix ssl::server_name ACL badly broken since inception.
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 [Sun, 30 Oct 2016 09:45:03 +0000 (22:45 +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.
projects / thirdparty / squid.git / log
