Prohibit bad --enable-linux-netfilter combinations (#1893)

Since 2009 commit 51f4d36, 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

@@ -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 */
 

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_;
 };

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 \

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 \