Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions agent/agentcontainers/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
19 changes: 8 additions & 11 deletions agent/agentcontainers/execer.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@ package agentcontainers

import (
"context"
"fmt"
"os/exec"
"runtime"
"strings"

"cdr.dev/slog/v3"
"github.com/coder/coder/v2/agent/agentexec"
Expand Down Expand Up @@ -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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

proc.agent.OperatingSystem = "linux" // Assuming Linux for devcontainers.

I'm not planning to dig any deeper on Windows here because it's not supported.

cmdArgs := append([]string{inName}, inArgs...)
args = append([]string{"-c", `"$@"`, ""}, cmdArgs...)
return name, args, dir, env
}

Expand Down
84 changes: 84 additions & 0 deletions agent/agentcontainers/execer_internal_test.go
Original file line number Diff line number Diff line change
@@ -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")
})
}
Loading