fix(agent/agentcontainers): prevent command injection in shell execer#26235
Conversation
commandEnvExecer.prepare rebuilt commands into a single shell string
using fmt.Sprintf("%q", arg), which produces Go string literals, not
shell-quoted tokens. Go's %q does not escape $, backticks, or other
metacharacters that remain active inside double quotes, so an argument
such as $(...) was evaluated by the shell as command substitution.
Arguments flow from devcontainer config and workspace-folder, making
this exploitable.
Pass the command to the shell as positional parameters and run "$@" so
the shell forwards argv verbatim without re-parsing it. The Windows path
keeps its prior behavior. It's suspected to already be non-functional
(the selected shell is usually PowerShell, which rejects the hardcoded
cmd.exe "/c" switch) and a correct implementation is deferred.
| args = append(args, fmt.Sprintf("%q", arg)) | ||
| cmdArgs := append([]string{inName}, inArgs...) | ||
| if runtime.GOOS == "windows" { | ||
| // Preserve the prior quoting behavior on Windows. This path was |
There was a problem hiding this comment.
Honestly I don't understand how/if this worked on Windows before, nor why there is any Windows handling here. usershell.get() prefers Powershell, but we use "/c" which seems to target cmd.exe. %q doesn't seem like the correct quoting model for either shell.
In any case, the fix addresses the core problem on Linux which is what we officially support for dev containers (https://coder.com/docs/user-guides/devcontainers#limitations). I want to delete the windows cases, but I've opted to leave them as-is for now.
There was a problem hiding this comment.
@mafredri do you have any context here that would helpful?
There was a problem hiding this comment.
My best guess is this was about leaving the door open to expand support to using Dev Containers on not just a Linux-based host workspace, but also Windows workspaces given the presence of Docker.
But unless there's motivation to actually do that it's just dead code and we should remove it and disable the tests for Windows.
There was a problem hiding this comment.
Agreed. I decided to remove the Windows handling here since I don't have any reason to keep it around and it doesn't seem correct anyway.
| // Preserve the prior quoting behavior on Windows. This path was | ||
| // already non-functional: the selected shell is usually PowerShell | ||
| // (see usershell.Get), which rejects the hardcoded cmd.exe "/c" | ||
| // switch. A correct Windows implementation is deferred. |
There was a problem hiding this comment.
General note on comment hygiene: If this is deferred, it needs to be a ticket rather than a code comment. Future plans may be expressed in comments if they are helpful in the future design/implementation, but we shouldn't be using them as a poor substitute for an issue tracker.
There was a problem hiding this comment.
Ack, this is all removed now.
| args = append(args, fmt.Sprintf("%q", arg)) | ||
| cmdArgs := append([]string{inName}, inArgs...) | ||
| if runtime.GOOS == "windows" { | ||
| // Preserve the prior quoting behavior on Windows. This path was |
There was a problem hiding this comment.
My best guess is this was about leaving the door open to expand support to using Dev Containers on not just a Linux-based host workspace, but also Windows workspaces given the presence of Docker.
But unless there's motivation to actually do that it's just dead code and we should remove it and disable the tests for Windows.
| // prevents arguments containing shell metacharacters such as $, `, and | ||
| // quotes from being interpreted (e.g. command substitution). The first | ||
| // argument after the script ($0) is a placeholder for the shell name. | ||
| args = append([]string{"-c", `"$@"`, shell}, cmdArgs...) |
There was a problem hiding this comment.
| args = append([]string{"-c", `"$@"`, shell}, cmdArgs...) | |
| args = append([]string{"-c", `"$@"`, ""}, cmdArgs...) |
Let's not make it seem like argv0 plays a role here as it is discarded anyway.
This solution is somewhat clever, but it does e.g. break shell script usage which could originate from our own implementation. I'm not sure if there is any such usage right now, but the result would be surprising (compared to other execer implementations).
I'm wondering if this is the correct level to apply this fix, or if we should handle it at the call site containing user inputs. Granted this fixes the class of bug even if future call-sites are introduced so I'm not going to block on this.
There was a problem hiding this comment.
Fair point, I adopted your suggestion for argv0.
I thought about doing shell escaping at a higher level, but this seemed like the right place to me from a security perspective. I figured if we need something to handle non-user input that contains variables/command substitution etc we could create a separate function for that.
RE: internal shell script usage, I audited callers and they all appear to pass literal args. The only one that stood out was an inline substitution, but it targets an in-container shell
coder/agent/agentcontainers/api.go
Line 1998 in a4c867f
| // switch. A correct Windows implementation is deferred. | ||
| var quoted []string | ||
| for _, arg := range cmdArgs { | ||
| quoted = append(quoted, fmt.Sprintf("%q", arg)) |
There was a problem hiding this comment.
If we keep the "$@" ... implementation, then I don't think this quoting is necessary. In fact, it probably breaks the implementation.
/bin/sh -c '"$@"' "" '"echo"' foo bar
: 1: "echo": not found
There was a problem hiding this comment.
This is deleted now since I decided to remove the Windows handling 😄
| // prevents arguments containing shell metacharacters such as $, `, and | ||
| // quotes from being interpreted (e.g. command substitution). The token | ||
| // before them fills $0, which "$@" never references, so it is discarded. | ||
| // This assumes a POSIX shell; Windows is not supported here. |
There was a problem hiding this comment.
It doesn't seem like we have anything preventing this from executing on Windows upstream of this call, so I was concerned that a similar vulnerability might existing for command parsing on powershell.exe or cmd.exe using this new approach, but it appears that the $@ will cause a parse error on powershell. This is tested via a custom harness built with Claude that I can't seem to attach here - I'll DM you. I haven't checked cmd.exe, nor am I sure if something structural doesn't prevent this running on windows in the first place.
Ideally we would harden the POSIX requirement, but that's tricky.
There was a problem hiding this comment.
I don't know the exact failure mode if someone tried Windows for this, but I do see we assume linux for devcontainers when creating the sub-agent (proc.agent.OperatingSystem is only assigned a value on this line)
coder/agent/agentcontainers/api.go
Line 1756 in c883db9
I'm not planning to dig any deeper on Windows here because it's not supported.
4 participants
commandEnvExecer.prepare rebuilt commands into a single shell string using
fmt.Sprintf("%q", arg), which produces Go string literals, not shell-quoted tokens. Go's %q does not escape$, backticks, or other metacharacters that remain active inside double quotes, so an argument such as$(...)was evaluated by the shell as command substitution. Arguments flow from devcontainer config and workspace-folder, making this exploitable.Pass the command to the shell as positional parameters and run
"$@"so the shell forwards argv verbatim without re-parsing it.The Windows previous handling is not required because Coder doesn't support devcontainers on Windows, so it is removed.