fix: use absolute paths for credential helper binaries to prevent PATH hijacking#236
fix: use absolute paths for credential helper binaries to prevent PATH hijacking#236bukinoshita wants to merge 7 commits intomainfrom
Conversation
…H hijacking Resolve secret-tool (Linux) and powershell.exe (Windows) to absolute paths before execution, preventing local PATH hijacking attacks that could steal Resend API keys. Linux: - Check trusted paths (/usr/bin, /usr/local/bin, /bin) first - Fall back to /usr/bin/which for resolution - Reject non-absolute paths from which output - Cache resolved paths for subsequent calls - Use absolute /usr/bin/which instead of bare 'which' in isAvailable() Windows: - Construct absolute PowerShell path from %SystemRoot% env var - Default to C:\Windows if SystemRoot is not set Closes BU-627 Co-authored-by: Bu Kinoshita <bukinoshita@users.noreply.github.com>
|
@cubic-dev-ai can you review? |
@bukinoshita I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/lib/credential-backends/windows.ts">
<violation number="1" location="src/lib/credential-backends/windows.ts:5">
P1: Do not trust `SystemRoot` from the environment for the executable path; it can be overridden to run an attacker-controlled `powershell.exe`.</violation>
</file>
<file name="src/lib/credential-backends/linux.ts">
<violation number="1" location="src/lib/credential-backends/linux.ts:83">
P1: The PATH fallback trusts any absolute `which` result, which can still execute a malicious `secret-tool` from an untrusted PATH directory.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| try { | ||
| const systemRoot = execFileSync( | ||
| 'cmd.exe', | ||
| ['/d', '/c', 'echo', '%SystemRoot%'], |
There was a problem hiding this comment.
It seems this command has the same fake-binary attack surface issue, because it's also PATH-resolved. process.env.SystemRoot is supposed to be safer but a previous review also flagged that, so not sure what's best here.
| return trusted; | ||
| } | ||
|
|
||
| const { stdout, code } = await run('/usr/bin/which', ['secret-tool']); |
There was a problem hiding this comment.
user/bin/which re-introduces the PATH vulnerability this PR tries to address; but I'm not sure we can cover EVERY possible secret tool path for Linux users 🤔
felipefreitag
left a comment
felipefreitag
left a comment
There was a problem hiding this comment.
I like the idea of hardening the secret tools call but I'm not sure how we can at the same time be flexible to support each user's installations and not leave open attack vectors. If we hardcode everything, users who customized their environments won't be able to use the CLI at all.
3 participants
Summary by cubic
Use absolute paths for credential helpers on Linux and Windows to prevent PATH hijacking and protect Resend API keys.
secret-toolfrom trusted paths (/usr/bin,/usr/local/bin,/bin), fall back to/usr/bin/which, reject non-absolute results, and cache the resolved path. All calls (get,set,delete,isAvailable) now use absolute paths.%SystemRoot%\System32\WindowsPowerShell\v1.0\powershell.exe, defaulting toC:\Windowsif unset. Use this path for bothexecFileandspawn./usr/bin/security).Written for commit 00723e3. Summary will update on new commits.