Add input validation and improve SOCKS5 authentication security

[39ff]

Summary

This PR adds comprehensive input validation to the proxy configuration parser and strengthens SOCKS5 authentication handling to prevent configuration injection attacks and authentication downgrade attacks.

Key Changes

setup/generate.php:

• Added input validation regex pattern ($reject_unsafe) to reject bytes that could break squid.conf tokenization or inject directives (whitespace, control chars, quotes, backslash, '#')
• Added filtering to skip empty lines and comments in proxy list
• Implemented host validation: checks for unsafe characters and ensures hostname/IPv4/IPv6 format using whitelist pattern [A-Za-z0-9.:\[\]_-]+
• Implemented port validation: ensures port is numeric and within valid range (1-65535)
• Implemented credential validation: rejects username/password containing unsafe characters and enforces 255-byte length limit per SOCKS5 RFC1929
• All validation failures now log descriptive error messages to STDERR

squid_patch/src/SocksPeerConnector.h:

• Simplified SOCKS5 greeting logic: always advertise only one authentication method instead of conditionally offering both "no auth" and "user/pass"
• When credentials are configured, advertise ONLY USER/PASS authentication (0x02) to prevent rogue servers from silently downgrading to "no authentication"
• When no credentials are configured, advertise only "no authentication" (0x00)
• Added server response validation: reject "no authentication" response if credentials were explicitly configured, preventing authentication downgrade attacks
• Added detailed comments explaining the security rationale

Security Impact

These changes prevent two attack vectors:

1. Configuration injection: Malicious proxy list entries (e.g., from remote URLs) can no longer inject squid.conf directives
2. Authentication downgrade: A rogue SOCKS5 server can no longer bypass configured credentials by claiming "no authentication" is acceptable

https://claude.ai/code/session_01TFReVCSK5wTRTbKqkF7gey

Summary by CodeRabbit

リリースノート

• バグ修正
  • プロキシ設定の入力検証を強化し、不正な形式やセキュリティリスクのあるエントリを拒否するようになりました。
  • SOCKS5認証方式の交渉ロジックを改善し、より厳密な認証ハンドシェイクを実施するようになりました。

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 449daeb9-e8fc-4964-9c21-001737c12fe8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/security-review-33NY5

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

setup/generate.php (1)

28-38: 参考（任意）: IPv6 リテラルのサポートは事実上限定的です。

Line 42 の host 正規表現は :, [, ] を許可しますが、Line 33 の explode(":", $line, 5) はコロン単位で無条件に分割するため、[::1]:8080:socks5:user:pass のような IPv6 リテラル行は正しくパースされません（host が [ などに化ける）。この挙動は本 PR の範囲外（既存仕様）ですが、host regex が IPv6 を許容する形になっている関係で、利用者に「IPv6 が使える」と誤解させる恐れがあります。README 等でのドキュメント化、もしくは将来的にパーサ自体を IPv6 対応へリファクタすることをご検討ください。

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@setup/generate.php` around lines 28 - 38, The current parser uses
explode(":", $line, 5) which naively splits on colons and therefore cannot
correctly parse IPv6 literals like "[::1]:8080:..."; because the host validation
regex still allows ':', '[' and ']', this can mislead users into thinking IPv6
is supported. Fix by either (A) documenting the limitation in README/setup docs
that IPv6 literals are not supported, or (B) updating the parsing and
validation: replace the naive explode(":", $line, 5) with IPv6-aware parsing
that detects a leading '[' IPv6 literal (find the matching ']' then the
following ':' port separator) and only then split the remaining fields, or as a
quicker mitigation alter the host validation regex to disallow ':' '[' ']' so
IPv6-like inputs are rejected rather than accepted; reference the explode(":",
$line, 5) usage and the host validation regex when making the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@setup/generate.php`:
- Line 43: The STDERR log writes raw values from $proxyInfo['host'] (and
similarly $proxyInfo['port'] before its check) which allows log injection;
sanitize these values before embedding in the message used in fwrite(STDERR,
...) by escaping control characters (e.g. with addcslashes($proxyInfo['host'],
"\0..\37") or rawurlencode) and use the sanitized variable in the "Skipping
proxy with invalid host" and corresponding port rejection messages; keep the
existing $reject_unsafe validation but ensure you never print unsanitized
$proxyInfo['host'] or $proxyInfo['port'] to STDERR.

---

Nitpick comments:
In `@setup/generate.php`:
- Around line 28-38: The current parser uses explode(":", $line, 5) which
naively splits on colons and therefore cannot correctly parse IPv6 literals like
"[::1]:8080:..."; because the host validation regex still allows ':', '[' and
']', this can mislead users into thinking IPv6 is supported. Fix by either (A)
documenting the limitation in README/setup docs that IPv6 literals are not
supported, or (B) updating the parsing and validation: replace the naive
explode(":", $line, 5) with IPv6-aware parsing that detects a leading '[' IPv6
literal (find the matching ']' then the following ':' port separator) and only
then split the remaining fields, or as a quicker mitigation alter the host
validation regex to disallow ':' '[' ']' so IPv6-like inputs are rejected rather
than accepted; reference the explode(":", $line, 5) usage and the host
validation regex when making the change.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aac10751-43ed-4e13-9692-926cf2306f6a

📥 Commits

Reviewing files that changed from the base of the PR and between 02e7f47 and d770435.

📒 Files selected for processing (2)

• setup/generate.php
• squid_patch/src/SocksPeerConnector.h

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

ログインジェクション: 拒否理由に不正値を素のままで埋め込んでいます。

$reject_unsafe は \s（改行含む）にマッチすると即 continue しますが、その直前に $proxyInfo['host'] をそのまま STDERR に書き出しています。悪意ある proxyList.txt の 1 行に evil\nFAKE LOG LINE のような値が含まれる場合、ログが複数行に分割され、下流のログ集約で偽のエントリを注入できます。port 側（Line 49）も、非数値で任意バイトが来る可能性があるため同じリスクがあります（Line 57/61 の host はこの時点で既に検証済みなので安全）。

出力前にサニタイズ（例えば rawurlencode もしくは addcslashes による制御文字のエスケープ）するのが安全です。

🛡️ 提案パッチ

     // Validate host: hostname or IPv4/IPv6 literal. No shell/conf metachars.
     if (preg_match($reject_unsafe, $proxyInfo['host']) ||
         !preg_match('/^[A-Za-z0-9.:\[\]_-]+$/', $proxyInfo['host'])) {
-        fwrite(STDERR, "Skipping proxy with invalid host: " . $proxyInfo['host'] . PHP_EOL);
+        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: " . $proxyInfo['port'] . PHP_EOL);
+        fwrite(STDERR, "Skipping proxy with invalid port: " . rawurlencode((string)$proxyInfo['port']) . PHP_EOL);
         continue;
     }

Also applies to: 49-49

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@setup/generate.php` at line 43, The STDERR log writes raw values from
$proxyInfo['host'] (and similarly $proxyInfo['port'] before its check) which
allows log injection; sanitize these values before embedding in the message used
in fwrite(STDERR, ...) by escaping control characters (e.g. with
addcslashes($proxyInfo['host'], "\0..\37") or rawurlencode) and use the
sanitized variable in the "Skipping proxy with invalid host" and corresponding
port rejection messages; keep the existing $reject_unsafe validation but ensure
you never print unsanitized $proxyInfo['host'] or $proxyInfo['port'] to STDERR.