]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Prohibit bad --enable-linux-netfilter combinations (#1893)
authorAlex Rousskov <rousskov@measurement-factory.com>
Sat, 5 Oct 2024 16:18:33 +0000 (16:18 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Sun, 6 Oct 2024 22:29:36 +0000 (22:29 +0000)
Since 2009 commit 51f4d36b, Squid declared that "all transparent build
options are independent, and may be used in any combination". That
declaration was accurate from a "successful build" point of view, but
Ip::Intercept::LookupNat() (and its precursors) started mishandling at
least two combinations of options as detailed below.

LookupNat() tries up to four lookup methods (until one succeeds):

1. NetfilterInterception(): --enable-linux-netfilter; SO_ORIGINAL_DST
2. IpfwInterception(): --enable-ipfw-transparent; getsockname()
3. PfInterception(): --enable-pf-transparent; getsockname() or /dev/pf
4. IpfInterception(newConn): --enable-ipf-transparent; ioctl(SIOCGNATL)

The first method -- NetfilterInterception() -- fails to look up the true
destination address of an intercepted connection when, for example, the
client goes away just before the lookup. In those (relatively common in
busy environments) cases, the intended destination address cannot be
obtained via getsockname(), but LookupNat() proceeds calling other
methods, including the two methods that may rely on getsockname().

Methods 2 and 3 may rely on a previous getsockname() call to provide
true destination address, but getsockname() answers are not compatible
with what NetfilterInterception() must provide -- the destination
address returned by getsockname() is Squid's own http(s)_port address.
When Squid reaches problematic code now encapsulated in a dedicated
UseInterceptionAddressesLookedUpEarlier() function, Squid may end up
connecting to its own http(s)_port! Such connections may cause
forwarding loops and other problems. In SslBump deployments, these loops
form _before_ Forwarded-For protection can detect and break them.

These problems can be prevented if an admin does not enable incompatible
combinations of interception lookup methods. The relevant code is
correctly documented as "Trust the user configured properly", but that
statement essentially invalidates our "may be used in any combination"
design assertion and leads to runtime failures when user configured
improperly. Those runtime failures are difficult to triage because they
lack signs pointing to a build misconfiguration.

This change bans incompatible NetfilterInterception()+getsockname()
combinations at compile time: Squid ./configured with
--enable-linux-netfilter cannot use --enable-ipfw-transparent or
(--enable-pf-transparent --without-nat-devpf).

TODO: Ban incompatible combinations at ./configure time as well!

We have considered and rejected an alternative solution where all
./configure option combinations are still allowed, but LookupNat()
returns immediately on NetfilterInterception()/SO_ORIGINAL_DST failures.
That solution is inferior to build-time bans because an admin may think
that their Squid uses other configured lookup method(s) if/as needed,
but Squid would never reach them in --enable-linux-netfilter builds.

The only viable alternative is to specify lookup methods in squid.conf,
similar to the existing "tproxy" vs. "intercept" http(s)_port modes. In
that case, squid.conf will be validated for incompatible method
combinations (if combinations are supported at all), and LookupNat()
will only use configured method(s).

src/ip/Intercept.cc
src/ip/Intercept.h
test-suite/buildtests/layer-02-maximus.opts
test-suite/buildtests/layer-04-noauth-everything.opts

index f98fe428a39a0f3c8f0b173d420c71ca74544090..1a7bc80d482c52a5187439eeeb71847419e5821e 100644 (file)
@@ -182,18 +182,32 @@ bool
 Ip::Intercept::IpfwInterception(const Comm::ConnectionPointer &newConn)
 {
 #if IPFW_TRANSPARENT
-    /* The getsockname() call performed already provided the TCP packet details.
-     * There is no way to identify whether they came from NAT or not.
-     * Trust the user configured properly.
-     */
-    debugs(89, 5, "address NAT: " << newConn);
-    return true;
+    return UseInterceptionAddressesLookedUpEarlier(__FUNCTION__, newConn);
 #else
     (void)newConn;
     return false;
 #endif
 }
 
+/// Assume that getsockname() has been called already and provided the necessary
+/// TCP packet details. There is no way to identify whether they came from NAT.
+/// Trust the user configured properly.
+bool
+Ip::Intercept::UseInterceptionAddressesLookedUpEarlier(const char * const caller, const Comm::ConnectionPointer &newConn)
+{
+    // paranoid: ./configure should prohibit these combinations
+#if LINUX_NETFILTER && PF_TRANSPARENT && !USE_NAT_DEVPF
+    static_assert(!"--enable-linux-netfilter is incompatible with --enable-pf-transparent --without-nat-devpf");
+#endif
+#if LINUX_NETFILTER && IPFW_TRANSPARENT
+    static_assert(!"--enable-linux-netfilter is incompatible with --enable-ipfw-transparent");
+#endif
+    // --enable-linux-netfilter is compatible with --enable-ipf-transparent
+
+    debugs(89, 5, caller << " uses " << newConn);
+    return true;
+}
+
 bool
 Ip::Intercept::IpfInterception(const Comm::ConnectionPointer &newConn)
 {
@@ -313,14 +327,7 @@ Ip::Intercept::PfInterception(const Comm::ConnectionPointer &newConn)
 #if PF_TRANSPARENT  /* --enable-pf-transparent */
 
 #if !USE_NAT_DEVPF
-    /* On recent PF versions the getsockname() call performed already provided
-     * the required TCP packet details.
-     * There is no way to identify whether they came from NAT or not.
-     *
-     * Trust the user configured properly.
-     */
-    debugs(89, 5, "address NAT divert-to: " << newConn);
-    return true;
+    return UseInterceptionAddressesLookedUpEarlier("recent PF version", newConn);
 
 #else /* USE_NAT_DEVPF / --with-nat-devpf */
 
index f47b810d56baf258157e4485292f3e2305306789..0913f18161a231789a59be22581341313cc7ea5b 100644 (file)
@@ -117,6 +117,8 @@ private:
      */
     bool PfInterception(const Comm::ConnectionPointer &newConn);
 
+    bool UseInterceptionAddressesLookedUpEarlier(const char *, const Comm::ConnectionPointer &);
+
     int transparentActive_;
     int interceptActive_;
 };
index 56e75bdf686730b3835330936a1714454726c06b..ce03ab5350b0a53cb843dfd1d5ced06f6e05d34a 100644 (file)
@@ -79,7 +79,6 @@ DISTCHECK_CONFIGURE_FLAGS=" \
        --enable-poll \
        --enable-select \
        --enable-http-violations \
-       --enable-ipfw-transparent \
        --enable-follow-x-forwarded-for \
        --enable-default-hostsfile=/etc/hosts \
        --enable-auth \
index b5ffd8a0fc24f23a27da3504797dbac53badb9b8..f89911b3862d93da00531de9e1bb837ce791c450 100644 (file)
@@ -80,7 +80,6 @@ DISTCHECK_CONFIGURE_FLAGS=" \
        --enable-poll \
        --enable-select \
        --enable-http-violations \
-       --enable-ipfw-transparent \
        --enable-follow-x-forwarded-for \
        --enable-internal-dns \
        --enable-default-hostsfile \