From c2c5637bd23fa4afbf3d951987c26bab5af1410f Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 5 Oct 2024 16:18:33 +0000 Subject: [PATCH] Prohibit bad --enable-linux-netfilter combinations (#1893) 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 combinations (if combinations are supported at all), and LookupNat() will only use configured method(s). --- src/ip/Intercept.cc | 35 +++++++++++-------- src/ip/Intercept.h | 2 ++ test-suite/buildtests/layer-02-maximus.opts | 1 - .../layer-04-noauth-everything.opts | 1 - 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/src/ip/Intercept.cc b/src/ip/Intercept.cc index f98fe428a39..1a7bc80d482 100644 --- a/src/ip/Intercept.cc +++ b/src/ip/Intercept.cc @@ -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 */ diff --git a/src/ip/Intercept.h b/src/ip/Intercept.h index f47b810d56b..0913f18161a 100644 --- a/src/ip/Intercept.h +++ b/src/ip/Intercept.h @@ -117,6 +117,8 @@ class Intercept */ bool PfInterception(const Comm::ConnectionPointer &newConn); + bool UseInterceptionAddressesLookedUpEarlier(const char *, const Comm::ConnectionPointer &); + int transparentActive_; int interceptActive_; }; diff --git a/test-suite/buildtests/layer-02-maximus.opts b/test-suite/buildtests/layer-02-maximus.opts index 56e75bdf686..ce03ab5350b 100644 --- a/test-suite/buildtests/layer-02-maximus.opts +++ b/test-suite/buildtests/layer-02-maximus.opts @@ -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 \ diff --git a/test-suite/buildtests/layer-04-noauth-everything.opts b/test-suite/buildtests/layer-04-noauth-everything.opts index b5ffd8a0fc2..f89911b3862 100644 --- a/test-suite/buildtests/layer-04-noauth-everything.opts +++ b/test-suite/buildtests/layer-04-noauth-everything.opts @@ -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 \