Skip to content

Commit b949480

Browse files
github-actions[bot]zedkipp
github-actions[bot]
and
zedkipp
authored
fix(agent/agentcontainers): prevent command injection in shell execer (#26235) (#26286)
Backport of #26235 Original PR: #26235 — fix(agent/agentcontainers): prevent command injection in shell execer Merge commit: 112c921 Requested by: @f0ssel Co-authored-by: Zach <3724288+zedkipp@users.noreply.github.com>
1 parent e4a7657 commit b949480

3 files changed

Lines changed: 99 additions & 20 deletions

File tree

‎agent/agentcontainers/api_test.go‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3923,16 +3923,14 @@ func TestAPI(t *testing.T) {
39233923
// Verify commands were executed through the custom shell and environment.
39243924
require.NotEmpty(t, fakeExec.commands, "commands should be executed")
39253925

3926-
// Want: /bin/custom-shell -c '"docker" "ps" "--all" "--quiet" "--no-trunc"'
3926+
// Want: /bin/custom-shell -c "$@" "" docker ps --all --quiet --no-trunc
3927+
// The command is passed as positional parameters and run via "$@" so
3928+
// the shell forwards argv without re-parsing it.
39273929
require.Equal(t, testShell, fakeExec.commands[0][0], "custom shell should be used")
3928-
if runtime.GOOS == "windows" {
3929-
require.Equal(t, "/c", fakeExec.commands[0][1], "shell should be called with /c on Windows")
3930-
} else {
3931-
require.Equal(t, "-c", fakeExec.commands[0][1], "shell should be called with -c")
3932-
}
3933-
require.Len(t, fakeExec.commands[0], 3, "command should have 3 arguments")
3934-
require.GreaterOrEqual(t, strings.Count(fakeExec.commands[0][2], " "), 2, "command/script should have multiple arguments")
3935-
require.True(t, strings.HasPrefix(fakeExec.commands[0][2], `"docker" "ps"`), "command should start with \"docker\" \"ps\"")
3930+
require.Equal(t, "-c", fakeExec.commands[0][1], "shell should be called with -c")
3931+
require.Equal(t, `"$@"`, fakeExec.commands[0][2], "script should run argv via \"$@\"")
3932+
require.Equal(t, "", fakeExec.commands[0][3], "$0 slot should be an empty placeholder")
3933+
require.Equal(t, []string{"docker", "ps", "--all", "--quiet", "--no-trunc"}, fakeExec.commands[0][4:], "argv should be passed through unquoted")
39363934

39373935
// Verify the environment was set on the command.
39383936
lastCmd := fakeExec.getLastCommand()

‎agent/agentcontainers/execer.go‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,7 @@ package agentcontainers
22

33
import (
44
"context"
5-
"fmt"
65
"os/exec"
7-
"runtime"
8-
"strings"
96

107
"cdr.dev/slog/v3"
118
"github.com/coder/coder/v2/agent/agentexec"
@@ -51,15 +48,15 @@ func (e *commandEnvExecer) prepare(ctx context.Context, inName string, inArgs ..
5148
return inName, inArgs, "", nil
5249
}
5350

54-
caller := "-c"
55-
if runtime.GOOS == "windows" {
56-
caller = "/c"
57-
}
5851
name = shell
59-
for _, arg := range append([]string{inName}, inArgs...) {
60-
args = append(args, fmt.Sprintf("%q", arg))
61-
}
62-
args = []string{caller, strings.Join(args, " ")}
52+
// Pass the command through the shell as positional parameters and run
53+
// "$@" so the shell re-emits argv verbatim without re-parsing it. This
54+
// prevents arguments containing shell metacharacters such as $, `, and
55+
// quotes from being interpreted (e.g. command substitution). The token
56+
// before them fills $0, which "$@" never references, so it is discarded.
57+
// This assumes a POSIX shell; Windows is not supported here.
58+
cmdArgs := append([]string{inName}, inArgs...)
59+
args = append([]string{"-c", `"$@"`, ""}, cmdArgs...)
6360
return name, args, dir, env
6461
}
6562

‎agent/agentcontainers/execer_internal_test.go‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
package agentcontainers
2+
3+
import (
4+
"bytes"
5+
"context"
6+
"path/filepath"
7+
"runtime"
8+
"testing"
9+
10+
"github.com/stretchr/testify/assert"
11+
"github.com/stretchr/testify/require"
12+
13+
"cdr.dev/slog/v3"
14+
"cdr.dev/slog/v3/sloggers/slogtest"
15+
"github.com/coder/coder/v2/agent/agentexec"
16+
"github.com/coder/coder/v2/agent/usershell"
17+
"github.com/coder/coder/v2/testutil"
18+
)
19+
20+
func TestCommandEnvExecer_Prepare(t *testing.T) {
21+
t.Parallel()
22+
23+
if runtime.GOOS == "windows" {
24+
t.Skip("the POSIX shell quoting under test does not apply on Windows")
25+
}
26+
27+
const shell = "/bin/sh"
28+
commandEnv := func(usershell.EnvInfoer, []string) (string, string, []string, error) {
29+
return shell, "/tmp", []string{"FOO=bar"}, nil
30+
}
31+
e := newCommandEnvExecer(slogtest.Make(t, nil).Leveled(slog.LevelDebug), commandEnv, agentexec.DefaultExecer)
32+
33+
t.Run("ArgvPassthrough", func(t *testing.T) {
34+
t.Parallel()
35+
36+
name, args, dir, env := e.prepare(context.Background(), "echo", "hello", "world")
37+
// The command is run as: shell -c "$@" "" <argv...> so that the
38+
// shell re-emits argv without re-parsing it. The empty $0 slot is
39+
// discarded.
40+
require.Equal(t, shell, name)
41+
require.Equal(t, []string{"-c", `"$@"`, "", "echo", "hello", "world"}, args)
42+
require.Equal(t, "/tmp", dir)
43+
require.Equal(t, []string{"FOO=bar"}, env)
44+
})
45+
46+
t.Run("MetacharactersNotInterpreted", func(t *testing.T) {
47+
t.Parallel()
48+
49+
payloads := []string{
50+
"$(echo INJECTED)",
51+
"`echo INJECTED`",
52+
"$HOME",
53+
"a; echo INJECTED",
54+
"a && echo INJECTED",
55+
"a | echo INJECTED",
56+
"a\necho INJECTED",
57+
"it's a \"test\" \\ end",
58+
"",
59+
}
60+
for _, payload := range payloads {
61+
ctx := testutil.Context(t, testutil.WaitShort)
62+
cmd := e.CommandContext(ctx, "printf", "%s", payload)
63+
var out bytes.Buffer
64+
cmd.Stdout = &out
65+
cmd.Stderr = &out
66+
require.NoError(t, cmd.Run(), "payload %q", payload)
67+
assert.Equal(t, payload, out.String(), "payload %q was altered by the shell", payload)
68+
}
69+
})
70+
71+
t.Run("CommandSubstitutionHasNoSideEffect", func(t *testing.T) {
72+
t.Parallel()
73+
74+
marker := filepath.Join(t.TempDir(), "pwned")
75+
ctx := testutil.Context(t, testutil.WaitShort)
76+
cmd := e.CommandContext(ctx, "echo", "$(touch "+marker+")")
77+
var out bytes.Buffer
78+
cmd.Stdout = &out
79+
cmd.Stderr = &out
80+
require.NoError(t, cmd.Run())
81+
require.Equal(t, "$(touch "+marker+")\n", out.String())
82+
require.NoFileExists(t, marker, "command substitution executed; injection is possible")
83+
})
84+
}

0 commit comments

Comments
 (0)