Skip to content

fix(dotnet): prevent shell command injection in CLI Agent (fixes #636)#647

Open
kuangmi-bit wants to merge 1 commit into
a2aproject:mainfrom
kuangmi-bit:fix/cli-agent-shell-injection
Open

fix(dotnet): prevent shell command injection in CLI Agent (fixes #636)#647
kuangmi-bit wants to merge 1 commit into
a2aproject:mainfrom
kuangmi-bit:fix/cli-agent-shell-injection

Conversation

@kuangmi-bit

Copy link
Copy Markdown

Summary

Fixes the shell command injection vulnerability reported in #636.

Root cause: ParseCommand only allowlisted the first token, then interpolated the entire string into /bin/bash -c "{command} {arguments}". Shell metacharacters (;, |, &&, $(), backticks) in the argument portion bypassed the allowlist entirely.

Fix: Replace shell-based execution with direct process invocation:

  1. No shell — Execute allowlisted commands directly via ProcessStartInfo, never through /bin/bash -c or cmd.exe /c
  2. ArgumentList — Parse arguments into a List<string>, pass via ArgumentList (argv). ArgumentList handles platform-specific quoting; metacharacters arrive as literal argv entries
  3. Narrowed allowlist — Remove interpreters (python, node, npm, dotnet) — they accept arbitrary code as arguments
  4. Security warning — Added doc comment that this sample should not be exposed to untrusted clients

Credit to @chopmob-cloud for the detailed root-cause analysis and fix direction in #636.

Replace shell-based command execution (/bin/bash -c / cmd.exe /c) with
direct process invocation using ProcessStartInfo.ArgumentList.

Root cause: ParseCommand only allowlisted the first token, then
interpolated the entire string into a shell command line. Shell
metacharacters (;, |, &&, $(), backticks) in the argument portion
bypassed the allowlist entirely.

Changes:
- Execute allowlisted commands directly — no shell, no /bin/bash -c
- Parse arguments into a List<string>, pass via ArgumentList (argv)
- ArgumentList handles platform-specific quoting, metacharacters
  arrive as literal argv entries — never interpreted by a shell
- Remove interpreters (python, node, npm, dotnet) from default
  allowlist — they accept arbitrary code as arguments
- Add security warning: this sample should not be exposed to
  untrusted clients

UseShellExecute = false is not a mitigation here — it only stops
.NET from using the OS shell to resolve FileName. The code was
explicitly invoking /bin/bash -c (and cmd.exe /c), so a shell
parsed the string regardless.

Credit to @chopmob-cloud for the detailed root-cause analysis
and fix direction.

Fixes a2aproject#636
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@chopmob-cloud

Copy link
Copy Markdown

Verified this against the branch. The sink from #636 is closed, and it is the right shape of fix rather than an escaping patch: there is no shell process left to interpret metacharacters, FileName is the executable itself, and ArgumentList hands the tokens to the OS as an argv array, so ;, |, &&, $() and backticks arrive as literal argument text. Dropping python, node, npm and dotnet is the other half of it, since an allowlist cannot constrain a process that accepts arbitrary program text as an argument.

Three things the change leaves behind.

Five allowlist entries only worked because of the shell. AllowedCommands still carries dir, date, time, echo and type. On Windows all five are cmd.exe internal commands rather than executables on PATH, so they resolved only through the cmd.exe /c wrapper this PR removes. With UseShellExecute = false and FileName = "dir", Process.Start throws Win32Exception, file not found. Not a security problem, but it breaks the sample's own first documented example:

CLI> dir                    # List directory (Windows)

run-demo.bat ships in the same directory, so Windows is a supported path here rather than an edge case. Two of the five are worth pruning outright rather than re-homing: type has no Unix binary at all, and time is a shell builtin on both platforms with /usr/bin/time not guaranteed to be present. The same reasoning that justified removing the interpreters applies here, that entries which depended on the shell should leave with it.

The rest of the list divides cleanly by platform and is unaffected either way. whoami, tasklist, netstat, ipconfig, ping and git are real Windows executables. ls, pwd, cat, head, tail and ps are real Unix binaries and were never reachable on Windows even under cmd.exe, so nothing regresses there.

The README now documents the design this PR removes. It still states "Windows: Uses cmd.exe for command execution" and "Linux/Mac: Uses /bin/bash for command execution", and still lists dotnet, node, npm and python under allowed development tools. For a sample whose stated purpose is demonstrating the safe pattern, prose describing the unsafe one is worth correcting in the same change, otherwise a reader working from the README reintroduces what the code just fixed.

Minor: using System.Runtime.InteropServices; is now the only reference to that namespace in the file, since RuntimeInformation.IsOSPlatform was its sole consumer.

Worth naming what the change deliberately does not cover, because the added remarks get this right: cat, head and tail with a caller-chosen path is still arbitrary file read, and ping still reaches the network. Those are properties of the allowlist rather than of the execution model, and the "should NOT be exposed to untrusted clients" note is the honest boundary for a sample that bridges remote messages to local processes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants