Merge pull request #31 from 39ff/claude/security-review-33NY5

Add input validation and improve SOCKS5 authentication security

Author: 39ff

setup/generate.php

@@ -19,15 +19,52 @@
 // the connection is direct to the target (not an HTTP proxy).
 $squid_socks = 'cache_peer %s parent %d 0 no-digest no-netdb-exchange connect-fail-limit=2 connect-timeout=8 round-robin no-query allow-miss proxy-only originserver name=%s %s';
 
+// Reject any byte that could break squid.conf tokenization or inject
+// a new directive (whitespace, control chars, quotes, backslash, '#').
+// Applied to host/user/pass to prevent config injection from a tainted
+// proxyList.txt (e.g. one fetched from a remote URL).
+$reject_unsafe = '/[\s"#\\\\\x00-\x1F\x7F]/';
+
 while ($line = fgets($proxies)){
     $line = trim($line);
+    if ($line === '' || $line[0] === '#') {
+        continue;
+    }
     $proxyInfo = array_combine($keys, array_pad((explode(":", $line, 5)), 5, ''));
     $squid_conf = [];
     $cred = '';
     if(!$proxyInfo['host'] && !$proxyInfo['port']){
         continue;
     }
 
+    // Validate host: hostname or IPv4 literal. No shell/conf metachars.
+    // IPv6 literals are not supported: the naive explode(":") parser above
+    // cannot split "[::1]:8080:..." correctly, so ':' / '[' / ']' are rejected
+    // to avoid giving the impression that IPv6 is accepted.
+    if (preg_match($reject_unsafe, $proxyInfo['host']) ||
+        !preg_match('/^[A-Za-z0-9._-]+$/', $proxyInfo['host'])) {
+        fwrite(STDERR, "Skipping proxy with invalid host: " . rawurlencode($proxyInfo['host']) . PHP_EOL);
+        continue;
+    }
+    // Validate port: 1-65535.
+    if (!ctype_digit((string)$proxyInfo['port']) ||
+        (int)$proxyInfo['port'] < 1 || (int)$proxyInfo['port'] > 65535) {
+        fwrite(STDERR, "Skipping proxy with invalid port: " . rawurlencode((string)$proxyInfo['port']) . PHP_EOL);
+        continue;
+    }
+    // Validate credentials: no whitespace/control chars/quotes/backslash/#.
+    // (SOCKS5 RFC1929 allows up to 255 bytes of arbitrary octets, but we
+    //  conservatively reject bytes that would break squid.conf.)
+    if (($proxyInfo['user'] !== '' && preg_match($reject_unsafe, $proxyInfo['user'])) ||
+        ($proxyInfo['pass'] !== '' && preg_match($reject_unsafe, $proxyInfo['pass']))) {
+        fwrite(STDERR, "Skipping proxy with unsafe characters in credentials: " . $proxyInfo['host'] . PHP_EOL);
+        continue;
+    }
+    if (strlen($proxyInfo['user']) > 255 || strlen($proxyInfo['pass']) > 255) {
+        fwrite(STDERR, "Skipping proxy with credentials exceeding 255 bytes: " . $proxyInfo['host'] . PHP_EOL);
+        continue;
+    }
+
     if(!$proxyInfo['scheme']){
         //open proxy server IP:Port Pattern.
         //No create gost

squid_patch/src/SocksPeerConnector.h

@@ -127,20 +127,15 @@ static inline bool socks5Connect(int fd,
     const bool hasAuth = (!user.empty() && !pass.empty());
 
     /* --- greeting ---------------------------------------------------- */
-    uint8_t greeting[4];
-    size_t gLen;
-    if (hasAuth) {
-        greeting[0] = 0x05;   /* VER                     */
-        greeting[1] = 0x02;   /* NMETHODS                */
-        greeting[2] = 0x00;   /* NO AUTHENTICATION       */
-        greeting[3] = 0x02;   /* USERNAME / PASSWORD      */
-        gLen = 4;
-    } else {
-        greeting[0] = 0x05;
-        greeting[1] = 0x01;
-        greeting[2] = 0x00;
-        gLen = 3;
-    }
+    /* When credentials are configured, advertise ONLY USER/PASS to
+     * prevent a rogue SOCKS5 server from silently downgrading to
+     * "no authentication" and accepting traffic without verifying
+     * the credentials the operator explicitly provided. */
+    uint8_t greeting[3];
+    greeting[0] = 0x05;       /* VER      */
+    greeting[1] = 0x01;       /* NMETHODS */
+    greeting[2] = hasAuth ? 0x02 : 0x00;  /* USER/PASS or NO AUTH */
+    const size_t gLen = 3;
 
     if (!syncSend(fd, greeting, gLen))
         return false;
@@ -182,7 +177,11 @@ static inline bool socks5Connect(int fd,
             return false;   /* auth failed or wrong sub-negotiation version */
 
     } else if (gResp[1] == 0x00) {
-        /* no auth required */
+        /* Server chose "no authentication". Only accept this when the
+         * operator did NOT configure credentials; otherwise the server
+         * is downgrading away from the authentication we required. */
+        if (hasAuth)
+            return false;
     } else {
         return false;       /* unsupported or unacceptable method (includes 0xFF) */
     }