Alex Rousskov [Tue, 2 Jan 2018 16:16:52 +0000 (09:16 -0700)]
Moved peer*(ps_state) functions into ps_state renamed PeerSelector (#113)
No functionality changes intended (other than debug message variations).
Also polished related documentation and debug messages.
Also converted "struct _icp_common_t" into an icp_common_t class, to
make its forward declarations simple. As a side effect, removed
__cplusplus ifdefs, addressing an old TODO.
Alex Rousskov [Thu, 28 Dec 2017 16:35:32 +0000 (09:35 -0700)]
Bug 2378: Duplicates in selected peer destinations (#112)
Duplicates in FwdServers lead to excessive peer connection retries, skew
in round-robin peer selection, and probably other problems.
This bug was fixed in 2008 but that v2 fix was never ported to v3. This
fix includes a bug 2408 fix for the original (bug 2378) fix, although I
adjusted bug 2408 logic to explicitly reject duplicate PINNED
destinations and to clarify why PINNED connection handling is "special".
I also centralized and improved peerAddFwdServer-related debugging,
removing duplicated and slightly inconsistent code.
Amos Jeffries [Fri, 15 Dec 2017 02:50:53 +0000 (15:50 +1300)]
Convert Acl::InnerNode to C++11 for-each loop (#101)
This also fixes a bug in some STL implementations where passing for_each &ACL::prepareForUse
results in the ACL class nil-method explicitly running instead of the child ACL class
virtual method.
Squid FTP server dying because of an unhandled exception. (#102)
Related message in cache.log:
FATAL: Dying from an exception handling failure; exception: reply
Unfortunately, Squid does not report the exact place where the exception
was thrown, however the most possible reason is a "Must(reply)" failure inside
Ftp::Server::writeErrorReply.
Do not download remote certificate for issuer X if the received server
certificate (signed by X) can be validated using a locally available CA
certificate. According to our tests, a typical browser does not follow
'CA Issuers' references to download 'missing' certificates when the
browser can validate the origin server certificate (or its chain) using
a local CA certificate. Avoiding unnecessary validations and downloads
not only saves time, but can prevent validation failures as well!
Alex Rousskov [Fri, 17 Nov 2017 16:30:09 +0000 (09:30 -0700)]
Relay peer CONNECT error status line and headers to users (#80)
Automated agents and human users (or their support staff!) often benefit
from knowing what went wrong. Dropping such details is a bad default.
For example, automation may rely on receiving the original status code.
Our CVE-2015-5400 fix (74f35ca) was too aggressive -- it hid all peer
errors behind a generic 502 (Bad Gateway) response. Pass-through peer
authentication errors were later (971003b) exposed again, but our CVE
fix intent was _not_ to hide _any_ peer errors in the first place! The
intent was to close the connection after delivering the error response.
Hiding peer errors was an (unfortunate) implementation choice.
It could be argued that some peer errors should not be relayed, but
since Squid successfully relayed all peer errors prior to 74f35ca and
continues to relay all non-CONNECT peer errors today, discriminating
peer errors is a separate (and possibly unnecessary) feature.
Ideally, Squid should mangle and relay the whole error message (instead
of sending small original headers). Squid should also relay 1xx control
messages while waiting for the final response. Unfortunately, doing so
properly, without reopening CVE-2015-5400 or duplicating a lot of
complex code, is a huge project. This small change fixes the most acute
manifestation of the "hiding errors from users" problem. The rest is a
long-term TODO.
This change started as a %<Hs fix described in the first bullet but
subsequent testing exposed the bug described in the second bullet,
fixing which resulted in other related fixes/improvements:
* Fixed %<Hs for received CONNECT errors: Correctly store the response
status code for %<Hs logging (instead of misplacing it in the %>Hs
location that was later overwritten with the correct to-client value).
* Fixed %<pt and %<tt for received CONNECT errors: Squid tunneling code
was missing message I/O timing maintenance required for those %codes.
* Probably fixed %<bs logging bug on forwarding retries: Squid did not
clear the (bodyBytesRead) counter between retries.
* Possibly improved %<Hs, %<pt, %<tt, %<bs logging on SslBump errors:
request->hier member did not copy all the sslServerBump->request->hier
details when generating an error response in
ConnStateData::serveDelayedError().
* Probably fixed a server.all.kbytes_out and server.other.kbytes_out
statistics bug: Squid missed written CONNECT request header bytes.
Also improved HierarchyLogEntry-related code: Reduced code duplication,
removed unnecessary destructor, and described the class. Removed
peer_http_request_sent timestamp because it ought to be equal to the
last request write time (now peer_last_write_).
TODO: Relay (expected) peer CONNECT error responses to users (instead of
hiding them behind a Squid-generated ERR_CONNECT_FAIL) and support %<h.
Bug 2821: Ignore Content-Range in non-206 responses (#77)
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.
Amos Jeffries [Fri, 3 Nov 2017 05:38:40 +0000 (18:38 +1300)]
Bug 4679: User names not sent to url_rewrite_program (#78)
Add accessors to AccessLogEntry for retrieving IDENT and External ACL user
labels in a consistent way. Use these accessors for all log and logformat
outputs.
NP: does not hide/remove the original cache.* members due to direct use
remaining in some code locations that cannot yet be avoided.
Amos Jeffries [Thu, 2 Nov 2017 08:14:54 +0000 (21:14 +1300)]
Move TLS/SSL http_port config values to libsecurity (#51)
These are most of the minor shuffling prerequisite for the proposal to allow generate-host-certificates to set a CA filename. These are required in libsecurity in order to prevent circular dependencies between libsecurity, libssl and libanyp.
Also contains some improvements to how configuration errors are displayed for these affected settings and some bugs fixed where the configured values were handled incorrectly.
Do not check 'configured_once' in mainInitialize(). (#74)
Since mainInitialize() is called only once, there is no need to do this
check. I assume the check became obsolete since trunk r212: before this
revision, mainInitialize() could be called twice from main() with a
similar 'first_time' variable protection.
Fixed reporting of validation errors for downloaded intermediate certs. (#72)
When Squid or its helper could not validate a downloaded intermediate
certificate (or the root certificate), Squid error page contained
'[Not available]' instead of the broken certificate details, and '-1'
instead of depth of the broken certificate in logs.
Bug 4718: Support filling raw buffer space of shared SBufs (#64)
SBuf::forceSize() requires exclusive SBuf ownership but its precursor
SBuf::rawSpace() method does not guarantee exclusivity. The pair of
calls may result in SBuf::forceSize() throwing for no good reason.
New SBuf API provides a new pair of raw buffer appending calls that
reduces the number of false negatives.
This change may alleviate bug 4718 symptoms but does not address its
core problem (which is still unconfirmed).
This bug was probably caused by Bug 2833 feature/fix (1a210de).
The primary fix here is limited to clientReplyContext::processExpired():
Collapsed forwarding code must ensure StoreEntry::mem_obj existence. It
was missing for cache hits purged from (or never admitted into) the
memory cache. Most storeClientListAdd() callers either have similar code
or call storeCreateEntry() which also creates StoreEntry::mem_obj.
Also avoided clobbering known StoreEntry URIs/method in some cases. The
known effect of this change is fixed store.log URI and method fields
when a hit transaction did not match the stored entry exactly (e.g., a
HEAD hit for a GET cached entry), but this improvement may have even
more important consequences: The original method is used by possibly
still-running entry filling code (e.g., determining the end of the
incoming response, validating the entry length, finding vary markers,
etc.). Changing the method affects those actions, essentially corrupting
the entry state. The same argument may apply to store ID and log URI.
We even tried to make URIs/method constant, but that is impractical w/o
addressing an XXX in MemStore::get(), which is outside this issue scope.
To facilitate that future fix, the code now distinguishes these cases:
* createMemObject(void): Buggy callers that create a new memory object
but do not know what URIs/method the hosting StoreEntry was based on.
Once these callers are fixed, we can make the URIs/method constant.
* createMemObject(trio): Callers that create a new memory object with
URIs/method that match the hosting StoreEntry.
* ensureMemObject(trio): Callers that are not sure whether StoreEntry
has a memory object but have URIs/method to create one if needed.
Fix SSL certificate cache refresh and collision handling (#40)
SslBump was ignoring some origin server certificate changes or differences,
incorrectly using the previously cached fake certificate (mimicking
now-stale properties or properties of a slightly different certificate).
Also, Squid was not detecting key collisions inside certificate caches.
On-disk certificate cache fixes:
Use the original certificate signature instead of the certificate
subject as part of the key. Using signatures reduces certificate key
collisions to deliberate attacks and woefully misconfigured origins,
and makes any mishandled attacks a lot less dangerous because the
attacking origin server certificate cannot by trusted by a properly
configured Squid and cannot be used for encryption by an attacker.
We have considered using certificate digests instead of signatures.
Digests would further reduce the attack surface to copies of public
certificates (as if the origin server was woefully misconfigured).
However, unlike the origin-supplied signatures, digests require
(expensive) computation in Squid, and implemented collision handling
should make any signature-based attacks unappealing. Signatures won
on performance grounds.
Other key components remain the same: NotValidAfter, NotValidBefore,
forced common name, non-default signing algorithm, and signing hash.
Store the original server certificate in the cache (together with
the generated certificate) for reliable key collision detection.
Upon detecting key collisions, ignore and replace the existing cache
entry with a freshly computed one. This change is required to
prevent an attacker from tricking Squid into hitting a cached
impersonating certificate when talking to a legitimate origin.
In-memory SSL context cache fixes:
Use the original server certificate (in ASN.1 form) as a part of the
cache key, to completely eliminate cache key collisions.
Other related improvements:
Make the LruMap keys template parameters.
Polish Ssl::CertificateDb class member names to match Squid coding
style. Rename some functions parameters to better match their
meaning.
Replace Ssl::CertificateProperties::dbKey() with:
Ssl::OnDiskCertificateDbKey() in ssl/gadgets.cc for
on-disk key generation by the ssl_crtd helper;
Ssl::InRamCertificateDbKey() in ssl/support.cc for
in-memory binary keys generation by the SSL context memory cache.
Optimization: Added Ssl::BIO_new_SBuf(SBuf*) for OpenSSL to write
directly into SBuf objects.
Alex Rousskov [Tue, 22 Aug 2017 01:09:23 +0000 (19:09 -0600)]
Do not die silently when dying early. (#43)
Report (to stderr) various problems (e.g., unhandled exceptions) that
may occur very early in Squid lifetime, before stderr-logging is forced
by SquidMain() and way before proper logging is configured by the first
_db_init() call.
To enable such early reporting, we started with a trivial change:
-FILE *debug_log = NULL;
+FILE *debug_log = stderr;
... but realized that debug_log may not be assigned early enough! The
resulting (larger) changes ensure that we can log (to stderr if
necessary) as soon as stderr itself is initialized. They also polish
related logging code, including localization of stderr checks and
elimination of double-closure during log rotation on Windows.
These reporting changes do not bypass or eliminate any failures.
r15240 broke ipcacheCheckNumeric() because that function's static
storage was no longer reset properly between calls.
This bug is very different from bug 4741, but their symptoms (e.g.,
false "Host header forgery" SECURITY ALERTs) can be the same.
I did not realize that std::vector::resize(n, x) ignores x when the
vector size is already at least n. It is not a reset()-like method. My
tests did not have enough different IP-based URLs to expose this bug.
Security::HandshakeParser::parseServerCertificates builds cert list with nils (#42)
... if squid does not compiled with OpenSSL support.
This patch fixes:
* HandshakeParser::ParseCertificate() to return a Security::Pointer
* HandshakeParser:: parseServerCertificates() to be a no-op if OpenSSL is
not used
* Fix compile error if squid compiled without openssl but with gnutls enabled
Alex Rousskov [Sun, 6 Aug 2017 00:20:40 +0000 (18:20 -0600)]
Fixed, changed addresses in README. Made README look better on Github.
Why not add README.md? Not enough reasons to warrant info duplication:
Markdown is not particularly helpful for rendering a trivial list of
references, and Github already renders HTTP links appropriately.
Why not move README to README.md? Many tools and console humans still
look for README rather than README.md.
Why not use Markdown in README? Github does not render such markup.
TODO: Consider removing detailed distribution terms at the bottom
because "everybody" knows what GPLv2 basically means, and we already
tell the reader where to find the exact licensing terms.
Alex Rousskov [Wed, 2 Aug 2017 22:13:27 +0000 (16:13 -0600)]
Automatically revive hopeless kids on reconfigure and after a timeout.
Squid declares kids with "repeated, frequent failures" as hopeless.
Hopeless kids were not automatically restarted. In the absence of
automated recovery, admins were forced to restart the whole Squid
instance (after fixing the underlying problem that led to those kid
failures). Squid instance restarts hurt users.
In many cases, the underlying kid-killing problem is temporary, and
Squid can eventually fully recover without any admin involvement.
Squid now automatically restarts a hopeless kid after a configurable
downtime (a new hopeless_kid_revival_delay directive with a 60 minute
default value).
Also restart all hopeless kids upon receiving a reconfiguration signal.
Also avoid sending signals to non-running kids, fixing an old minor bug.
Garri Djavadyan [Tue, 1 Aug 2017 00:03:18 +0000 (18:03 -0600)]
Bug 4648: Squid ignores object revalidation for HTTPS scheme
Squid skips object revalidation for HTTPS scheme and, hence, does not
honor a reload_into_ims option (among other settings).
TODO: Add an httpLike() method or function to detect all HTTP-like
schemes instead of comparing with AnyP::PROTO_HTTP directly. There are
20+ candidates for similar bugs: git grep '[!=]= AnyP::PROTO_HTTP'.
* 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
Alex Rousskov [Thu, 20 Jul 2017 03:45:59 +0000 (21:45 -0600)]
Made Ip::Address::fromHost() handle nil pointers after fd9c47d. (#25)
No functionality changes expected. Nobody was passing nil pointers to
this code before or after fd9c47d AFAICT, but now that this code is
exposed as a public method, it must handle nil pointers.
Detected by Coverity Scan. Issue 1415049 (FORWARD_NULL).
Alex Rousskov [Wed, 12 Jul 2017 05:04:41 +0000 (23:04 -0600)]
Happy Eyeballs: Deliver DNS resolution results to peer selection ASAP.
To make eyeballs happy, DNS code must deliver each lookup result to the
IP cache and, ultimately, to upper layers of ipcache_nbgethostbyname()
callers. This requires changing two interfaces:
1. between the DNS and the IP cache (the IDNSCB callback);
2. between the IP cache and peer selection code (the IPH callback).
The IDNSCB callback is now called after every usable A and AAAA lookup
instead of waiting for both answers. The IPH callback now has a sister
API for incremental delivery: The Dns::IpReceiver class.
To safely handle incremental delivery of IP addresses to the IP cache, I
upgraded ipcache_addrs from an equivalent of a C POD to a C++ CachedIps
container. The encapsulation allowed me to clearly separate the two IP
cache iteration APIs:
* All IPs (used by, e.g., ACL matching and host verification code) and
* just the "good" IPs (used only for peer selection for now).
CachedIps stores IPs together with their good/bad status in a single
std::vector. Eventually, the CachedIp element may be extended to store
TTL. The following implementation alternatives were considered and
rejected (at least for now) while optimizing for the "a few (and usually
just one), usually good IPs" case:
* Using std::list or std::deque storage would consume more RAM[1] for
the common case of one (or few) IPs per name and slowed down IPs
iteration code.
[1] http://info.prelert.com/blog/stl-container-memory-usage
* Separating IP from its status, like the old code did, would make it
easier to mismatch IP and its status, make it harder to add more
metadata like per-IP TTL, and only save memory when storing many IPs
per name.
The drawback of the selected "all IP-related info in one place" approach
is that we need smart iterators (e.g., the added GoodIpsIterator) or a
visitor API.
I added a new interface class for the incremental notification about
newly found IP addresses (Dns::IpReceiver) instead of adding second
IPH-like function pointer because we cannot safely call cbdata-protected
functions multiple times for the same cbdata object -- only
cbdataReferenceValidDone() dereferences the opaque pointer properly, and
that function cannot be called repeatedly. CbcPointer solves that
problem (but requires a class). Class methods also allow for more
precise notifications, with fewer ifs in the recipient code.
The new IpCacheLookupForwarder class hides the differences between the
old C-style IPH callbacks and the new Dns::IpReceiver. Eventually, we
may be able to move all lookup-specific data/methods into
IpCacheLookupForwarder, away from the IP cache entries where that info
is useless at best.
mgr:ipcache no longer reports "IPcache Entries In Use" but that info is
now available as "cbdata ipcache_entry" row in mgr:mem.
Do not cache IPv6 /etc/hosts addresses when IPv6 support is disabled.
This change simplified code, made it more consistent (we did not cache
AAAA records), and fixed ipcacheCycleAddr() and ipcacheMarkAllGood()
that were clearing supposed-to-be-permanent "bad (IPv6 disabled)" marks.
Also fixed two DNS TTL bugs. Squid now uses minimum TTL among all used
DNS records[2]. Old ipcacheParse() was trying to do the same but:
* could overwrite a zero TTL with a positive value
* took into account TTLs from unused record types (e.g., CNAME).
[2] Subject to *_dns_ttl limits in squid.conf, as before.
Also fixed "delete xstrdup" (i.e., malloc()ed) pointer in bracketed IP
parsing code (now moved to Ip::Address::FromHost()).
Also prohibited duplicate addresses from entering the IP cache. Allowing
duplicates may be useful for various hacks, but the IP cache code
assumes that cached IPs are unique and fails to mark bad repeated IPs.
Also fixed sending Squid Announcements to unsupported/disabled IPv6
addresses discovered via /etc/hosts.
Also slightly optimized dstdomain when dealing with IP-based host names:
The code now skips unnecessary Ip::Address to ipcache_addrs conversion.
This simplification may also help remove the ipcacheCheckNumeric() hack.
The bracketed IP parsing code was moved to Ip::Address::fromHost(). It
still needs a lot of love.
Bug 1961 extra: Convert the URL::parse method API to take const URI strings
The input buffer is no longer truncated when overly long. All callers have
been checked that they handle the bool false return value in ways that do
not rely on that truncation.
Callers that were making non-const copies of buffers specifically for the
parsing stage are altered not to do so. This allows a few data copies and
allocations to be removed entirely, or delayed to remove from error handling
paths.
While checking all the callers of Http::FromUrl several places were found to
be using the "raw" URL string before the parsing and validation was done. The
simplest in src/mime.cc is already applied to v5 r15234. A more complicated
redesign in src/store_digest.cc is included here for review. One other marked
with an "XXX: polluting ..." note.
Also, added several TODO's to mark code where class URL needs to be used when
the parser is a bit more efficient.
Also, removed a leftover definition of already removed urlParse() function.
* 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.
Amos Jeffries [Mon, 26 Jun 2017 02:14:42 +0000 (14:14 +1200)]
Bug 1961 partial: move urlParse() into class URL
* rename local variables in urlParse() to avoid symbol
clashes with class URL members and methods.
* move HttpRequest method assignment out to the single caller
which actually needed it. Others all passed in the method
which was already set on the HttpRequest object passed.
* removed now needless HttpRequest parameter of urlParse()
* rename urlParse as a class URL method
* make URL::parseFinish() private
* remove unnecessary CONNECT_PORT define
* add RFC documentation for 'CONNECT' URI handling
* fixed two XXX in URL-rewrite handling doing unnecessary
HttpRequest object creation and destruction cycles on
invalid URL-rewrite helper output.
Alex Rousskov [Mon, 26 Jun 2017 00:10:34 +0000 (18:10 -0600)]
Minimize direct comparisons with ACCESS_ALLOWED and ACCESS_DENIED.
No functionality changes expected.
Added allow_t API to avoid direct comparisons with ACCESS_ALLOWED and
ACCESS_DENIED. Developers using direct comparisons eventually mishandle
exceptional ACCESS_DUNNO and ACCESS_AUTH_REQUIRED cases where neither
"allow" nor "deny" rule matched. The new API cannot fully prevent such
bugs, but should either led the developer to the right choice (usually
.allowed()) or alert the reviewer about an unusual choice (i.e.,
denied()).
The vast majority of checks use allowed(), but we could not eliminate
the remaining denied() cases ("miss_access" and "cache" directives) for
backward compatibility reasons -- previously "working" deployments may
suddenly start blocking cache misses and/or stop caching:
http://lists.squid-cache.org/pipermail/squid-dev/2017-May/008576.html
Alex Rousskov [Sat, 24 Jun 2017 00:01:51 +0000 (18:01 -0600)]
Fixed 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.
Andreas Weigel [Sun, 18 Jun 2017 14:26:55 +0000 (02:26 +1200)]
Fix option --foreground to implement expected behavior
... and allow usage of SMP mode with service supervisors that do not work
well with daemons.
Currently, --foreground behavior is counter-intuitive in that the launched
process, while staying in the foreground, forks another "master" process,
which will create additional children (kids), depending on the number of
configured workers/diskers.
Furthermore, sending a SIGINT/SIGTERM signal to this foreground process
terminates it, but leaves all the children running.
The behavior got introduced with v4 rev.14561.
From discussion on squid-dev, the following behavior is implemented:
* -N: The initial process is a master and a worker process.
No kids.
No daemonimization.
* --foreground: The initial process is the master process.
One or more worker kids (depending on workers=N).
No daemonimization.
* neither: The initial process double-forks the master process.
One or more worker kids (depending on workers=N).
Daemonimization.
The Release Notes for v4 were updated to reflect the corrected behavior.
projects / thirdparty / squid.git / log
