diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 1429de7d85354..8acc6fabd38b8 100644 --- a/agent/agentcontainers/api_test.go +++ b/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() diff --git a/agent/agentcontainers/execer.go b/agent/agentcontainers/execer.go index 0f85687893486..4695c95947716 100644 --- 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/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 } diff --git a/agent/agentcontainers/execer_internal_test.go b/agent/agentcontainers/execer_internal_test.go new file mode 100644 index 0000000000000..8b98693b734cf --- /dev/null +++ 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/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 "$@" "" 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") + }) +}