Add SOCKS5 anti-reuse guard to prevent cross-target routing

.github/workflows/squid-build-test.yml

@@ -444,6 +444,140 @@ jobs:
           pkill microsocks 2>/dev/null || true
           pkill -f 'python3 -m http.server' 2>/dev/null || true
 
+  # ------------------------------------------------------------------
+  # 4b. Test: SOCKS5 connection reuse / no cross-target routing
+  #     Regression for codex P1/P2: a SOCKS tunnel bound to target X must
+  #     never be reused for target Y, even with server pconns ENABLED.
+  # ------------------------------------------------------------------
+  test-socks5-reuse:
+    name: Test SOCKS5 No Cross-Target Reuse
+    needs: build
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Download image artifact
+        uses: actions/download-artifact@v4
+        with:
+          name: squid-image
+          path: /tmp
+
+      - name: Load image
+        run: docker load -i /tmp/squid-image.tar
+
+      - name: Start two distinct origin servers (A and B)
+        run: |
+          mkdir -p /tmp/wwwA /tmp/wwwB
+          echo 'ORIGIN-AAAA' > /tmp/wwwA/ip
+          echo 'ORIGIN-BBBB' > /tmp/wwwB/ip
+          python3 -m http.server 18090 --directory /tmp/wwwA &
+          python3 -m http.server 18091 --directory /tmp/wwwB &
+          sleep 1
+          curl -sf http://127.0.0.1:18090/ip | grep -q ORIGIN-AAAA
+          curl -sf http://127.0.0.1:18091/ip | grep -q ORIGIN-BBBB
+          echo "--- Origins A/B ready ---"
+
+      - name: Build and start SOCKS5 test server (microsocks)
+        run: |
+          if [ ! -f /tmp/microsocks/microsocks ]; then
+            git clone --depth 1 https://github.com/rofl0r/microsocks.git /tmp/microsocks
+            cd /tmp/microsocks && make -j"$(nproc)"
+          fi
+          /tmp/microsocks/microsocks -p 11090 &
+          sleep 1
+          curl -sf --socks5-hostname 127.0.0.1:11090 http://127.0.0.1:18090/ip | grep -q ORIGIN-AAAA
+          curl -sf --socks5-hostname 127.0.0.1:11090 http://127.0.0.1:18091/ip | grep -q ORIGIN-BBBB
+          echo "--- SOCKS5 server reaches both origins ---"
+
+      - name: Create Squid config with server pconns ENABLED
+        run: |
+          mkdir -p /tmp/squid-conf-reuse
+          cat > /tmp/squid-conf-reuse/squid.conf <<CONF
+          http_port 3128
+          http_access allow all
+          never_direct allow all
+          server_persistent_connections on
+          client_persistent_connections on
+          cache_peer 127.0.0.1 parent 11090 0 no-query no-digest connect-fail-limit=2 connect-timeout=8 round-robin proxy-only originserver name=socks_reuse socks5
+          visible_hostname test
+          cache deny all
+          pid_filename /var/run/squid/squid.pid
+          cache_log /var/log/squid/cache.log
+          access_log stdio:/var/log/squid/access.log combined
+          CONF
+          echo "--- Config (server_persistent_connections ON) ---"
+          cat /tmp/squid-conf-reuse/squid.conf
+
+      - name: Start Squid
+        run: |
+          docker run -d --name squid-reuse --network host \
+            -v /tmp/squid-conf-reuse:/etc/squid/conf.d:ro \
+            -e SQUID_CONFIG_FILE=/etc/squid/conf.d/squid.conf \
+            ${{ env.SQUID_IMAGE }}
+          sleep 5
+          if ! docker ps --filter name=squid-reuse --filter status=running -q | grep -q .; then
+            echo "ERROR: Squid container is not running"
+            docker logs squid-reuse 2>&1 || true
+            exit 1
+          fi
+
+      - name: Wait for Squid to listen
+        run: |
+          for i in $(seq 1 30); do
+            if bash -c 'echo > /dev/tcp/127.0.0.1/3128' 2>/dev/null; then
+              echo "Squid is listening after ${i}s"
+              exit 0
+            fi
+            sleep 1
+          done
+          echo "ERROR: Squid never started listening"
+          docker logs squid-reuse 2>&1 || true
+          exit 1
+
+      - name: Alternate A/B requests and assert no cross-target reuse
+        run: |
+          fail=0
+          for i in 1 2 3 4; do
+            RA=$(curl -s --max-time 15 -x http://127.0.0.1:3128 http://127.0.0.1:18090/ip || true)
+            RB=$(curl -s --max-time 15 -x http://127.0.0.1:3128 http://127.0.0.1:18091/ip || true)
+            echo "iter ${i}: A='${RA}' B='${RB}'"
+            echo "${RA}" | grep -q 'ORIGIN-AAAA' || { echo "FAIL: target A returned '${RA}'"; fail=1; }
+            echo "${RB}" | grep -q 'ORIGIN-BBBB' || { echo "FAIL: cross-target reuse! target B returned '${RB}'"; fail=1; }
+          done
+          echo "=== Squid logs ==="
+          docker logs squid-reuse 2>&1 | tail -40 || true
+          [ "${fail}" -eq 0 ] || { echo "--- Cross-target reuse detected ---"; exit 1; }
+          echo "--- No cross-target reuse: OK ---"
+
+      - name: Post Squid logs to PR on failure
+        if: failure()
+        uses: actions/github-script@v7
+        with:
+          script: |
+            const { execSync } = require('child_process');
+            let logs = '';
+            try { logs = execSync('docker logs squid-reuse 2>&1', {encoding: 'utf8', maxBuffer: 50*1024}); } catch(e) { logs = e.stdout || e.message; }
+            const body = `### Reuse Test Squid Logs\n\`\`\`\n${logs.slice(-3000)}\n\`\`\``;
+            const issueNumber = context.issue.number;
+            if (!issueNumber) {
+              console.log('No PR context; skipping comment.');
+            } else {
+              await github.rest.issues.createComment({
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                issue_number: issueNumber,
+                body: body
+              });
+            }
+
+      - name: Cleanup
+        if: always()
+        run: |
+          docker rm -f squid-reuse 2>/dev/null || true
+          pkill microsocks 2>/dev/null || true
+          pkill -f 'python3 -m http.server' 2>/dev/null || true
+
   # ------------------------------------------------------------------
   # 5. Test: generate.php produces correct config
   # ------------------------------------------------------------------

squid_patch/Dockerfile

@@ -30,13 +30,28 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
 
 WORKDIR /build
 
-# Download and extract Squid source (pinned version + integrity check)
+# Download and extract Squid source (pinned version + integrity check).
+# Fetch from several mirrors with retries: the primary www.squid-cache.org
+# host has intermittent outages.  Integrity is guaranteed by the SHA256 check
+# regardless of which mirror served the bytes, so mirror order is safe.
 ARG SQUID_TAR_SHA256=0b07b187e723f04770dd25beb89aec12030a158696aa8892d87c8b26853408a7
-RUN wget -q "https://www.squid-cache.org/Versions/v6/squid-${SQUID_VERSION}.tar.xz" \
-        -O squid.tar.xz \
-    && echo "${SQUID_TAR_SHA256}  squid.tar.xz" | sha256sum -c - \
-    && tar xf squid.tar.xz \
-    && rm squid.tar.xz
+RUN set -eux; \
+    SQUID_TAG="SQUID_$(echo "${SQUID_VERSION}" | tr . _)"; \
+    for url in \
+        "https://github.com/squid-cache/squid/releases/download/${SQUID_TAG}/squid-${SQUID_VERSION}.tar.xz" \
+        "https://www.squid-cache.org/Versions/v6/squid-${SQUID_VERSION}.tar.xz" \
+        "http://www.squid-cache.org/Versions/v6/squid-${SQUID_VERSION}.tar.xz" \
+    ; do \
+        echo "Fetching Squid source: ${url}"; \
+        if wget -q --tries=3 --timeout=30 -O squid.tar.xz "${url}"; then \
+            echo "Downloaded from ${url}"; \
+            break; \
+        fi; \
+        rm -f squid.tar.xz; \
+    done; \
+    echo "${SQUID_TAR_SHA256}  squid.tar.xz" | sha256sum -c -; \
+    tar xf squid.tar.xz; \
+    rm squid.tar.xz
 
 # Copy patch sources and apply
 COPY src/    /patches/src/

squid_patch/patch_apply.sh

@@ -69,6 +69,70 @@ fi
 
 echo "    CachePeer.cc patched OK"
 
+# ---------------------------------------------------------------------------
+# 1c. comm/Connection.h  –  add socksNegotiated flag (SOCKS anti-reuse guard)
+# ---------------------------------------------------------------------------
+# A SOCKS-negotiated cache_peer connection is a raw tunnel bound to ONE target
+# (request->url.host():port).  The persistent-connection pool is keyed by peer,
+# NOT target, so a pooled SOCKS connection could otherwise be reused for a
+# different target (silent mis-routing) or receive a second SOCKS greeting
+# injected into a live stream.  This per-connection flag lets the FwdState /
+# tunnel hooks detect and refuse such reuse.
+CONNECTION_H="${SQUID_SRC}/src/comm/Connection.h"
+echo "==> Patching ${CONNECTION_H}"
+[ -f "${CONNECTION_H}" ] || die "comm/Connection.h not found"
+
+if ! grep -q 'socksNegotiated' "${CONNECTION_H}"; then
+    python3 - "${CONNECTION_H}" << 'PYEOF'
+import sys, re
+
+filepath = sys.argv[1]
+with open(filepath, 'r') as f:
+    content = f.read()
+
+# Locate the *definition* of class Connection (skip forward declarations like
+# "class Connection;").  A definition has an opening "{" before any ";".
+match = None
+brace_start = -1
+for mm in re.finditer(r'class\s+Connection\b', content):
+    rest = content[mm.end():]
+    brace = rest.find('{')
+    semi = rest.find(';')
+    if brace != -1 and (semi == -1 or brace < semi):
+        match = mm
+        brace_start = mm.end() + brace
+        break
+
+if match is None:
+    print("ERROR: class Connection definition not found in Connection.h", file=sys.stderr)
+    sys.exit(1)
+
+# Brace-match to find the closing "}" of the class body.
+depth = 1
+pos = brace_start + 1
+while pos < len(content) and depth > 0:
+    if content[pos] == '{': depth += 1
+    elif content[pos] == '}': depth -= 1
+    pos += 1
+
+# pos is right after the closing "}"; insert the field just before it.
+field = ('\npublic:\n'
+         '    /* SOCKS peer support: true once SocksPeerConnector has\n'
+         '     * negotiated a tunnel on this fd.  Prevents a pooled SOCKS\n'
+         '     * tunnel (bound to one target) from being reused for another. */\n'
+         '    bool socksNegotiated = false;\n')
+insert_at = pos - 1
+content = content[:insert_at] + field + content[insert_at:]
+
+with open(filepath, 'w') as f:
+    f.write(content)
+print("    Inserted socksNegotiated flag into class Connection")
+PYEOF
+    grep -q 'socksNegotiated' "${CONNECTION_H}" || die "Failed to add socksNegotiated to Connection.h"
+fi
+
+echo "    comm/Connection.h patched OK"
+
 # ---------------------------------------------------------------------------
 # 2. cache_cf.cc  –  parse socks4 / socks5 / socks-user= / socks-pass=
 # ---------------------------------------------------------------------------
@@ -210,6 +274,27 @@ socks_hook = r'''
     /* SOCKS peer negotiation: after TCP connect, before HTTP dispatch */
     if (const auto sp = serverConnection()->getPeer()) {
         if (sp->socks_type) {
+            /* Anti-reuse guard: a connection that has already been
+             * SOCKS-negotiated is a pooled tunnel bound to a *previous*
+             * target.  Re-negotiating would inject a second greeting into a
+             * live stream, and using it as-is would silently mis-route this
+             * request.  Drop it and let FwdState retry on a fresh fd. */
+            if (serverConnection()->socksNegotiated) {
+                /* A reused, already-negotiated pconn is a tunnel bound to a
+                 * previous target.  Tear it down before retrying: noteConnection()
+                 * has already set destinationReceipt and syncWithServerConn() has
+                 * installed serverConn/closeHandler, so a bare retryOrBail() would
+                 * re-enter noteConnection() with destinationReceipt still set and
+                 * trip assert(!destinationReceipt).  Mirror serverClosed(). */
+                debugs(17, 2, "SOCKS: dropping reused negotiated connection to "
+                       << sp->host << "; retrying on a fresh fd");
+                closeServerConnection("reused SOCKS tunnel cannot serve a new target");
+                serverConn = nullptr;
+                destinationReceipt = nullptr;
+                retryOrBail();
+                return;
+            }
+
             /* The SOCKS tunnel is bound to (request->url.host():port).
              * The pconn pool is keyed by peer address, NOT target, so a
              * pooled SOCKS-negotiated connection would silently route the
@@ -231,9 +316,13 @@ socks_hook = r'''
                     sp->socks_user ? std::string(sp->socks_user) : std::string(),
                     sp->socks_pass ? std::string(sp->socks_pass) : std::string())) {
                 debugs(17, 2, "SOCKS negotiation FAILED for peer " << sp->host);
+                closeServerConnection("SOCKS negotiation failed");
+                serverConn = nullptr;
+                destinationReceipt = nullptr;
                 retryOrBail();
                 return;
             }
+            serverConnection()->socksNegotiated = true;
             debugs(17, 3, "SOCKS negotiation OK for peer " << sp->host);
         }
     }
@@ -299,6 +388,19 @@ socks_tunnel_hook = r'''
     /* SOCKS peer: negotiate tunnel right after TCP connect */
     if (conn->getPeer() && conn->getPeer()->socks_type) {
         const auto sp = conn->getPeer();
+        /* Anti-reuse guard: never re-negotiate (or reuse) a connection that
+         * already carries a SOCKS tunnel to a previous target. */
+        if (conn->socksNegotiated) {
+            /* Reused tunnel connection already bound to a previous target:
+             * close the pending conn (server.conn is still nil here) before
+             * retrying, matching tunnel.cc's other error paths. */
+            debugs(26, 2, "SOCKS: dropping reused negotiated tunnel connection to "
+                   << sp->host << "; retrying");
+            closePendingConnection(conn, "reused SOCKS tunnel cannot serve a new target");
+            saveError(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al));
+            retryOrBail("SOCKS tunnel reuse");
+            return;
+        }
         /* Same rationale as FwdState::dispatch(): the SOCKS tunnel is
          * bound to one target host, so prevent this connection from being
          * returned to the pconn pool where another request could pick it
@@ -316,10 +418,12 @@ socks_tunnel_hook = r'''
                 sp->socks_user ? std::string(sp->socks_user) : std::string(),
                 sp->socks_pass ? std::string(sp->socks_pass) : std::string())) {
             debugs(26, 2, "SOCKS tunnel negotiation FAILED for " << sp->host);
+            closePendingConnection(conn, "SOCKS negotiation failed");
             saveError(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al));
             retryOrBail("SOCKS negotiation failed");
             return;
         }
+        conn->socksNegotiated = true;
         debugs(26, 3, "SOCKS tunnel negotiation OK for " << sp->host);
     }
 
@@ -357,6 +461,7 @@ echo ""
 echo "Modified files:"
 echo "  - src/CachePeer.h            (added socks_type/user/pass fields)"
 echo "  - src/CachePeer.cc           (added socks_user/pass cleanup in destructor)"
+echo "  - src/comm/Connection.h      (added socksNegotiated anti-reuse flag)"
 echo "  - src/cache_cf.cc            (added socks4/socks5 option parsing)"
 echo "  - src/FwdState.cc            (SOCKS negotiation in dispatch())"
 echo "  - src/tunnel.cc              (SOCKS negotiation in connectDone())"