fix(agent/agentcontainers): prevent command injection in shell execer (#26235) (#26289)

f0ssel · zedkipp · web-flow · commit 4aa84f2e6a28 · 2026-06-11T15:47:48.000-04:00
Backport of #26235 to `release/2.29`. - Original PR: #26235 (fix(agent/agentcontainers): prevent command injection in shell execer) - Merge commit: 112c921 `commandEnvExecer.prepare` rebuilt commands into a single shell string using `fmt.Sprintf("%q", arg)`, which produces Go string literals rather than shell-quoted tokens. Go's `%q` does not escape `$`, backticks, or other metacharacters that stay active inside double quotes, so an argument such as `$(...)` was evaluated by the shell as command substitution. The fix passes the command to the shell as positional parameters and runs `"$@"` so the shell forwards argv verbatim. <details> <summary>Backport notes</summary> The cherry-pick (`git cherry-pick -x 112c921`) auto-merged context differences in `execer.go` and `api_test.go` cleanly. The only manual adaptation was in the new `execer_internal_test.go`: its imports were changed from `cdr.dev/slog/v3` to `cdr.dev/slog`, because `release/2.29` predates the slog v3 migration. No functional changes. Verified on `release/2.29`: - `go build ./agent/agentcontainers/` passes - `go test ./agent/agentcontainers/ -run TestCommandEnvExecer_Prepare` passes (3 subtests) - `go test ./agent/agentcontainers/ -run TestAPI/CommandEnv` passes </details> > [!NOTE] > Generated by Coder Agents on behalf of @f0ssel. Co-authored-by: Zach <3724288+zedkipp@users.noreply.github.com>
diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go
@@ -2768,16 +2768,14 @@ func TestAPI(t *testing.T) {
 		// Verify commands were executed through the custom shell and environment.
 		require.NotEmpty(t, fakeExec.commands, "commands should be executed")
 
-		// Want: /bin/custom-shell -c '"docker" "ps" "--all" "--quiet" "--no-trunc"'
+		// Want: /bin/custom-shell -c "$@" "" docker ps --all --quiet --no-trunc
+		// The command is passed as positional parameters and run via "$@" so
+		// the shell forwards argv without re-parsing it.
 		require.Equal(t, testShell, fakeExec.commands[0][0], "custom shell should be used")
-		if runtime.GOOS == "windows" {
-			require.Equal(t, "/c", fakeExec.commands[0][1], "shell should be called with /c on Windows")
-		} else {
-			require.Equal(t, "-c", fakeExec.commands[0][1], "shell should be called with -c")
-		}
-		require.Len(t, fakeExec.commands[0], 3, "command should have 3 arguments")
-		require.GreaterOrEqual(t, strings.Count(fakeExec.commands[0][2], " "), 2, "command/script should have multiple arguments")
-		require.True(t, strings.HasPrefix(fakeExec.commands[0][2], `"docker" "ps"`), "command should start with \"docker\" \"ps\"")
+		require.Equal(t, "-c", fakeExec.commands[0][1], "shell should be called with -c")
+		require.Equal(t, `"$@"`, fakeExec.commands[0][2], "script should run argv via \"$@\"")
+		require.Equal(t, "", fakeExec.commands[0][3], "$0 slot should be an empty placeholder")
+		require.Equal(t, []string{"docker", "ps", "--all", "--quiet", "--no-trunc"}, fakeExec.commands[0][4:], "argv should be passed through unquoted")
 
 		// Verify the environment was set on the command.
 		lastCmd := fakeExec.getLastCommand()
diff --git a/agent/agentcontainers/execer.go b/agent/agentcontainers/execer.go
@@ -2,10 +2,7 @@ package agentcontainers
 
 import (
 	"context"
-	"fmt"
 	"os/exec"
-	"runtime"
-	"strings"
 
 	"cdr.dev/slog"
 	"github.com/coder/coder/v2/agent/agentexec"
@@ -51,15 +48,15 @@ func (e *commandEnvExecer) prepare(ctx context.Context, inName string, inArgs ..
 		return inName, inArgs, "", nil
 	}
 
-	caller := "-c"
-	if runtime.GOOS == "windows" {
-		caller = "/c"
-	}
 	name = shell
-	for _, arg := range append([]string{inName}, inArgs...) {
-		args = append(args, fmt.Sprintf("%q", arg))
-	}
-	args = []string{caller, strings.Join(args, " ")}
+	// Pass the command through the shell as positional parameters and run
+	// "$@" so the shell re-emits argv verbatim without re-parsing it. This
+	// 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.
+	cmdArgs := append([]string{inName}, inArgs...)
+	args = append([]string{"-c", `"$@"`, ""}, cmdArgs...)
 	return name, args, dir, env
 }
 
diff --git a/agent/agentcontainers/execer_internal_test.go b/agent/agentcontainers/execer_internal_test.go
@@ -0,0 +1,84 @@
+package agentcontainers
+
+import (
+	"bytes"
+	"context"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"cdr.dev/slog"
+	"cdr.dev/slog/sloggers/slogtest"
+	"github.com/coder/coder/v2/agent/agentexec"
+	"github.com/coder/coder/v2/agent/usershell"
+	"github.com/coder/coder/v2/testutil"
+)
+
+func TestCommandEnvExecer_Prepare(t *testing.T) {
+	t.Parallel()
+
+	if runtime.GOOS == "windows" {
+		t.Skip("the POSIX shell quoting under test does not apply on Windows")
+	}
+
+	const shell = "/bin/sh"
+	commandEnv := func(usershell.EnvInfoer, []string) (string, string, []string, error) {
+		return shell, "/tmp", []string{"FOO=bar"}, nil
+	}
+	e := newCommandEnvExecer(slogtest.Make(t, nil).Leveled(slog.LevelDebug), commandEnv, agentexec.DefaultExecer)
+
+	t.Run("ArgvPassthrough", func(t *testing.T) {
+		t.Parallel()
+
+		name, args, dir, env := e.prepare(context.Background(), "echo", "hello", "world")
+		// The command is run as: shell -c "$@" "" <argv...> so that the
+		// shell re-emits argv without re-parsing it. The empty $0 slot is
+		// discarded.
+		require.Equal(t, shell, name)
+		require.Equal(t, []string{"-c", `"$@"`, "", "echo", "hello", "world"}, args)
+		require.Equal(t, "/tmp", dir)
+		require.Equal(t, []string{"FOO=bar"}, env)
+	})
+
+	t.Run("MetacharactersNotInterpreted", func(t *testing.T) {
+		t.Parallel()
+
+		payloads := []string{
+			"$(echo INJECTED)",
+			"`echo INJECTED`",
+			"$HOME",
+			"a; echo INJECTED",
+			"a && echo INJECTED",
+			"a | echo INJECTED",
+			"a\necho INJECTED",
+			"it's a \"test\" \\ end",
+			"",
+		}
+		for _, payload := range payloads {
+			ctx := testutil.Context(t, testutil.WaitShort)
+			cmd := e.CommandContext(ctx, "printf", "%s", payload)
+			var out bytes.Buffer
+			cmd.Stdout = &out
+			cmd.Stderr = &out
+			require.NoError(t, cmd.Run(), "payload %q", payload)
+			assert.Equal(t, payload, out.String(), "payload %q was altered by the shell", payload)
+		}
+	})
+
+	t.Run("CommandSubstitutionHasNoSideEffect", func(t *testing.T) {
+		t.Parallel()
+
+		marker := filepath.Join(t.TempDir(), "pwned")
+		ctx := testutil.Context(t, testutil.WaitShort)
+		cmd := e.CommandContext(ctx, "echo", "$(touch "+marker+")")
+		var out bytes.Buffer
+		cmd.Stdout = &out
+		cmd.Stderr = &out
+		require.NoError(t, cmd.Run())
+		require.Equal(t, "$(touch "+marker+")\n", out.String())
+		require.NoFileExists(t, marker, "command substitution executed; injection is possible")
+	})
+}
