www/squid - Host header forgery extra patch not compatible with Squid 5

marinbernard-pep06 commented 1 year ago

Overview

This issue is linked to issue #6070 in the core repository.

www/squid was recently updated from version 4.15 to version 5.7. Since version 3, the port includes an extra patch to disable host header forgery checks with SSL requests. Due to upstream changes, this patch is not compatible anymore with Squid 5.

Fuzzing issue

The current patch is a standard unified diff with 3 lines of context targeting the ClientRequestContext::hostHeaderIpVerify() function. Since the expected context does not exist anymore due to upstream changes, the patch utility switches to fuzz mode and tries to find a fallback context to apply the diff. As the 3 context lines provided in the patch file are pretty generic, a matching alternative is quickly found in the clientFollowXForwardedForCheck() function, which is defined a few lines above the original one. As a consequence, the patch is not applied to the intended code region, with unknown consequences.

Resolution

I have a working patch for Squid 5 (built and tested on a 22.7.5 VM). I'll try to open a pull request soon to have it merged ASAP.

fichtner commented 1 year ago

As the 3 context lines provided in the patch file are pretty generic

What are the odds? I've never heard this in my entire working life that a patch got applied in fuzz mode and effortlessly compiles. ;)

marinbernard-pep06 commented 1 year ago

What are the odds? I've never heard this in my entire working life that a patch got applied in fuzz mode and effortlessly compiles. ;)

Here's what happens with the current (master) version of the ports tree.

# pwd
/usr/ports/www/squid

# make clean
===>  Cleaning for squid-5.7

# make extract
===>  License GPLv2 accepted by the user
===>   squid-5.7 depends on file: /usr/local/sbin/pkg - found
===> Fetching all distfiles required by squid-5.7 for building
===>  Extracting for squid-5.7
=> SHA256 Checksum OK for squid-5.7.tar.xz.

# cp \
    /usr/obj/usr/ports/www/squid/work/squid-5.7/src/client_side_request.cc \
    /tmp/client_side_request.cc.orig

# make patch
===>  Patching for squid-5.7
===>  Applying extra patch /usr/ports/www/squid/files/extra-patch-host-header-forgery
===>  Applying FreeBSD patches for squid-5.7 from /usr/ports/www/squid/files

# diff -U10 \
    /tmp/client_side_request.cc.orig \
    /usr/obj/usr/ports/www/squid/work/squid-5.7/src/client_side_request.cc

--- /tmp/client_side_request.cc.orig    2022-10-10 14:21:05.956297000 +0200
+++ /usr/obj/usr/ports/www/squid/work/squid-5.7/src/client_side_request.cc      2022-10-10 14:21:42.311775000 +0200
@@ -482,20 +482,23 @@
             request->indirect_client_addr = addr;
             request->x_forwarded_for_iterator.cut(l);
             calloutContext->acl_checklist = clientAclChecklistCreate(Config.accessList.followXFF, http);
             if (!Config.onoff.acl_uses_indirect_client) {
                 /* override the default src_addr tested if we have to go deeper than one level into XFF */
                 Filled(calloutContext->acl_checklist)->src_addr = request->indirect_client_addr;
             }
             calloutContext->acl_checklist->nonBlockingCheck(clientFollowXForwardedForCheck, data);
             return;
         }
+        http->request->flags.hostVerified = true;
+        http->doCallouts();
+        return;
     }

     /* clean up, and pass control to clientAccessCheck */
     if (Config.onoff.log_uses_indirect_client) {
         /*
         * Ensure that the access log shows the indirect client
         * instead of the direct client.
         */
         http->al->cache.caddr = request->indirect_client_addr;
         if (ConnStateData *conn = http->getConn())

Line 482 is not part of the targeted function, but of clientFollowXForwardedForCheck(), which is probably never invoked unless an X-Forwarded-For header needs to be validated.

Since the http variable is declared and assigned in both functions, the compiler won't complain about it. It may not cause any harm beyond flagging the remote host IP as verified in the middle of processing an X-Forwarded-For header.

The problem with this patch is that with max fuzz = 2, the original patch:

--- src/client_side_request.cc.orig 2019-02-19 02:46:22 UTC
+++ src/client_side_request.cc
@@ -538,6 +538,9 @@ ClientRequestContext::hostHeaderIpVerify
             }
             debugs(85, 3, HERE << "validate IP " << clientConn->local << " non-match from Host: IP " << ia->in_addrs[i]);
         }
+        http->request->flags.hostVerified = true;
+        http->doCallouts();
+        return;
     }
     debugs(85, 3, HERE << "FAIL: validate IP " << clientConn->local << " possible from Host:");
     hostHeaderVerifyFailed("local IP", "any domain IP");

becomes:

--- src/client_side_request.cc.orig 2019-02-19 02:46:22 UTC
+++ src/client_side_request.cc
@@ -538,6 +538,9 @@ ClientRequestContext::hostHeaderIpVerify
         }
+        http->request->flags.hostVerified = true;
+        http->doCallouts();
+        return;
     }

So basically, the patch will fallback to the nearest location with a couple of closing brackets with a matching indentation.

fichtner commented 1 year ago

Did this slightly different since I had to see the 4.x and 5.x side to side anyway, see https://github.com/opnsense/ports/commit/513869b824d8

In any case thanks A LOT for the report and analysis!

Cheers, Franco

opnsense / ports