]> git.ipfire.org Git - thirdparty/squid.git/commit - include/splay.h
Fix dupe handling in Splay ACLs: src, dst, http_status, etc. (#1632)
authorAlex Rousskov <rousskov@measurement-factory.com>
Thu, 11 Jan 2024 07:10:15 +0000 (07:10 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Thu, 11 Jan 2024 10:15:05 +0000 (10:15 +0000)
commit0898d0f4c23df6854c035919c5480e87f62b5784
treee6364ac83b684c85638b5c69e9f79190f754e383
parent02c8fd1ccfd925a937762ca2bd6015fa397d94b8
Fix dupe handling in Splay ACLs: src, dst, http_status, etc. (#1632)

Squid was dangerously mishandling duplicate[^1] values in ACLs with
Splay tree storage: src, dst, localip, http_status, dstdomain,
srcdomain, and ssl::server_name. These problems were all rooted in the
same old code but had diverged across two ACL groups, as detailed in the
corresponding sections below. Fortunately, all known problems were
accompanied with Squid cache.log WARNINGs: Squid configurations that do
not emit cited or similar warnings are not affected by these bugs.

[^1]: Squid Splay trees can only store unique values. Uniqueness is
defined in terms of a configurable comparison function that returns zero
when the two values are "not unique" (i.e. are considered to be
"duplicated" or "equal" in that Splay context). Those two "equal" values
may differ a lot in other contexts! For example, the following two
status code ranges are equal from acl_httpstatus_data::compare() point
of view, but are obviously very different from an admin and http_access
rules points of view: 200-200 and 200-299.

### Group 1: src, dst, localip, and http_status ACLs

These ACLs ignored (i.e. never matched) some configured ACL values in
problematic use cases. They also gave wrong "remove X" advice and
incorrectly classified values as being subranges or subsets:

    Processing: acl testA http_status 200 200-300
    WARNING: '200-300' is a subrange of '200'
    WARNING: because of this '200-300' is ignored ...

    Processing: acl testB http_status 300-400 200-300
    WARNING: '200-300' is a subrange of '300-400'
    WARNING: because of this '200-300' is ignored ...
    WARNING: You should probably remove '300-400' from the ACL...

    Processing: acl testC src 10.0.0.1  10.0.0.0-10.0.0.255
    WARNING: (B) 10.0.0.1 is a subnetwork of (A) 10.0.0.0-10.0.0.255
    WARNING: because of this 10.0.0.0-10.0.0.255 is ignored...

    Processing: acl testD src 10.0.0.0-10.0.0.1  10.0.0.1-10.0.0.255
    WARNING: (A) 10.0.0.1-10.0.0.255 is a subnetwork of (B) 10.0.0.0-...
    WARNING: because of this 10.0.0.1-10.0.0.255 is ignored...
    WARNING: You should probably remove 10.0.0.1-10.0.0.255 from the ACL

Since 2002 commit 96da6e8, IP-based ACLs like src, dst, and localip
eliminate duplicates[^1] among configured ACL values. That elimination
code was buggy since inception. Those bugs were later duplicated in
http_status code (2005 commit a0ec9f6). This change fixes those bugs.

To correctly eliminate duplicates when facing two (fully or partially)
overlapping ranges -- new A and old B -- we must pick the right
corrective action depending on the kind of overlap:

    * A is a subrange of B: Ignore fully duplicated new range A. Keep B.
    * B is a subrange of A: Remove fully duplicated old range B. Add A.
    * A partially overlaps with B: Add a union of A and B. Remove B.

Both acl_httpstatus_data::compare() and acl_ip_data::NetworkCompare()
mishandled the last two cases because these functions effectively
implemented the following buggy logic instead:

    - in all three cases: Ignore new range A. Keep B.

Their WARNINGs also suggested wrong corrective actions in two mishandled
cases (see the last WARNING lines in testB and testD output above).

### Group 2: dstdomain, srcdomain, and ssl::server_name ACLs

    Processing: acl testE dstdomain .example.com example.com
    ERROR: '.example.com' is a subdomain of 'example.com'
    ERROR: You need to remove '.example.com' from the ACL named 'testE'

2002 commit 96da6e8 bugs mentioned in Group 1 section stopped affecting
domain-based ACLs (i.e. dstdomain, srcdomain, and ssl::server_name) in
2011 commit 14e563c that adjusted aclDomainCompare() to self_destruct()
in problematic cases. However, that adjustment emitted wrong advice and
incorrect subdomain classification in cases where strlen() checks do not
work for determining which of the two configured ACL values should be
removed (see testE ERRORs above).

This change improves that handling by replacing the call to
self_destruct() with proper duplicate[^1] resolution code (and fixing
cache.log messages). We (now) support duplicate values instead of
rejecting configurations containing them because duplicate values do not
invalidate an ACL -- an ACL with duplicates could match as expected. It
may be difficult for some admins to avoid duplication, especially when
ACL values come from multiple sources. Squid should continue to warn
about duplicates (because they waste resources and may indicate a deeper
problem), but killing Squid or otherwise rejecting ACLs with duplicates
is bad UX.

N.B. Domain-based ACLs use sets of values rather than "ranges" discussed
in Group 1 section, but domain sets follow the same basic principles.

### All of the above seven ACLs

The problems in both ACL groups were fixed by factoring out "insert ACL
values while correctly handling duplicates" algorithm (see the three
bullets in Group 1 section) into Acl::SplayInserter::Merge() function.

Without duplicates, the new ACL value insertion code has the same cost
as the old one. The vast majority of duplicate cases incur a constant
additional overhead because Splay tree dynamic reorganization makes the
right tree nodes immediately available. A few cases duplicate double the
number of comparisons during tree searches (when Splay reorganization
cannot cope with a particularly "bad" ACL value order), but since these
"bad" cases ought to be very rare, and since all problematic cases are
accompanied by WARNINGs, this extra cost is deemed acceptable.

This change also fixes memory leaks associated with ignored ACL values.

Also added test-suite/test-squid-conf.sh support for matching multiple
stderr messages. Without this, testing a variety of closely-related
cases requires creating lots of test configuration files (two per test
case), increasing noise and, more importantly, making it difficult to
handle related test cases as one coherent collection. The new
"expect-messages" feature is barely sufficient for testing these
changes, but we are now at (or perhaps even beyond) the limit of what
can be reasonably done using shell scripts and test instruction files:
The next step is to convert instruction files (and likely some test
cases themselves!) to scripts written in Perl or a better language.
19 files changed:
include/splay.h
src/acl/Acl.cc
src/acl/DomainData.cc
src/acl/HttpStatus.cc
src/acl/HttpStatus.h
src/acl/Ip.cc
src/acl/Ip.h
src/acl/Makefile.am
src/acl/SplayInserter.h [new file with mode: 0644]
src/ip/Address.cc
src/ip/Address.h
test-suite/Makefile.am
test-suite/squidconf/bad-acl-dstdomain-dupe.conf [new file with mode: 0644]
test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions [new file with mode: 0644]
test-suite/squidconf/bad-acl-http-status-dupe.conf [new file with mode: 0644]
test-suite/squidconf/bad-acl-http-status-dupe.conf.instructions [new file with mode: 0644]
test-suite/squidconf/bad-acl-src-dupe.conf [new file with mode: 0644]
test-suite/squidconf/bad-acl-src-dupe.conf.instructions [new file with mode: 0644]
test-suite/test-squid-conf.sh