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

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.

agent/agentcontainers/api_test.go

@@ -3923,16 +3923,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()

agent/agentcontainers/execer.go

@@ -2,10 +2,7 @@ package agentcontainers
 
 import (
 	"context"
-	"fmt"
 	"os/exec"
-	"runtime"
-	"strings"
 
 	"cdr.dev/slog/v3"
 	"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
 }
 

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/v3"
+	"cdr.dev/slog/v3/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")
+	})
+}