fix(agent/agentcontainers): forward execer args without shell interpretation#26201
fix(agent/agentcontainers): forward execer args without shell interpretation#26201zedkipp wants to merge 1 commit into
Conversation
…ner labels When the agent processed a container, attacker-controlled metadata such as devcontainer.metadata labels reached the user's shell with quoting that did not block command substitution. A container started on the workspace's Docker daemon could therefore embed shell code in a label and have the agent execute it as the agent user on the workspace host, escaping the container. Invoke docker and devcontainer commands directly rather than through a shell. Untrusted strings are passed as discrete argv entries, so they cannot be reinterpreted as shell syntax. The user's PATH continues to apply so user-installed binaries still resolve. Authored with assistance from Coder Agents.
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 1 | Last posted: Round 1, 9 findings (5 P3, 1 P4, 3 Nit), COMMENT. Review Finding inventoryFindings
Round logRound 1Panel round. Netero first pass: 1 P3, 1 Nit. Panel (18 reviewers): 4 P3, 1 P4, 2 Nit new. 0 dropped. Reviewed against 242c4d7..fefa8d7. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Solid security fix. The old sh -c wrapper with Go %q quoting had a real shell injection surface for any attacker-controlled argv content (container labels, workspace paths). This PR eliminates the class by removing the shell entirely and forwarding argv directly. The test suite is thorough: ten distinct injection vectors, PATH resolution edge cases, and graceful degradation.
"The intermediate shell was pure indirection cost with a security surface as the interest rate." (Ryosuke)
5 P3, 1 P4, 3 Nit. All are documentation accuracy, test coverage gaps, or operational signal issues. The security fix itself is clean and complete.
Two behavioral observations worth knowing: (1) When lookPathInEnv fails to resolve a command, the bare name falls through to exec.Cmd's internal LookPath, which searches the agent's PATH. This is more forgiving than the old shell behavior, and for the hardcoded commands (docker, devcontainer), both PATHs almost always agree. (2) recordingExecer is the fifth agentexec.Execer test fake in the tree. The duplication is structurally forced by Go's test package boundary (internal test package cannot import external test fakes).
🤖 This review was automatically generated with Coder Agents.
| if strings.ContainsRune(name, filepath.Separator) { | ||
| return name, nil | ||
| } | ||
| if runtime.GOOS == "windows" { |
There was a problem hiding this comment.
P3 [CRF-1] On Windows, lookPathInEnv delegates to exec.LookPath(name), which searches the agent process's PATH rather than the env-supplied PATH. The old shell-based approach resolved commands against the user's PATH because the shell inherited the user's environment.
If a Windows user's CommandEnv PATH includes a custom docker location absent from the agent's PATH, container commands fail silently. The doc comment at lines 95-96 acknowledges this gap, but there is no ticket or follow-up.
Suggested fix: either implement Windows-specific PATH scanning (parsing PATHEXT) or file a ticket and update the skip message to reference it. (Netero)
🤖
| t.Parallel() | ||
| if runtime.GOOS == "windows" { | ||
| t.Skip("PATH resolution semantics differ on Windows; covered separately.") | ||
| } |
There was a problem hiding this comment.
Nit [CRF-2] The skip message says "covered separately" but no Windows-specific test for lookPathInEnv's exec.LookPath delegation exists in this PR or elsewhere in agent/agentcontainers/. Same issue at line 139 ("covered there"). The skips are correct; the messages make a false claim about existing coverage.
Suggested: "Windows PATH resolution uses exec.LookPath; not tested here." (Netero)
🤖
| return c | ||
| } | ||
|
|
||
| // lookPathInEnv resolves a command name against the PATH found in env. It |
There was a problem hiding this comment.
P3 [CRF-3] The doc says "mirrors os/exec.LookPath" but diverges in at least two ways:
-
isExecutable(line 128) checksPerm()&0o111 != 0(any user class). The stdlib'sfindExecutablecallsunix.Eaccess(file, unix.X_OK)to verify the calling process can actually execute the file, accounting for real UID, GID, and supplementary groups. If a binary is owned by root:docker with mode 0o750 and the agent runs as uid=1000 outside the docker group,isExecutableresolves it but exec fails with "permission denied," skipping a valid binary later in PATH. -
Names with path separators (line 101-102) are returned unchanged with nil error. The stdlib validates executability for such names.
"The old shell-based approach got correct per-user resolution for free because the shell itself performs eaccess-equivalent checks during PATH lookup." (Mafuuu)
Since eaccess lives in internal/syscall/unix (inaccessible outside stdlib), the behavioral fix is impractical. Update the doc to drop "mirrors os/exec.LookPath" and describe the actual behavior: permission-bit-based PATH lookup using the supplied env. (Mafuuu P3, Meruem Note, Razor Note, Knov Note)
🤖
| var pathVar string | ||
| for _, kv := range env { | ||
| if k, v, ok := strings.Cut(kv, "="); ok && k == "PATH" { | ||
| pathVar = v // POSIX: later definitions override earlier ones. |
There was a problem hiding this comment.
P3 [CRF-4] POSIX: later definitions override earlier ones is incorrect. POSIX does not guarantee last-wins for duplicate environment variable names; the behavior is explicitly undefined. With raw execve + glibc, getenv("PATH") returns the first match when the env array contains duplicate PATH entries.
The last-wins behavior comes from Go's exec.Cmd.Start(), which calls dedupEnv to remove duplicates in favor of the last entry (confirmed in os/exec/exec.go). Because both lookPathInEnv and dedupEnv keep the last value, the code is correct. The comment is misleading: applying this "POSIX rule" in a non-Go context would give the wrong answer.
Suggested: // Go's exec.Cmd deduplicates env to last-wins; match that here. (Razor)
🤖
| } | ||
| } | ||
| for _, dir := range filepath.SplitList(pathVar) { | ||
| if dir == "" { |
There was a problem hiding this comment.
P3 [CRF-5] This empty PATH entry branch is new code with no test exercising it. Coverage confirms these lines are uncovered. The existing "EmptyPATH" test case (PATH=) checks a different condition: filepath.SplitList("") returns [], so the loop body never runs and the dir == "" rewrite is never reached.
A test case like
{name: "EmptyPATHEntry", nameArg: "tool", env: []string{"PATH=:" + dir2}, want: filepath.Join(dir2, "tool")}would exercise the branch. (Bisky)
(Bisky)
🤖
| shell, dir, env, err := e.commandEnv(nil, nil) | ||
| _, dir, env, err := e.commandEnv(nil, nil) | ||
| if err != nil { | ||
| e.logger.Error(ctx, "get command environment failed", slog.Error(err)) |
There was a problem hiding this comment.
P3 [CRF-6] The error log reads "get command environment failed" with only slog.Error(err). An operator investigating workspace exec failures at scale sees this line but cannot tell which command (docker, devcontainer, a custom binary) triggered it without cross-referencing timestamps.
Fix: add slog.F("cmd", inName) to the logger.Error call. (Chopper)
🤖
| {name: "Newline", args: []string{"inspect", "line1\nline2"}}, | ||
| } | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
Nit [CRF-7] Three call sites (lines 71, 111, 127) create context.Background(). The project is on Go 1.26; t.Context() (available since 1.24) returns a context canceled when the test ends, giving lifecycle-aware cleanup for free. (Ging-Go)
🤖
| // prepare returns the command name, args, working directory and environment | ||
| // to hand to the wrapped Execer. | ||
| // | ||
| // The shell returned by CommandEnv is intentionally discarded: this layer no | ||
| // longer invokes a shell, so attacker-controlled argv elements cannot be | ||
| // reinterpreted as shell syntax. To match what the user's shell would resolve, | ||
| // the command name is resolved against the PATH in env rather than the agent | ||
| // process's PATH. If lookup fails, the original name is forwarded so the | ||
| // wrapped execer can surface a normal "executable not found" error. |
There was a problem hiding this comment.
P4 [CRF-8] Several doc comments in this PR carry mechanism narration that restates what the code already shows. The security rationale (lines 53-56) earns its place; the surrounding padding does not. Audit found 9 of 14 new comment blocks with bloat.
Examples: lines 50-51 ("returns the command name, args, working directory and environment to hand to the wrapped Execer") restate the function signature. recordingExecer doc (test file lines 19-21) says the same thing twice in different words.
Note: the lookPathInEnv doc rewrite is likely addressed by CRF-3's fix (dropping the inaccurate "mirrors" claim). (Gon)
🤖
| t.Skip("lookPathInEnv defers to exec.LookPath on Windows; covered there") | ||
| } | ||
|
|
||
| dir1 := t.TempDir() |
There was a problem hiding this comment.
Nit [CRF-9] dir1/dir2 tell the reader nothing about their contents. The test places non-executable fixtures in dir1 and executables in dir2. Names like noExecDir/execDir make the setup and every referencing test case self-documenting. (Gon)
🤖
commandEnvExecer previously assembled an
sh -cscript by wrapping each argument in fmt.Sprintf("%q", arg). Go's %q produces double-quoted Go syntax, and POSIX shells still perform $(...), backtick, and $VAR expansion inside double quotes, so attacker-controlled values such as Docker container labels could be evaluated by the agent's shell as a container-to-host escape.Drop the shell wrapper entirely and forward the cmd and args to the inner Execer as discrete argv entries. The PATH from CommandEnv is now honored by a small env-aware lookup (lookPathInEnv) so user-installed binaries (devcontainer, custom docker) continue to resolve as they did under the shell.
Authored with assistance from Coder Agents.