]> git.ipfire.org Git - thirdparty/squid.git/log
thirdparty/squid.git
4 years agoCopyright update for auto-generated files (#537)
squidadm [Mon, 6 Jan 2020 17:30:20 +0000 (17:30 +0000)] 
Copyright update for auto-generated files (#537)

4 years agoFix string truncation in getnameinfo() (#462)
Amos Jeffries [Sun, 5 Jan 2020 13:29:29 +0000 (13:29 +0000)] 
Fix string truncation in getnameinfo() (#462)

strncpy can leave strings unterminated. Use xstrncpy instead
of relying on callers to terminate their buffers.

4 years agoSource Format Enforcement (#532)
squidadm [Thu, 2 Jan 2020 03:06:33 +0000 (03:06 +0000)] 
Source Format Enforcement (#532)

4 years agoDocs: Fix Squid-5 reference in v5 release notes (#530)
DrDaveD [Wed, 1 Jan 2020 23:46:49 +0000 (23:46 +0000)] 
Docs: Fix Squid-5 reference in v5 release notes (#530)

4 years agoBug 5007: Docs: Fix max_filedescriptors description (#529)
DrDaveD [Wed, 1 Jan 2020 22:56:34 +0000 (22:56 +0000)] 
Bug 5007: Docs: Fix max_filedescriptors description (#529)

max_filedescriptors can also be used to _raise_  the number of
descriptors (up to the OS hard limit).

4 years agoBug 4735: Truncated chunked responses cached as whole (#528)
DrDaveD [Mon, 30 Dec 2019 20:43:33 +0000 (20:43 +0000)] 
Bug 4735: Truncated chunked responses cached as whole (#528)

Mark responses received without the last chunk as responses that have
bad (and, hence, unknown) message body length (i.e. ENTRY_BAD_LENGTH).
If they were being cached, such responses will be released and will stop
being shareable.

4 years agoFix server_cert_fingerprint on cert validator-reported errors (#522)
Christos Tsantilas [Wed, 25 Dec 2019 17:21:30 +0000 (17:21 +0000)] 
Fix server_cert_fingerprint on cert validator-reported errors (#522)

The server_cert_fingerprint ACL mismatched when sslproxy_cert_error
directive was applied to validation errors reported by the certificate
validator because the ACL could not find the server certificate.

This is a Measurement Factory project.

4 years agoCentralized PagePool/PageStack ID generation (#525)
Eduard Bagdasaryan [Tue, 24 Dec 2019 18:10:49 +0000 (18:10 +0000)] 
Centralized PagePool/PageStack ID generation (#525)

Easy-to-find-in-cache.log and predictable/stable stack IDs for shared
memory pages and/or index slot numbers are very useful when debugging
cache metadata corruption issues because they allow to track related
(e.g. same-stack) operations across huge SMP logs.

4 years agoFixed prohibitively slow search for new SMP shm pages (#523)
Eduard Bagdasaryan [Mon, 23 Dec 2019 08:54:53 +0000 (08:54 +0000)] 
Fixed prohibitively slow search for new SMP shm pages (#523)

The original Ipc::Mem::PageStack algorithm used an optimistic linear
search to locate the next free page. Measurements showed that, in
certain cases, that search could take seconds on busy caches, iterating
over millions of page index items and effectively stalling all workers
while showing 100% CPU utilization.

The new code uses a deterministic stack. It is still lock-free. The spin
loops around stack head pointer updates are expected to quit after at
most few iterations, even with a large number of workers. These loops do
not have ABA update problems. They are not spin locks.

4 years agoSmarter auth_param utf8 handling, including CP1251 support (#480)
Sergey Kirpa [Mon, 23 Dec 2019 08:01:21 +0000 (08:01 +0000)] 
Smarter auth_param utf8 handling, including CP1251 support (#480)

When forwarding authentication credentials to authentication helpers:

* With auth_param utf8 parameter: Squid assumed that the received
  credentials are encoded with Latin-1 (and re-encoded them with UTF-8).
  This assumption is wrong for Internet Explorer running with CP1251
  regional settings. Now Squid uses HTTP Accept-Language request header
  to guess the received credentials encoding (Latin-1, CP1251, or UTF-8)
  and converts the first two encodings into UTF-8.

* Without auth_param utf8 parameter: No changes. Squid sends credentials
  in their original encoding, only applying RFC 1738 escaping on top.

Chrome and Firefox should not be affected because they always use UTF-8
encoding when sending authentication credentials.

4 years agoFix some compile errors from Windows MinGW (#71)
Amos Jeffries [Sat, 21 Dec 2019 21:14:11 +0000 (21:14 +0000)] 
Fix some compile errors from Windows MinGW (#71)

MinGW compiler is a bit more limited than most GCC or Clang available.
The types used on Windows for some API declarations differ from those
commonly used on Linux/BSD systems.

TODO: There are additional compile issues to resolve.

4 years agoFix FTP buffers handling (#521)
Christos Tsantilas [Fri, 20 Dec 2019 07:29:58 +0000 (07:29 +0000)] 
Fix FTP buffers handling (#521)

Fix the parsing of the received listing from FTP services.
Also relaxed size/filename grammar used for DOS listings: Tolerate
multiple spaces between the size and the filename.

This is a Measurement Factory project

4 years agoFix shared memory size calculation on 64-bit systems (#520)
Eduard Bagdasaryan [Tue, 17 Dec 2019 15:01:05 +0000 (15:01 +0000)] 
Fix shared memory size calculation on 64-bit systems (#520)

Since commit 2253ee0, the wrong type (uint32 instead of size_t) was used
to calculate the PagePool::theLevels size. theLevels memory (positioned
by different and correct code) did not overlap with the raw pages
buffer, but the raw pages buffer could, in some cases, be 32 bits short,
placing the last 4 bytes of the last page outside of allocated memory.

In practice, shared memory allocations are page-aligned, and the
difference in 4 bytes was probably compensated by the extra allocated
bytes in most (or perhaps even all) cases.

4 years agoFix request URL generation in reverse proxy configurations (#519)
Guido Vranken [Thu, 12 Dec 2019 03:27:40 +0000 (03:27 +0000)] 
Fix request URL generation in reverse proxy configurations (#519)

4 years agoSend HTTP/500 (Internal Server Error) when lacking peers (#490)
Eduard Bagdasaryan [Wed, 27 Nov 2019 03:30:34 +0000 (03:30 +0000)] 
Send HTTP/500 (Internal Server Error) when lacking peers (#490)

... instead of sending HTTP/503 (Service Unavailable) in
tunneling cases.

Tunneling code reported 503 while regular forwarding code reported 500
errors under identical "no peers to try" circumstances.

For consistency sake, we need to use one of these codes in both places.
We considered and rejected 503 because RFC7231 limits its usage to
several specific cases, not including 'lacking peers' case.

4 years agoBug 5008: SIGBUS in PagePool::level() with custom rock slot size (#515)
jijiwawa [Sat, 23 Nov 2019 10:24:41 +0000 (10:24 +0000)] 
Bug 5008: SIGBUS in PagePool::level() with custom rock slot size (#515)

SMP Squids were crashing on arm64 due to incorrect memory alignment of
Ipc::Mem::PagePool::theLevels array. The relative position of the array
depends on the number of workers and the number of pages (influenced by
the cache capacity and slot size), so some configurations worked OK.

We have to manually align manually positioned fields inside shared
memory segments. Thankfully, C++11 provides alignment-computing APIs.

4 years agoBug 5009: Build failure with older clang libc++ (#514)
Alex Rousskov [Sat, 23 Nov 2019 09:18:24 +0000 (09:18 +0000)] 
Bug 5009: Build failure with older clang libc++ (#514)

Older clang libc++ implementations correctly reject implicit usage of an
explicit (in C++11) std::map copy constructor with "chosen constructor
is explicit in copy-initialization" errors. The same code becomes legal
in C++14[1], so newer libc++ implementation allow implicit usage (even
in C++11), but there is no need for copy-initialization here at all.
Evidently, libstdc++ has never declared constructors explicit.

The bug was seen with Apple clang in Xcode 5.1.1 (roughly upstream clang
3.4) and Xcode 6.2 (roughly upstream clang 3.5), both using libc++.

[1] http://cplusplus.github.io/LWG/lwg-defects.html#2193

4 years agoDocs: mention --foreground in squid -z man page (#508)
Marcos Mello [Sat, 23 Nov 2019 06:25:47 +0000 (06:25 +0000)] 
Docs: mention --foreground in squid -z man page (#508)

4 years agoext_lm_group_acl: Improved username handling (#512)
aaron-costello [Fri, 22 Nov 2019 02:44:29 +0000 (02:44 +0000)] 
ext_lm_group_acl: Improved username handling (#512)

4 years agoHappy Eyeballs: Do not wait for already exhausted spares (#509)
Eduard Bagdasaryan [Thu, 21 Nov 2019 08:19:47 +0000 (08:19 +0000)] 
Happy Eyeballs: Do not wait for already exhausted spares (#509)

ResolvedPeers::findSpareOrNextPeer() never returned the next peer. That
bug made doneWithSpares() return false when the next peer was present,
even though no spares were expected. The exact effects of this bug are
not known, but wrong false doneWithSpares() outcome may force Squid to
wait for more spare addresses that would never come, possibly slowing
down transactions.

4 years agoFix Security::ServerOptions copy-constructor (#513)
Amos Jeffries [Wed, 20 Nov 2019 05:20:04 +0000 (05:20 +0000)] 
Fix Security::ServerOptions copy-constructor (#513)

clientCaStack uses a std::unique_ptr which cannot be copied.

4 years agoFix detection of sys/sysctl.h detection (#511)
Amos Jeffries [Mon, 18 Nov 2019 12:06:56 +0000 (01:06 +1300)] 
Fix detection of sys/sysctl.h detection (#511)

Make sure we test the EUI specific headers using same flags
chosen for final build operations. This should make the
test detect the header as unavailable if the user options
would make the compiler #warning be a fatal error later.

4 years agoPrep for v4.9 (#505)
Amos Jeffries [Tue, 5 Nov 2019 18:55:13 +0000 (07:55 +1300)] 
Prep for v4.9 (#505)

* Prep for v4.9

* Note last minute changes

4 years agocachemgr.cgi: Add validation for hostname parameter (#504)
aaron-costello [Sun, 3 Nov 2019 16:22:22 +0000 (16:22 +0000)] 
cachemgr.cgi: Add validation for hostname parameter (#504)

Prevention of HTML/invalid chars in host param

4 years agoTranslatons: Fixed grammatical error in French error pages (#500)
wouldsmina [Thu, 31 Oct 2019 06:15:56 +0000 (06:15 +0000)] 
Translatons: Fixed grammatical error in French error pages (#500)

4 years agoFix expiration of self-signed generated certs to be 3 years (#501)
James DeFelice [Mon, 28 Oct 2019 10:53:51 +0000 (10:53 +0000)] 
Fix expiration of self-signed generated certs to be 3 years (#501)

Generated self-signed certificates lifetime was incorrectly set to
356*3 days, which is approximately 27 days shorter than 3 years.

The fixed problem does not affect Squids using "sslproxy_cert_adapt
setValidAfter" and Squids using a configured signing CA certificate.

4 years agoPreserve caller context across Happy Eyeballs connection attempts (#499)
Eduard Bagdasaryan [Mon, 21 Oct 2019 21:32:18 +0000 (21:32 +0000)] 
Preserve caller context across Happy Eyeballs connection attempts (#499)

To efficiently enforce various global and local limits, Happy Eyeballs
jobs uses two stand-alone HappyOrderEnforcer services that create job
calls. Thus, they need manual adjustments to preserve job context.

If similar changes are required in many places, we may want to add a
CodeContext member to the AsyncJob itself so that callbacks can
magically restore their context without service modifications (assuming
the job was created in or somehow provided the right context before
those callbacks).

4 years agoHash Digest noncedata (#491)
squidcontrib [Sun, 20 Oct 2019 18:59:08 +0000 (18:59 +0000)] 
Hash Digest noncedata (#491)

These commits together
1. Hash the noncedata for Digest nonces before encoding,
   to match the documentation.
2. Encode Digest nonces using hex, rather than base64.

4 years agoPreserve caller context across DNS lookups (#496)
Alex Rousskov [Sun, 20 Oct 2019 07:35:40 +0000 (07:35 +0000)] 
Preserve caller context across DNS lookups (#496)

For now, the context is restored when communicating with _individual_
callers waiting for their DNS lookup results. Eventually, we might also
support establishing a multi-caller context during DNS answer parsing,
before individual callers are notified. That feature would most likely
require making idns_query refcount-based (a serious change). Or we could
realize that maintaining a very basic query ID-based context is enough.

4 years agoFix build after ccfbe8f (Report context of cache.log messages) (#495)
Alex Rousskov [Sat, 19 Oct 2019 16:19:10 +0000 (16:19 +0000)] 
Fix build after ccfbe8f (Report context of cache.log messages) (#495)

In some environments (e.g., Ubuntu 14 without libcppunit-dev), GCC
correctly complained that it does not know how to print "[unknown]",
probably because none of the other headers imported `<ostream>` before
importing base/InstanceId.h.

4 years agosmblib: Improve SMB server name maintenance (#494)
aaron-costello [Fri, 18 Oct 2019 23:03:27 +0000 (23:03 +0000)] 
smblib: Improve SMB server name maintenance (#494)

4 years agoRe-enabled updates of stored headers on HTTP 304 responses (#485)
Alex Rousskov [Sat, 12 Oct 2019 00:40:35 +0000 (00:40 +0000)] 
Re-enabled updates of stored headers on HTTP 304 responses (#485)

Commit 60ba25f disabled header updates (introduced in commit abf396e)
after we discovered critical inconsistencies in related metadata
updates. Finding a way to keep metadata consistent proved to be very
difficult. The primary challenge is the multitude of often concurrent
and semi-dependent activities associated with a single StoreEntry object
(e.g., writing an incoming miss response into RAM, caching the response,
loading a cache hit into RAM, and sending a response to the client).

Concurrent activities (re)set or use overlapping sets of 304-dependent
members, including StoreEntry "basics" (e.g. StoreEntry::swap_file_sz
and timestamp), "reply" (MemObject::reply_ including its hdr_sz member),
and "data" (MemObject::data_hdr). A data member update by one activity
affects other activities.

Splitting one StoreEntry object into two internally consistent and
"constant" StoreEntry objects (one old and one updated) does not work
well because there is no mechanism to share StoreEntry "data" storage
and invokeHandlers() call streams after the split.

To avoid crashes and response corruption due to inconsistent sizes and
offsets, all size-related data members must be based on the same entry
"version". If we update one such member, we must update all others.

Furthermore, due to copying of information into activity-local
variables/state, we cannot update anything while an activity is still
running. For example, two HTTP clients may use the same StoreEntry to
receive data, and one of them may already be in the middle of response
sending, using old response offsets/sizes, when a 304 update arrives for
the other.

With any updates of size-related StoreEntry data members ruled out, the
only remaining solution for preserving consistency is to keep all those
members constant/stale despite the 304 update! The updated size-related
info has to go somewhere else (inside the same StoreEntry object).

The updated headers are now stored in a new MemObject::updatedReply
field. The updated headers are swapped out, but the old StoreEntry is
not (and was not before these changes) associated with the new store
entry anchor. After the old StoreEntry is abandoned, new local hits will
use the updated anchors. Other worker hits will use the updated anchors
immediately, but they will create fresh StoreEntry objects.

We update freshness-related data members because the associated instant
decisions should not lead to inconsistencies, and using the latest info
is preferable. If any counter-examples are found, we may have to split
StoreEntry::timestamp/etc. fields into stale and fresh groups.

We do not update Vary-related data members. See rationale below[1].

Also removed HttpHeader::update() code that disabled needUpdate() checks
for non-CF configurations. The check is expensive but storing the
updated response is a lot more expensive so even if a small fraction of
checks prevents updates, we may improve performance. Also moved the
corresponding code into HttpReply so that most Header::update() callers
(that have nothing to do with 304 updates) do not spend time on it.

Also adjusted CheckQuickAbortIsReasonable(): The old expectlen setting
code did not check for unknown content length, relying on "-1 + hdr_sz"
to be negative only when no data has been received. We now use a more
direct (but possibly still broken for HTTP/0) test (hdr_sz <= 0) and
avoid using unknown content_length in arithmetic expressions. HTTP/0
aside, responses without Content-Length should still be aborted but now
with a correct "unknown content length" debug message.

MemObject is always constructed with an (empty) base reply. We now also
assert that MemObject always has a (possibly empty) base reply after
checking that all [indirect] replaceBaseReply() callers either
* supply a non-nil reply or
* call replaceHttpReply() with a true andStartWriting parameter, going
  through an existing assert(rep) in StoreEntry::startWriting().

[1] Why exclude Vary response headers from 304 updates?

RFC 7234 Section 4.3.4 requires that Squid updates cached Vary. However,
reacting to changed variance requires either forgetting all cached
entries attached to the old Vary mark entry (bad for caching) or
re-keying all those entries using a new variance and a new Vary mark.
Re-keying requires both maintaining a list of cached Vary-controlled
entries and caching _request_ headers for every such entry!

Whether HTTP compliance is worth this complexity is debatable. Perhaps
origins should not return 304s to change variance? FWIW, Fastly folks
decided that it is _not_ worth it for them; look for the "Side note" in
https://www.smashingmagazine.com/2017/11/understanding-vary-header/

4 years agoReport context of level-0/1 cache.log messages (#483)
Alex Rousskov [Fri, 4 Oct 2019 14:14:09 +0000 (14:14 +0000)] 
Report context of level-0/1 cache.log messages (#483)

Most runtime level-0/1 cache.log messages do not carry information
sufficient to identify the transaction that caused the problem. Admins
are forced to guess the transaction based on message timestamp and, if
they are lucky to get one, request URL. The associated triage and
mitigation delay costs are often significant and can be huge, especially
when administering busy proxies in complex deployment environments.

The focus of this change is associating level-0/1 cache.log messages
with access.log records, but the same API is also used for associating
master transactions with (sections of) debugging cache.log messages.

Since level-0/1 messages are rare, association code usually wastes
resources. This performance overhead is reduced by saving pointers to
the existing transaction information (e.g., ALE). ALE gives access to
HttpRequest and MasterXaction (where available), is available in
contexts where HttpRequest and MasterXaction do not exist, and (unlike
HttpRequest) represents the whole master transaction rather than its
(often repeated) component.

CodeContext::Current() represents the current master transaction (or a
similar "primary processing task" context). A new context is created
when the master transaction (or a similar "primary processing task")
starts. Context changes usually happen in low-level transaction-unaware
callback-calling "context switching" code such as DoSelect().

The vast majority of AsyncCalls, including AsyncCall-based callbacks,
should run in their creator's context. This association is easily
automated. The primary difficulty is in handling C-style typeless calls
that prohibit context storage and restoration automation:

* In our design, the "context switching" code is ultimately responsible
  for associating the being-saved callback with the current code context
  and for restoring CodeContext::Current() when calling that callback.

* An alternative design would task the higher-level callback creator and
  callback recipient with saving/restoring CodeContext::Current(). That
  design is inferior because there are a lot more callback creators and
  recipients than "context switchers". That alternative would require a
  lot more manual changes.

The code context remains unknown if the context creator is not
instrumented to set CodeContext::Current(). TODO: Instrument ICP query
listener. Check for others.

The code context gets forgotten if the context switcher dealing with
C-style callbacks does not remember and restore CodeContext::Current().
TODO: Instrument remaining DoSelect()s, event.cc, as well as DNS, ICP,
HTCP, and IPC listeners/handlers. Check for others.

This change covers epoll DoSelect(), TcpAcceptor, ConnStateData, and SMP
disk I/O (IpcIoFile). It already annotates several level-0/1 messages
and significantly improves complex debugging. The remaining
instrumentation TODOs are likely to use similar techniques.

Squid might report wrong context until all context switchers are
instrumented, but the vast majority of uninstrumented cases result in
benign loss of context knowledge rather than mis-attribution. No design
guarantees correct attribution until all C-style callbacks are gone.

TODO: Remove legacy ctx_enter()/exit() debugging that covers very little
while suffering from worse mis-attribution problems.

Also log "-" instead of the made-up method "NONE".

4 years agoBug 4989: Leaking StoreEntry objects on Cache Digest rebuilds (#487)
Craig Gowing [Thu, 3 Oct 2019 23:11:48 +0000 (23:11 +0000)] 
Bug 4989: Leaking StoreEntry objects on Cache Digest rebuilds (#487)

When writing a newly generated Cache Digest to cache, Squid relied on a
cache key collision to purge the old digest entry. Since 4310f8b, the
collision resolution method -- forcePublicKey() -- leaked an idle (i.e.
lock_count=0) digest entry. If Squid still had unlocked entries lying
around, then the problem could extend to clashes unrelated to Digests.

Until 4310f8b, StoreEntry::forcePublicKey() called setPrivateKey()
before releasing the old entry. That explicit call was wasteful in many
cases, but, unbeknownst to its removal authors, it allowed release() to
destroy an idle Cache Digest entry by effectively disabling the
ENTRY_SPECIAL hack in StoreEntry::locked().

This change removes the ENTRY_SPECIAL hack in StoreEntry::locked(),
addressing an old TODO. The two ENTRY_SPECIAL creators (icons and Cache
Digests) now lock their entries to prevent their unwanted destruction.

Also explicitly release the old Cache Digest entry (instead of relying
on the implicit key collision) to avoid the unchecked assumption that
the Cache Digest key never changes.

4 years agoCleanup: rename String::limitInit() to assign() (#488)
josepjones [Thu, 3 Oct 2019 13:57:32 +0000 (13:57 +0000)] 
Cleanup: rename String::limitInit() to assign() (#488)

4 years agoAdd GeneratingCONNECT step for the existing at_step ACL (#484)
Christos Tsantilas [Wed, 2 Oct 2019 09:20:02 +0000 (09:20 +0000)] 
Add GeneratingCONNECT step for the existing at_step ACL (#484)

The new step allows admins to customize CONNECT request generation using
request_header_access and request_header_replace. As detailed below,
matching the request method does not work for those purposes.

request_header_access controls what HTTP request headers Squid _sends_.
However, this directive, like most other ACL-driven Squid directives,
uses the received request, _not_ the being-formed to-be-sent request.
This can be confusing, but it is the correct design/behavior.

When Squid is about to send a CONNECT request to a cache peer, what
_received_ request should request_header_access ACLs look at? There are
several ways to answer that question:

1. The received CONNECT request (or, for intercepted TLS connections,
   its faked equivalent). This is how the current CONNECT-sending code
   works when establishing the tunnel for a not-yet-bumped client
   connection. Problems:

    1. The CONNECT request received by Squid was not meant for the cache
       peer. It was meant specifically for this Squid. While it may have
       info useful for request_header_access checks, it is conceptually
       wrong to think of whats happening as "forwarding of that received
       CONNECT to the cache peer". Unlike GET requests, the CONNECT
       request that this Squid will send to the cache peer is dedicated
       to the Squid-peer connection. It is generated, not forwarded.

    2. That CONNECT request may have been received a long time ago, and
       Squid may have forwarded many bumped GET requests since then. It
       feels strange to consult such an old/"disconnected" message.
2. The received (and bumped) GET request. This is how the current
   CONNECT-sending code works when establishing the tunnel for an
   already bumped client connection. Problem:

    1. Squid is about to send a generated CONNECT request, not to
       forward a received GET request (the latter will happen later,
       after the CONNECT transaction). The two requests may differ a
       lot. Using a GET request when generating a CONNECT request is
       confusing: "Why is my CONNECT method ACL does not match when
       Squid sends a CONNECT request?!"

3. No request. Problems:

    1. Some old configurations that use request-specific ACLs with
       request_header_access will generate runtime "missing request"
       warnings and may fail to work correctly.

    2. Extra admin work is required to store request-specific
       information as connection annotations that request_header_access
       ACLs can still access.

    3. Conceptually, there is a request that Squid is establishing this
       CONNECT tunnel for. Squid will access-log that request. Hiding
       that information from ACLs feels odd/wrong. And some of that info
       would still be accessible to ACLs (via ALE/etc.). This hiding
       does not really hide all of the details.

Our solution preserves what received request Squid is looking at. Items
1 and 2 above still apply (depending on the configuration and on the
request being processed), with their unique problems intact. Those
problems are not as bad as the problems associated with the item 3!

The at_step ACL was added for SslBump but, IIRC, we knew that it may
eventually cover other request processing steps. Generating a CONNECT
request is one of those other steps.

This is a Measurement Factory project.

4 years agoLog "-" instead of the made-up method "NONE". (#486)
Alex Rousskov [Sat, 28 Sep 2019 13:58:28 +0000 (13:58 +0000)] 
Log "-" instead of the made-up method "NONE". (#486)

Also useful for the future ALE-using context-reporting code that must be
silent regarding unknowns, including unknown request methods.

4 years agoBug 4987: restore support for the "cache_object://" URI scheme (#475)
Christos Tsantilas [Tue, 24 Sep 2019 10:04:05 +0000 (10:04 +0000)] 
Bug 4987: restore support for the "cache_object://" URI scheme (#475)

Broken by commit 6c880a1.

4 years agoFix URN response exception handling (#479)
Eduard Bagdasaryan [Sat, 21 Sep 2019 22:45:03 +0000 (22:45 +0000)] 
Fix URN response exception handling (#479)

CID 1453857: Error handling issues (UNCAUGHT_EXCEPT)

Due to various Store deficiencies, storeUnregister() might call swapout
code which Broadcast()s and throws Ipc::OneToOneUniQueue::ItemTooLarge.

4 years agoFix the SQUID_CC_REQUIRE_ARGUMENT autoconf function (#478)
Dimitry Andric [Sat, 21 Sep 2019 13:34:40 +0000 (13:34 +0000)] 
Fix the SQUID_CC_REQUIRE_ARGUMENT autoconf function (#478)

Inside AC_DEFUN(), autoconf replaces `$1` with the first argument of the
function.  In this case, the first argument is a variable name. To get
the _value_ of that variable, one has to use `$$1`.

One known effect of this fix (in many build environments) is the
disappearance of the following annoying extra error when a build fails
for some other reason:

    unrecognized command line option -Wno-deprecated-register

4 years agoSupply ALE to request_header_access/reply_header_access (#477)
Eduard Bagdasaryan [Sat, 21 Sep 2019 12:46:03 +0000 (12:46 +0000)] 
Supply ALE to request_header_access/reply_header_access (#477)

Commit cb36505 already covered many (but not all) similar cases.

Also supply HttpReply to cache_peer_access. Lack of reply
caused "ACL is used in context without an HTTP response" bug.

4 years agoSet ALE::reply to the 200 (Connection established) (#476)
Eduard Bagdasaryan [Fri, 20 Sep 2019 11:15:21 +0000 (11:15 +0000)] 
Set ALE::reply to the 200 (Connection established) (#476)

... thus addressing a TODO. Lack of reply may cause
"ACL is used in context without an HTTP response" errors in some
contexts. These contexts (if any) should be fixed separately.

4 years agoFix URN response handling (#472)
Eduard Bagdasaryan [Sun, 15 Sep 2019 15:32:30 +0000 (15:32 +0000)] 
Fix URN response handling (#472)

urnHandleReply() may be called several times while copying the entry
from the store. Each time it must use the buffer length that is left
(from the previous call).

Also do not abandon a urn entry, still having clients attached.

Also allow urnHandleReply() to produce a reply if it receives a
zero-sized buffer. This may happen after the entry has been fully
stored.

4 years agoFix build broken by 6c880a1 (#473)
Eduard Bagdasaryan [Sat, 14 Sep 2019 14:53:27 +0000 (14:53 +0000)] 
Fix build broken by 6c880a1 (#473)

4 years agoAdd %master_xaction logformat code (#471)
Eduard Bagdasaryan [Fri, 13 Sep 2019 00:56:20 +0000 (00:56 +0000)] 
Add %master_xaction logformat code (#471)

Currently, knowing master transaction ID can be very helpful in triage,
especially when dealing with flash crowds on busy proxies. Upcoming
changes will also tie many current "anonymous" level-0/1 messages to
logged transactions via this ID.

4 years agoFix detection of OpenSSL built w/o deprecated features support (#470)
FX Coudert [Wed, 11 Sep 2019 05:12:04 +0000 (05:12 +0000)] 
Fix detection of OpenSSL built w/o deprecated features support (#470)

SSL_library_init() is deprecated since OpenSSL v1.1 and is absent in
OpenSSL built without deprecated features. Several distributions (e.g.
Homebrew) ship OpenSSL built without deprecated features.

4 years agoRFC 7230: server MUST reject messages with BWS after field-name (#445)
Amos Jeffries [Wed, 11 Sep 2019 02:52:52 +0000 (02:52 +0000)] 
RFC 7230: server MUST reject messages with BWS after field-name (#445)

Obey the RFC requirement to reject HTTP requests with whitespace
between field-name and the colon delimiter. Rejection is
critical in the presence of broken HTTP agents that mishandle
malformed messages.

Also obey requirement to always strip such whitespace from HTTP
response messages. The relaxed parser is no longer necessary for
this response change.

For now non-HTTP protocols retain the old behaviour of removal
only when using the relaxed parser.

4 years agoUpdate URI parser to use SBuf parsing APIs (#275)
Amos Jeffries [Tue, 10 Sep 2019 09:32:43 +0000 (09:32 +0000)] 
Update URI parser to use SBuf parsing APIs (#275)

Initial replacement of URI/URL parse method internals with
SBuf and Tokenizer based parse.

For now this parsing only handles the scheme section of
URL. With this we add the missing check for alpha character
as first in the scheme name for unknown schemes and
prohibit URL without any scheme (previously accepted).

Also polishes the documentation, URN and asterisk-form
URI parsing.

Also, adds validation of URN NID portion characters to
ensure valid authority host names are generated for
THTTP lookup URLs.

4 years agoSupply ALE with HttpReply before checking http_reply_access (#398)
Eduard Bagdasaryan [Tue, 10 Sep 2019 05:36:25 +0000 (05:36 +0000)] 
Supply ALE with HttpReply before checking http_reply_access (#398)

Before this fix, Squid warned "ALE missing HttpReply object",
because ALE::reply was initialized too late. This problem affected
both HTTP and FTP code paths.

To avoid these problems, ALE::reply is initialized early with the
available (received) response.

Also:

* Fixed logging control (1xx) responses in case these responses
  are final. Before this fix, '-' was logged.

* All reply headers ('<h') were logged with a single CR separator,
  instead of CRLF.

* Supply ALE for send_hit ACL. cb36505 already covered many (but not
  all) similar cases.

4 years agoFix "BUG: Lost previously bumped from-Squid connection" (#460)
Christos Tsantilas [Mon, 9 Sep 2019 14:14:50 +0000 (14:14 +0000)] 
Fix "BUG: Lost previously bumped from-Squid connection" (#460)

FwdState assumed that PeerSelector always returns the connection pinned
by previous SslBump steps. That assumption was wrong in two cases:

1. The previously pinned connection got closed.
2. PeerSelector policies prevent pinned connection reuse. For example,
    connection destination is now denied by cache_peer_access rules.

PeerSelector now returns a PINNED selection even if a pinned connection
cannot or should not be used. The initiator is now fully responsible for
checking the returned connection usability, including the new
ConnStateData::pinning::peerAccessDenied flag. Unusable pinned
connection is now treated as any other fatal (for the transaction)
forwarding problem rather than an internal BUG.

The above changes do not change traffic on the wire but remove bogus
level-1 BUG messages from cache.log.

We also polished which error page is returned depending on the pinning
validation problem: ERR_ZERO_SIZE_OBJECT is returned when the validation
failed because of the peer disappearance or to-server connection
closure. Other cases use ERR_CANNOT_FORWARD. Eventually, these errors
can be detailed further to distinguish various problems. We may also
want to generalize ERR_CONFLICT_HOST and/or ERR_FORWARDING_DENIED to
make them applicable in this context.

This is a Measurement Factory project.

4 years agoSource Format Enforcement (#467)
squidadm [Sat, 7 Sep 2019 01:43:07 +0000 (01:43 +0000)] 
Source Format Enforcement (#467)

4 years agoSupply ALE for note ACL (#463)
Eduard Bagdasaryan [Thu, 5 Sep 2019 05:46:59 +0000 (05:46 +0000)] 
Supply ALE for note ACL (#463)

The following cache.log WARNING is a symptom of this bug:

    ACL is used in context without an ALE state. Assuming mismatch.

Commit cb36505 already covered many (but not all) similar cases.

4 years agoFix cross-compilation (#464)
Fabrice Fontaine [Fri, 23 Aug 2019 20:22:19 +0000 (20:22 +0000)] 
Fix cross-compilation (#464)

Do not check check file descriptor maximum value through AC_RUN_IFELSE
when cross-compiling as this will raise an error

Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
4 years agoBug 4918: Crashes when using OpenSSL prior to v1.0.2 (#465)
Eneas U de Queiroz [Wed, 21 Aug 2019 18:01:24 +0000 (18:01 +0000)] 
Bug 4918: Crashes when using OpenSSL prior to v1.0.2 (#465)

The implementation of x509_get0_signature() replacement in 24b30fd was
based on OpenSSL v1.1.0 where `signature` and `sig_alg` members of
`x509_st` structure stopped being raw pointers and became structures.
The mismatch caused segfaults when using OpenSSL versions that lacked
x509_get0_signature() -- anything earlier than OpenSSL v1.0.2.

    // OpenSSL < v1.1.0
    struct x509_st {
        X509_CINF *cert_info;
        X509_ALGOR *sig_alg;
        ASN1_BIT_STRING *signature;
        ...
    }

    // OpenSSL >= v1.1.0
    struct x509_st {
        X509_CINF cert_info;
        X509_ALGOR sig_alg;
        ASN1_BIT_STRING signature;
        ...
    }

A C-style reinterpreting cast hid the type mismatch from the compilers
and reviewers.

Tested with OpenSSL v1.0.1f. The types of the two data members were
checked back to OpenSSL v0.9.6.

Signed-off-by: Eneas U de Queiroz <cotequeiroz@gmail.com>
4 years agoBug 3552 pt3: Happy Eyeballs: Parallel TCP connection attempts (#386)
Christos Tsantilas [Thu, 15 Aug 2019 08:21:08 +0000 (08:21 +0000)] 
Bug 3552 pt3: Happy Eyeballs: Parallel TCP connection attempts (#386)

A site with both AAAA and A DNS records may only be available via one of
the IP address families; attempts to connect to that site over the other
IP family will fail, sometimes after painful timeouts. Prior to these
changes, Squid tried to connect to resolved addresses sequentially,
which often resulted in unhappy user eyeballs and aborted (by clients)
client-Squid connections, especially when the first DNS answer contained
multiple unusable IP addresses.

To reduce user-visible delays, Squid now follows the Happy Eyeballs (RFC
8305) strategy: Start opening a to-server TCP connection using IPvX and,
if that "primary" connection was not established fast enough, initiate a
parallel "spare" connection opening attempt using IPvY. As before, X is
the IP protocol family in the first/fastest DNS response received by
Squid. As more IP addresses (from each family) become known, they feed
subsequent connection opening attempts on primary and spare tracks.

No changes in peer selection. No changes in peer usage: Squid still
exhausts all paths to peer[N] before using peer[N+1] IPs, even if it
means waiting for DNS A answer for peer[N] while sitting on an AAAA
answer for peer[N+1].

Happy Eyeballs implementations must balance the desire to improve
response times (by opening as many parallel connections as fast as
possible) with the dangers of triggering DoS alarms and creating
significant traffic overheads. To control that balance, Squid never uses
more than two parallel tracks for forwarding a single request and
provides three admin-configurable parameters (with reasonable defaults).

* happy_eyeballs_connect_timeout forces spare connection establishment
  track to wait a little (to give the primary track a chance to
  establish a connection).

* happy_eyeballs_connect_gap spaces out spare connection openings (to
  minimize spare traffic).

* happy_eyeballs_connect_limit caps the total number of spare
  connections (to reduce stress on Squid resources).

This is a Measurement Factory project.

4 years agoFixed parsing of TLS messages that span multiple records (#457)
Christos Tsantilas [Tue, 13 Aug 2019 14:44:22 +0000 (14:44 +0000)] 
Fixed parsing of TLS messages that span multiple records (#457)

Squid fed the TLS message parser with one TLS record fragment
at a time but allowed InsufficientInput exceptions to bubble up
beyond the TLS message parsing code. If a server handshake
message spans multiple TLS records, and Squid reads all those
records together with the end of the TLS server handshake, then
the higher-level code interprets InsufficientInput as the need
for more TLS records for the record parser (rather than more
fragments for the TLS message parser). The affected transaction
would then time out or otherwise fail while waiting for those
non-existent TLS records to come from the server.

We now parse TLS messages only after accumulating all same-type
TLS records. For truncated handshakes, this may reduce the
level of information extracted by Squid in some cases, but
this approach keeps the code simple. The handshake is still
available for logging if that partial info is needed for triage.

Test case: 1000-sans.badssl.com which sends a huge server certificate.

This is a Measurement Factory project.

4 years agoIgnore malformed Host header in intercept and reverse proxy mode (#456)
Amos Jeffries [Tue, 13 Aug 2019 13:50:06 +0000 (13:50 +0000)] 
Ignore malformed Host header in intercept and reverse proxy mode (#456)

4 years agoFixed on_unsupported_protocol tunnel action (#339)
Christos Tsantilas [Mon, 12 Aug 2019 15:48:10 +0000 (15:48 +0000)] 
Fixed on_unsupported_protocol tunnel action (#339)

Instead of tunneling traffic, a matching on_unsupported_protocol
"tunnel" action resulted in a Squid error response sent to the client
(or, where an error response was not possible, in a connection closure).
The following three cases were fixed:

    * port: http_port (real CONNECT)
    * ssl_bump action: client-first or step1 bump
    * handling phase: parsing TLS client handshake
    * expected data: TLS Client Hello

    * port: http_port (real CONNECT)
    * ssl_bump action: client-first or step1 bump
    * handling phase: parsing the first bumped HTTP request
    * expected data: HTTP request header

    * port: https_port (fake CONNECT)
    * ssl_bump action: any action except terminate
    * handling phase: parsing TLS client handshake
    * expected data: TLS Client Hello

Also, when on_unsupported_protocol was configured, Squid wasted RAM and
CPU cycles to buffer client HTTP requests beyond the point of no return
(i.e., roughly, beyond the first HTTP request on a connection or in a
tunnel), when on_unsupported_protocol settings no longer apply.

Client handshake accumulation is now driven by preservingClientData_. We
set that data member when the connection is accepted (because we may
decide to start preserving bytes right away) and reset it whenever that
decision may change, including when switching to a new protocol inside
CONNECT tunnel and confirming the expected/supported protocol by
successfully parsing its handshake.

Squid does not stop handshake preservation when on_unsupported_protocol
gets disabled during reconfiguration, but Squid will not tunnel
preserved bytes if that happens (and will not tunnel a partial handshake
if on_unsupported_protocol configuration keeps changing).

Also changed how IPv6-based certificates are generated. Their CN field
value is no longer surrounded by [square brackets]. This change was done
to improve Squid code that had to be modified to fix
on_unsupported_protocol. It affects certificate cache key so old
IPv6-based certificates will never be found (and will eventually be
purged) while new ones will be generated and cached instead. We believe
these IPv6-based certificates are rare and untrusted by browsers so the
change in their CN should not have a significant affect on users.

This is a Measurement Factory project.

4 years agoFixed MemStore::copyFromShm() debugging (#454)
Eduard Bagdasaryan [Sun, 11 Aug 2019 20:17:53 +0000 (20:17 +0000)] 
Fixed MemStore::copyFromShm() debugging (#454)

* Level-9 is reserved for data dumps.

* The "mem-loaded all" message was printing -1 instead of the
  accumulated object size. It also deserves a lower debugging level
  because it happens at most once per transaction.

4 years agoAdded ENTRY_REQUIRES_COLLAPSING debugging (#453)
Eduard Bagdasaryan [Sat, 10 Aug 2019 15:46:19 +0000 (15:46 +0000)] 
Added ENTRY_REQUIRES_COLLAPSING debugging (#453)

...  forgotten in master d2a6dcb.

4 years agoFix parsing of certificate validator responses (#452)
Christos Tsantilas [Fri, 9 Aug 2019 04:18:37 +0000 (04:18 +0000)] 
Fix parsing of certificate validator responses (#452)

If a certificate validator did not end its response with an end-of-line
or whitespace character, then Squid, while parsing the response,
accessed the bytes after the end of the buffer where the response is
stored.

This is a Measurement Factory project.

4 years agoFix openSUSE ./configure-time Linux PAM authentication detection (#450)
Adam Majer [Thu, 8 Aug 2019 09:47:59 +0000 (09:47 +0000)] 
Fix openSUSE ./configure-time Linux PAM authentication detection (#450)

Recently, openSUSE Factory enabled -Wreturn-type in CFLAGS which
resulted in ./configure failing to detect Linux type of PAM
authentication.

4 years agoFix rock disk entry contamination related to aborted swapouts (#444)
Eduard Bagdasaryan [Wed, 7 Aug 2019 12:04:30 +0000 (12:04 +0000)] 
Fix rock disk entry contamination related to aborted swapouts (#444)

Also probably fixed hit response corruption related to aborted rock
swapins.

The following disk entry contamination sequence was observed in detailed
cache logs during high-load Polygraph tests. Some of the observed
level-1 errors matched those in real-world deployments.

1. Worker A schedules AX – a request to write a piece of entry A to disk
   slot X. The disker is busy writing and reading other slots for worker
   A and other workers so AX stays in the A-to-Disker queue for a while.
2. Entry A aborts swapout (for any reason, including network errors
   while receiving the being-stored response). Squid makes disk slot X
   available for other entries to use. AX stays queued.
3. Another worker B picks up disk slot X (from the shared free disk slot
   pool) and schedules BX, a request to write a piece of entry B to disk
   slot X. BX gets queued in an B-to-Disker queue. AX stays queued.
4. The disker satisfies write request BX. Disk slot X contains entry B
   contents now. AX stays queued.
5. The disker satisfies write request AX. Disk slot X is a part of entry
   B slot chain but contains former entry A contents now! HTTP requests
   for entry B now read entry A leftovers from disk and complain about
   metadata mismatches (at best) or get wrong response contents (at
   worst).

To prevent premature disk slot reuse, we now keep disk slots reserved
while they are in the disk queue, even if the corresponding cache entry
is long gone: Individual disk write requests now "own" the slot they are
writing. The Rock::IoState object owns reserved but not yet used slots
so that they can be freed when the object is gone. The disk entry owns
the (successfully written) slots added to its chain in the map.

The new slot ownership scheme required changes in what metadata the
writing code has to maintain. For example, we now keep the address of
the previous slot in the entry chain so that we can update its .next
field after a successful disk write. Also, the old code detecting
dropped write requests could no longer rely on the now-empty .next field
in the previous map entry. The rewritten code numbers I/O transactions
so that out-of-order replies can be detected without using the map.

I tried to simplify the metadata maintenance code by shortening
reservation lifetimes and using just-in-time [first] slot reservations.
The new code may also leak fewer slots when facing C++ exceptions.

As for reading, I realized that we had no protection from dropped rock
read requests. If the first read request is dropped, the metadata
decoding would probably fail but if subsequent reads are skipped, the
client may be fed with data that is missing those skipped blocks. I did
not try to reproduce these problems, but they are now fixed using the
same I/O transaction numbering mechanism that the writing code now uses.
Negative length checks in store_client.cc treat dropped reads as errors.

I also removed commented out "partial writing" code because IoState
class member changes should expose any dangerous merge problems.

4 years agoBug 4978: eCAP crash after using MyHost().newRequest() (#449)
Vadim Salavatov [Tue, 6 Aug 2019 23:11:36 +0000 (23:11 +0000)] 
Bug 4978: eCAP crash after using MyHost().newRequest() (#449)

Since commit 8babada, Squid was using a c_str() result after its
std::string toString() source went out of scope.

4 years agoDocs: Fix Programming Guide typos (#448)
Emil Hessman [Mon, 5 Aug 2019 18:39:23 +0000 (18:39 +0000)] 
Docs: Fix Programming Guide typos (#448)

4 years agoRefactor RefCount tests to use cppunit (#442)
josepjones [Mon, 29 Jul 2019 12:20:15 +0000 (12:20 +0000)] 
Refactor RefCount tests to use cppunit (#442)

4 years agoBug 4969: GCC-9 build failure: stringop-truncation (#441)
Amos Jeffries [Fri, 19 Jul 2019 15:11:49 +0000 (15:11 +0000)] 
Bug 4969: GCC-9 build failure: stringop-truncation (#441)

GCC-9 with Squid use of -Werror makes these warning hard
errors which can no longer be ignored. We are thus required
to alter this third-party code when built for Squid.

Truncation of these strings is fine. Rather than suppress
GCC warnings, switch to xstrncpy() which has similar
behaviour but guarantees c-string terminator exists within
the copied range limit (removing need for two -1 hacks).

This change will add terminators on path and device_type
values in the rare case of overly long configured values.
It is not clear what ancient Domain Controllers would do
when handed un-terminated c-string in those cases, but was
unlikely to be good.

4 years agoDisabled partial rock cache_dir writes (#440)
Eduard Bagdasaryan [Fri, 19 Jul 2019 09:23:55 +0000 (09:23 +0000)] 
Disabled partial rock cache_dir writes (#440)

Partial disk writes may be useful for CF disk slaves and SMP disk hit
readers, but their correct implementation requires a lot of additional
work, the current implementation is insufficient/buggy, and partially
written entries were probably never read because Rock writers do not
enable shared/appending locks.

Here is a brief (but complicated) history of the issue, for the record:

    1. 807feb1 adds partial disk writes "in order to propagate data from
       the hit writer to the collapsed hit readers". The readers
       probably could not read any data though because the disk entry
       was still exclusively locked for writing. The developers either
       did not realize that or intended to address it later, but did not
       document their intentions -- all this development was happening
       on a fast-moving CF development branch.

    2. 0b8be48 makes those partial disk writes conditional on CF being
       enabled. It is not clear whether the developers wanted to reduce
       the scope of a risky feature or did not realize that non-CF use
       cases also benefit from partial writes (when fully supported).

    3. ce49546 adds low-level appending lock support but does not
       actually use append locks for any caches.

    4. 4475555 adds appending support to the shared memory cache.

    5. 4976925 explicitly disables partial disk writes, acknowledging
       that they were probably never used (by readers) anyway due to the
       lack of a Ipc::StoreMap::startAppending() call. The same commit
       documents that partial writes caused problems (for writers) in
       performance tests.

    6. 5296bbd re-enables partial disk writes (for writers) after fixing
       problems detected earlier in performance tests. This commit does
       not add the critical (for readers) startAppending() call. It
       looks like the lack of that call was overlooked, again!

4 years agoBug 4970: excessive gnutls_certificate_credentials debug msgs (#439)
Thomas Zajic [Fri, 19 Jul 2019 00:51:45 +0000 (00:51 +0000)] 
Bug 4970: excessive gnutls_certificate_credentials debug msgs (#439)

Also fixed destruction message text format for consistency sake and to
match scripts/find-alive.pl expectations.

Broken since commit 60fcfad.

4 years agoDetail validation errors for loaded-from-file certificate chains (#438)
ghulands [Wed, 17 Jul 2019 01:06:33 +0000 (01:06 +0000)] 
Detail validation errors for loaded-from-file certificate chains (#438)

4 years agoBug 4966: Lower cache_peer hostname (#420)
uhliarik [Mon, 15 Jul 2019 03:42:39 +0000 (03:42 +0000)] 
Bug 4966: Lower cache_peer hostname (#420)

When parsing entries from /etc/hosts file, they are all lowered
(see bug 3040). If cache_peer hostname is uppercase, it will
lead to DNS resolution failure. Lowering cache_peer host fixes
this issue.

This change may expose broken Squid configurations that
incorrectly relied on non-lowercase peer host names to
bypass Squid's "is this cache_peer different from me?"
check. Though such configurations should encounter
forwarding loop errors later anyway.

4 years agoRename allow_t to Acl::Answer (#425)
josepjones [Fri, 12 Jul 2019 14:09:48 +0000 (14:09 +0000)] 
Rename allow_t to Acl::Answer (#425)

Enact the TODO item asking to rename class to Acl::Answer

4 years agoPrevent truncation for large origin-relative domains (#427)
Amos Jeffries [Fri, 12 Jul 2019 03:08:00 +0000 (03:08 +0000)] 
Prevent truncation for large origin-relative domains (#427)

The domain in a URL must obey hostname length restrictions after
append_domain is applied, not just MAX_URL for the normalized
absolute-URL.

4 years ago4.8 Release prep (#431)
Amos Jeffries [Mon, 8 Jul 2019 09:07:47 +0000 (21:07 +1200)] 
4.8 Release prep (#431)

* Fix date for v4.6 release

4 years agoBug 4957: Multiple XSS issues in cachemgr.cgi (#429)
Amos Jeffries [Fri, 5 Jul 2019 03:17:26 +0000 (03:17 +0000)] 
Bug 4957: Multiple XSS issues in cachemgr.cgi (#429)

The cachemgr.cgi web module of the squid proxy is vulnerable
to XSS issue. The vulnerable parameters "user_name" and "auth"
have insufficient sanitization in place.

4 years agoPrevent TLS transaction stalls by preserving flags.read_pending (#423)
Eduard Bagdasaryan [Sun, 30 Jun 2019 10:57:58 +0000 (10:57 +0000)] 
Prevent TLS transaction stalls by preserving flags.read_pending (#423)

DoSelect() cleared flags.read_pending just like it clears the read
handler, but that was wrong: Unlike the read handler, the "pending read"
state is not managed by the read _handler_. It is managed by the reading
_method_. DoSelect() calls read handler so it has the right to clear the
handler before that call -- the called handler is given an opportunity
to set another read handler as needed. DoSelect() does not call the
reading method.

In most cases, the reading method is called by the reading handler so
the code appears to "work". However, if the reading handler has to defer
the read, it will not call the reading method. Deferred reads are
naturally ignorant about the high-level read_pending flag so they cannot
(and should not) attempt to preserve that flag. The flag and the
associated "I have buffered data to give to the reader" state get lost.
When the deferred read is kicked, it starts waiting for socket I/O.

The loss of the flag usually leads to (minor) response time increases
because the peer socket will have more data available for reading,
triggering a call to the read handler which (this time) calls the
reading method which supplies the previously buffered data. Thus, the
code usually appears to "work" even when the bug is triggered.

However, when the buffered data contained the _last_ response byte, the
peer socket often remains idle (not ready for reading), the transaction
gets stalled and eventually timeouts/aborts without delivering the last
byte(s) to the client. Such bugs have been observed in the wild.

This change delegates read_pending management to the reading method.
When the method is not called (e.g., because reading is deferred), the
flag remains set and continues to reflect the current reading state.

When the reading method is "downgraded" from tls_read_method() to
default_read_method(), the new code clears flags.read_pending to avoid
endless "this FD is ready for reading" looping. The downgrade itself is
a dangerous operation because it may result in the loss of the already
buffered data. I hope Squid does not lose data when downgrading (and
added comments to justify that hope), but I cannot guarantee that. This
change itself does not introduce data losses.

I also made fde I/O methods private to improve our chances of preventing
a similar bug if future code changes carelessly reset them.

4 years agoFix macro name clash on freebsd (#426)
Francesco Chemolli [Sat, 29 Jun 2019 16:01:32 +0000 (16:01 +0000)] 
Fix macro name clash on freebsd (#426)

FreeBSD defines FD_NONE in /usr/include/fcntl.h to be magic to
the system. We are not using that name explicitly anywhere, but
it may make sense to keep it around as a default value for
fd_type. Rename the symbol to avoid the clash and fix the build
on FreeBSD.

4 years agoBug 4842: Memory leak when http_reply_access uses external_acl (#424)
Christos Tsantilas [Sat, 29 Jun 2019 07:15:34 +0000 (07:15 +0000)] 
Bug 4842: Memory leak when http_reply_access uses external_acl (#424)

Http::One::Server::handleReply() sets AccessLogEntry::reply which may
already be set. It is already set, for example, when the ACL code
has already called syncAle() because external ACLs require an ALE.

This bug was introduced by commit fbbea6620.

This is a Measurement Factory project.

4 years agoRemove userinfo support from old protocols (#419)
Amos Jeffries [Sun, 23 Jun 2019 15:15:56 +0000 (15:15 +0000)] 
Remove userinfo support from old protocols (#419)

RFC 1738 defines the URL schemes for gopher and wais as not
having the userinfo@ segment.

coap, coaps, whois and cache_object also do not use this segment.

For these cases we can obey the RFC7230 requirement to ignore the
segment when producing normalized absolute URL.

Of the supported protocols only FTP requires userinfo, and because
we cannot be certain for unknown protocols allow it as well.

5 years agoFix handling of tiny invalid responses (#422)
Eduard Bagdasaryan [Sun, 23 Jun 2019 14:14:47 +0000 (14:14 +0000)] 
Fix handling of tiny invalid responses (#422)

Squid converted any invalid response shorter than 4 bytes into an
invalid "HTTP/1.1 0 Init" response (with those received characters and a
CRLFCRLF suffix as a body). In some cases (e.g., with ICAP RESPMOD), the
resulting body was not sent to the client at all.

Now Squid handles such responses the same way it handles any non-HTTP/1
(and non-ICY) response, converting it into a valid HTTP/200 response
with an X-Transformed-From:HTTP/0.9 header and received bytes as
a message body.

5 years agoSend Connection:close with the known-last request on a connection (#421)
Eduard Bagdasaryan [Fri, 21 Jun 2019 13:03:30 +0000 (13:03 +0000)] 
Send Connection:close with the known-last request on a connection (#421)

Squid did not send Connection:close request header on a to-be-closed
HTTP/1.1 connection, violating RFC 7230 Section 6.6 "SHOULD send"
requirement.

5 years agoUpdate HttpHeader::getAuth to SBuf (#416)
Amos Jeffries [Wed, 19 Jun 2019 05:58:36 +0000 (05:58 +0000)] 
Update HttpHeader::getAuth to SBuf (#416)

Replace the fixed-size buffer for decoding base64 tokens with an
SBuf to avoid decoder issues on large inputs.

Update callers to SBuf API operations for more efficient memory
management.

5 years agoAdd the NO_TLSv1_3 option to available tls-options values (#418)
Christos Tsantilas [Sat, 15 Jun 2019 08:19:15 +0000 (08:19 +0000)] 
Add the NO_TLSv1_3 option to available  tls-options values (#418)

... also fix the deprecated sslversion option to exclude tls v1.3 from
allowed protocols where required.

This is a Measurement Factory project

5 years agoFix Digest auth parameter parsing (#415)
Amos Jeffries [Sat, 8 Jun 2019 21:09:23 +0000 (21:09 +0000)] 
Fix Digest auth parameter parsing (#415)

Only remove quoting if the domain=, uri= or qop= parameter
value is surrounded by double-quotes.

5 years agoFix GCC-9 build issues (#413)
Amos Jeffries [Sat, 8 Jun 2019 11:40:40 +0000 (11:40 +0000)] 
Fix GCC-9 build issues (#413)

GCC-9 continues the development track started with GCC-8
producing more warnings and errors about possible code issues
which Squid use of "-Wall -Werror" turns into hard build
failures:

 error: ‘strncpy’ output may be truncated copying 6 bytes from a
   string of length 6 [-Werror=stringop-truncation]

 error: ‘%s’ directive argument is null
  [-Werror=format-overflow=]

 error: ‘void* memset(void*, int, size_t)’ clearing an object of
  type ... with no trivial copy-assignment; use assignment or
  value-initialization instead [-Werror=class-memaccess]

 error: ‘void* memset(void*, int, size_t)’ clearing an object of
  non-trivial type ...; use assignment or value-initialization
  instead [-Werror=class-memaccess]

Also, segmentation faults with minimal builds have been
identified as std::string template differences between
optimized and non-optimized object binaries. This results in
cppunit (built with optimizations) crashing unit tests when
freeing memory. Workaround that temporarily by removing the use
of --disable-optimizations from minimal builds.

5 years agoICAP stalls large HTTP transactions (#414)
Eduard Bagdasaryan [Thu, 6 Jun 2019 12:08:31 +0000 (12:08 +0000)] 
ICAP stalls large HTTP transactions (#414)

Calling virginBodySending.plan() has nothing to do with ICAP trailers
support. These (expensive!) plans are necessary if and only if we may
echo the virgin message back to Squid.

Since 6fbd6e, ICAP code always planned to echo the virgin body, even
when no echoing was needed. Echoing requires buffering the virgin
message body, so these plans stalled responses with bodies exceeding
~65K buffer capacity when, for example, the ICAP server did not respond
until getting the entire request body.

5 years agoBug 4953: to_localhost does not include :: (#410)
Amos Jeffries [Thu, 6 Jun 2019 12:06:41 +0000 (00:06 +1200)] 
Bug 4953: to_localhost does not include :: (#410)

5 years agoCall PeerOptions::parseOptions() as early as possible
Amos Jeffries [Thu, 4 Apr 2019 12:08:56 +0000 (01:08 +1300)] 
Call PeerOptions::parseOptions() as early as possible

5 years agoMove session tls-options= assignment out of CreateSession() function
Amos Jeffries [Sat, 29 Dec 2018 23:58:49 +0000 (12:58 +1300)] 
Move session tls-options= assignment out of CreateSession() function

The original implementation utilized OpenSSL inheritence of
library-specific options from SSL_CTX* to SSL* structures.
This does not work with GnuTLS which requires setting the
options on each session object.

The workaround of looking up the session options inside CreateSession()
was only looking for client connection details. Which is broken when
creating server sessions.

* Extend Security::CreateSession() to take a PeerOptions reference
  containing the specific config details to associate with the new
  session (if any).

* Move selection of CachePeer vs tls_outgoing_* options
  out to Security::CreateClientSession().

* Pass the Any::PortCfg::secure details into the
  Security::CreateServerSession().

5 years agoSeparate tls-min-version= config from options= config
Amos Jeffries [Sat, 29 Dec 2018 22:56:07 +0000 (11:56 +1300)] 
Separate tls-min-version= config from options= config

.. until later when parsing the library-specific strings
into their parsedOptions binary form.

This allows multiple options= and tls-min-version= settings
with only the latest of each being used independent of what
is going on with the other config settings.

Also, add nil definitions for the NO_TLSv* OpenSSL options
to make the tls-min-version= not result in fatal errors when
using minimalized OpenSSL library builds.

5 years agoDelay parsing of options= parameters
Amos Jeffries [Thu, 27 Dec 2018 09:17:49 +0000 (22:17 +1300)] 
Delay parsing of options= parameters

.. until context (or session) creation time after squid.conf parsing.

Since a nil value in parsedOptions has meaning as default settings
we cannot use that to indicate a re-parse required. Add a flag
to set indicating changes have been made and check for a re-parse
when using parsedOptions to update a context or session.

5 years agoUpdate PeekingPeerConnector to use PeerOptions API
Amos Jeffries [Thu, 27 Dec 2018 06:42:31 +0000 (19:42 +1300)] 
Update PeekingPeerConnector to use PeerOptions API

.. for SSL_set_options() call.

Allowing parsedOptions to be private within PeerOptions
which is needed to safely delay its value being set until
after squid.conf line is fully parsed.

5 years agoFix tls-min-version= being ignored
Amos Jeffries [Sat, 10 Nov 2018 04:00:12 +0000 (17:00 +1300)] 
Fix tls-min-version= being ignored

Audit required change to make PeerOptions::parse() call
parseOptions() when 'options=' altered sslOptions instead of
delaying the parse to context creation.
This missed the fact that for GnuTLS the tlsMinVersion was
also updating the sslOptions string rather than the
parsedOptions variable later in the configuration process.

Call parseOptions() to reset the parsedOptions value whenever
sslOptions string is altered.

5 years agoBug 4912: same-name notes being appended instead of replaced (#393)
Amish [Wed, 22 May 2019 05:42:06 +0000 (05:42 +0000)] 
Bug 4912: same-name notes being appended instead of replaced (#393)

When auth helper replies with clt_conn_tag=foo (or any KV pair) and
then later in future replaces it with clt_conn_tag=bar. The new pair
was getting appended instead of getting replaced.

i.e. notes would contain two KV pairs:
clt_conn_tag=foo
clt_conn_tag=bar

This fixes it and also fixes:
https://bugs.squid-cache.org/show_bug.cgi?id=4912

5 years agoReplace uudecode with libnettle base64 decoder (#406)
Amos Jeffries [Tue, 21 May 2019 21:31:31 +0000 (21:31 +0000)] 
Replace uudecode with libnettle base64 decoder (#406)

Since RFC 7235 updated the HTTP Authentication credentials token
to the token68 characterset it is possible that characters
uudecode cannot cope with are received.

The Nettle decoder better handles characters which are valid but
not to be used for Basic auth token.

5 years agoBug 4936: terminating c-strings beyond BASE64_DECODE_LENGTH (#409)
Amos Jeffries [Tue, 21 May 2019 05:02:06 +0000 (05:02 +0000)] 
Bug 4936: terminating c-strings beyond BASE64_DECODE_LENGTH (#409)

5 years agoESI: throw on large stack recursions (#408)
Amos Jeffries [Mon, 20 May 2019 11:23:13 +0000 (11:23 +0000)] 
ESI: throw on large stack recursions (#408)

This reduces the impact on concurrent clients to only those
accessing the malformed resource.

Depending on what type of recursion is being performed the
resource may appear to the client with missing segments, or
not at all.

5 years agoBug 4937: cachemgr.cgi: unallocated memory access (#407)
Amos Jeffries [Sat, 18 May 2019 17:02:33 +0000 (17:02 +0000)] 
Bug 4937: cachemgr.cgi: unallocated memory access (#407)

... after base64_decode_update

Ensure that a terminator exists for the decoded string before
using str*() syscalls.

5 years agoRFC 7230 forbids generation of userinfo subcomponent of https URL (#405)
Amos Jeffries [Thu, 16 May 2019 15:22:54 +0000 (15:22 +0000)] 
RFC 7230 forbids generation of userinfo subcomponent of https URL (#405)

5 years agoIgnore ECONNABORTED in accept(2) (#404)
Matthieu Herrb [Mon, 13 May 2019 08:45:57 +0000 (08:45 +0000)] 
Ignore ECONNABORTED in accept(2) (#404)

An aborted connection attempt does not affect listening socket's
ability to accept other connections. If the error is not ignored, Squid
gets stuck after logging an oldAccept error like this one:

    oldAccept ...: (53) Software caused connection abort

This bug fix was motivated by accept(2) changes in OpenBSD v6.5 that
resulted in new ECONNABORTED errors under regular deployment conditions:
https://github.com/openbsd/src/commit/c255b5a