From 439fe5b586669b9959aab23d42283f83580ba0c2 Mon Sep 17 00:00:00 2001 From: Adao Oliveira Jr Date: Wed, 11 Nov 2026 18:25:03 -0600 Subject: [PATCH] fix(plugin): find a shell's -c script after its options and behind env A plugin whose command is a POSIX shell gets plugin values in its -c script through the environment, so a value from the cluster is never read as shell syntax (SECURITY.md). ExecCommand took the argument right after the first short option group containing c as the script, and only when the command itself was one of the five shells, so values were pasted as text into the script the shell really runs in: - sh -c -x '...', sh -c -o errexit '...' and sh -c -- '...': a shell reads options up to the first argument that does not start with - or +, or up to --, so the script comes after -x, -o errexit or --; - env bash -c '...' and busybox sh -c '...': the command is not a shell. shellCode now reads the options as the shells do: groups that start with - or + (c turns on -c, +c too), o taking the next argument when it ends its group, -- or a lone - ending the options, and bash's long options. It finds the shell behind env, whose options it reads as getopt does, and behind busybox's sh, ash, hush and bash. The script and the options before it get values through the environment; arguments after the script ($0, $1, ...) still get them as text. Where the shells read an option differently, or k10s does not know it (bash's -O, o inside a group, a lone +, other long options, an env option it does not know), every argument gets the environment: safe, though a value meant for $1 then arrives as the variable's name. A Windows build of a shell (bash.exe) counts as the shell. env -S is not a way to a shell: it builds a command line of its own from a string and expands ${VAR} in it, so the environment would not keep a value out of it. It stays a program that reads its own command line, as python3 -c is, and gets values as text. SECURITY.md names env and busybox, and puts env -S with python3 -c; docs/plugins.md says how values reach a -c script. From the review of #28. Signed-off-by: Adao Oliveira Jr --- SECURITY.md | 13 ++- docs/plugins.md | 15 +++ internal/plugin/plugin.go | 199 ++++++++++++++++++++++++++++++--- internal/plugin/plugin_test.go | 104 +++++++++++++++++ 4 files changed, 309 insertions(+), 22 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 6bb1203..ed90611 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -79,12 +79,13 @@ chose to run, so anyone who can write to `~/.k10s/plugins.yaml` can already run commands as you; that is not a vulnerability in k10s. k10s runs the plugin's command directly, never through a shell of its own, and fills object names, namespaces and column values into its arguments as text. When -the command is `sh`, `bash`, `zsh`, `dash` or `ksh` with `-c`, the script -gets them as environment variables instead of having them pasted into its -text. k10s itself turning a value from the cluster into code, or a plugin -with `confirm: true` that runs without asking, is a vulnerability. What a -plugin's own script or program does with those values, as with `python3 -c` -or `kubectl exec ... -- sh -c`, is up to the plugin. See +the command is `sh`, `bash`, `zsh`, `dash` or `ksh` with `-c`, run directly +or through `env` or `busybox`, the script gets them as environment variables +instead of having them pasted into its text. k10s itself turning a value +from the cluster into code, or a plugin with `confirm: true` that runs +without asking, is a vulnerability. What a plugin's own script or program +does with those values, as with `python3 -c`, `env -S` or +`kubectl exec ... -- sh -c`, is up to the plugin. See [docs/plugins.md](docs/plugins.md). **The installer and uninstaller**, `install.sh` and `uninstall.sh`, are in diff --git a/docs/plugins.md b/docs/plugins.md index 411989f..9c14522 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -86,6 +86,21 @@ args: - kubectl get pod "$NAME" -n "$NAMESPACE" -o json | jq .status ``` +k10s does not paste values into the script a shell runs with `-c`. When the +command is `sh`, `bash`, `zsh`, `dash` or `ksh`, run directly or through +`env` or `busybox`, the script reads `$NAME`, `$NAMESPACE` and the other +variables from the environment, and `$COL-STATUS` becomes +`${K10S_COL_}`, so a value is never read as shell syntax. Quote them as +any shell variable: `"$NAME"`, not `'$NAME'`, which the shell leaves as it is. +Options such as `-x` or `-o pipefail` may come before or after `-c`, and `--` +may end them. Arguments after the script, which the shell passes as `$0`, +`$1` and so on, get the values as text. When k10s cannot tell which argument +is the script, for an option it does not read such as bash's `-O`, every +argument reads the values from the environment. `env -S` is not a way to a +shell: it builds a command line of its own from a string and expands +`${VAR}` in it, so k10s fills values into that string as text, as it does +for `python3 -c`. + Foreground commands temporarily leave the alternate screen and inherit the terminal, so interactive tools such as `fzf` work. Background commands have no attached terminal or visible output. diff --git a/internal/plugin/plugin.go b/internal/plugin/plugin.go index 778e9f6..5a6b7ac 100644 --- a/internal/plugin/plugin.go +++ b/internal/plugin/plugin.go @@ -253,8 +253,14 @@ func columnEnvName(column string) string { return "K10S_COL_" + strings.ToUpper(hex.EncodeToString([]byte(column))) } +// programName is how a command is matched against the shells below: its base +// name in lower case, without a Windows .exe. +func programName(command string) string { + return strings.TrimSuffix(strings.ToLower(filepath.Base(command)), ".exe") +} + func isPOSIXShell(command string) bool { - switch strings.ToLower(filepath.Base(command)) { + switch programName(command) { case "sh", "bash", "zsh", "dash", "ksh": return true default: @@ -262,6 +268,178 @@ func isPOSIXShell(command string) bool { } } +// shellStart finds a POSIX shell on a plugin's command line and returns the +// index in args of the shell's own first argument: 0 when the command is the +// shell, 1 for busybox's shell (busybox sh), and after env's options and +// NAME=value arguments for env (env sh, env -i PATH=... sh). env -S is not +// a way to a shell: it builds a command line of its own from a string and +// expands ${VAR} in it, so it is a program that reads its own command line, +// as python3 -c is. unsure is true when env has an option k10s does not +// know, which may hide a shell. +func shellStart(command string, args []string) (start int, shell, unsure bool) { + switch name := programName(command); { + case isPOSIXShell(command): + return 0, true, false + case name == "busybox": + if len(args) > 0 { + switch strings.ToLower(args[0]) { + case "sh", "ash", "hush", "bash": + return 1, true, false + } + } + case name == "env": + i := 0 + options: + for i < len(args) { + switch arg := args[i]; { + case arg == "--": + i++ + break options + case arg == "-" || arg == "--ignore-environment": + i++ + case arg == "--unset" || arg == "--chdir": + i += 2 + case strings.HasPrefix(arg, "--unset=") || strings.HasPrefix(arg, "--chdir="): + i++ + case strings.HasPrefix(arg, "--split-string"): + return 0, false, false + case strings.HasPrefix(arg, "--"): + return 0, false, true + case strings.HasPrefix(arg, "-"): + // A group of short options, read as getopt does: GNU's and + // the BSDs' flags, and the options that take a value, which + // is the rest of the group or else the next argument. + next := 1 + group: + for j := 1; j < len(arg); j++ { + switch arg[j] { + case 'i', 'v', '0': + case 'u', 'C', 'P', 'L', 'U': + if j == len(arg)-1 { + next = 2 + } + break group + case 'S': + return 0, false, false + default: + return 0, false, true + } + } + i += next + default: + break options + } + } + for i < len(args) && strings.Contains(args[i], "=") { + i++ + } + if i < len(args) { + start, shell, unsure := shellStart(args[i], args[i+1:]) + return i + 1 + start, shell, unsure + } + } + return 0, false, false +} + +// plainOptions are the option letters that take no value in any of the +// shells above. +const plainOptions = "aCefhilmnprsuvx" + +// optionLike reports whether a shell reads arg as a group of options. +func optionLike(arg string) bool { + return len(arg) > 1 && (arg[0] == '-' || arg[0] == '+') +} + +// shellScript reads a POSIX shell's arguments the way the shells do and +// returns the index of the script that -c runs: the first argument after the +// options, or -1 when there is no -c. Options are the arguments that start +// with - or +, up to a -- or a lone -, which ends them; c anywhere in a group +// turns on -c, and an o that ends its group takes the next argument as its +// value (-o pipefail, -eo pipefail). So -x, -o errexit or -- after -c come +// before the script, not in its place. unsure is true for an option the +// shells read differently or k10s does not know, such as bash's -O, an o +// inside a group, a lone +, or a long option other than bash's. +func shellScript(args []string) (script int, unsure bool) { + command := false + i := 0 +options: + for i < len(args) { + arg := args[i] + switch { + case arg == "-" || arg == "--": + i++ + break options + case arg == "+": + return -1, true // zsh reads it as --, bash and dash as an empty group + case strings.HasPrefix(arg, "--"): + switch arg { + case "--rcfile", "--init-file": + if i+1 >= len(args) || optionLike(args[i+1]) { + return -1, true + } + i += 2 + case "--debug", "--debugger", "--dump-po-strings", "--dump-strings", + "--help", "--login", "--noediting", "--noprofile", "--norc", + "--posix", "--pretty-print", "--restricted", "--verbose", "--version": + i++ + default: + return -1, true + } + case optionLike(arg): + group := arg[1:] + for j := 0; j < len(group); j++ { + switch letter := group[j]; { + case letter == 'c': + command = true + case letter == 'o' && j == len(group)-1: + if i+1 >= len(args) || optionLike(args[i+1]) { + return -1, true + } + i++ + case strings.IndexByte(plainOptions, letter) < 0: + return -1, true + } + } + i++ + default: + break options + } + } + if !command || i >= len(args) { + return -1, false + } + return i, false +} + +// shellCode marks the arguments that a POSIX shell on the plugin's command +// line may run as code: the -c script, and the options before it. Those get +// plugin values through the environment, never as text. When k10s cannot +// tell which argument is the script, every argument is marked; a value meant +// for an argument after the script, which the shell passes as $0, $1 and so +// on, then arrives as the variable's name, which is safe if not what was +// meant. +func shellCode(command string, args []string) []bool { + code := make([]bool, len(args)) + start, shell, unsure := shellStart(command, args) + last := -1 + switch { + case unsure: + start, last = 0, len(args)-1 + case shell: + script, unsure := shellScript(args[start:]) + switch { + case unsure: + last = len(args) - 1 + case script >= 0: + last = start + script + } + } + for i := start; i <= last; i++ { + code[i] = true + } + return code +} + // expandShellColumns maps k9s's $COL-NAME syntax (not a valid POSIX variable // name) to private environment variables. Other plugin variables stay intact // and are expanded by the shell from the environment. Shell expansion does @@ -291,24 +469,13 @@ func (p Plugin) DisplayCommand(vars Vars) string { } // ExecCommand constructs a process without involving a shell. Shell behavior -// remains available by configuring command: sh and args: [-c, ...], as in k9s. +// remains available by configuring command: sh and args: [-c, ...], as in k9s; +// shellCode decides which arguments get plugin values through the environment. func (p Plugin) ExecCommand(vars Vars) *exec.Cmd { + code := shellCode(p.Command, p.Args) args := make([]string, len(p.Args)) - shellCode := -1 - if isPOSIXShell(p.Command) { - for i, arg := range p.Args { - // POSIX shells accept -c and grouped short flags such as -lc. - // Long options (--norc, --rcfile) are not short flag groups even - // when their spelling happens to contain the letter c. - shortFlags := len(arg) >= 2 && arg[0] == '-' && arg[1] != '-' - if shortFlags && strings.Contains(arg[1:], "c") && i+1 < len(p.Args) { - shellCode = i + 1 - break - } - } - } for i, arg := range p.Args { - if i == shellCode { + if code[i] { args[i] = expandShellColumns(arg, vars) } else { args[i] = Expand(arg, vars) diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go index 5d8249c..279557f 100644 --- a/internal/plugin/plugin_test.go +++ b/internal/plugin/plugin_test.go @@ -239,3 +239,107 @@ func TestExecCommandOverridesExistingPluginEnvironmentValues(t *testing.T) { t.Errorf("USER = %q, want kubeconfig identity", out) } } + +func TestShellCodeMarksWhatTheShellMayRunAsCode(t *testing.T) { + // One mark per argument: c when it gets plugin values through the + // environment, - when it gets them as text. + for _, tc := range []struct { + command string + args []string + marks string + }{ + // The -c script, and the options before it. + {"sh", []string{"-c", "script", "sh", "$NAME"}, "cc--"}, + {"/bin/sh", []string{"-ec", "script"}, "cc"}, + {"bash.exe", []string{"-lc", "script"}, "cc"}, + {"sh", []string{"-c", "-x", "script", "sh"}, "ccc-"}, + {"sh", []string{"-c", "-o", "errexit", "script"}, "cccc"}, + {"sh", []string{"-o", "errexit", "-c", "script", "sh", "$NAME"}, "cccc--"}, + {"bash", []string{"-euxo", "pipefail", "-c", "script"}, "cccc"}, + {"sh", []string{"-co", "errexit", "script", "sh"}, "ccc-"}, + {"sh", []string{"+x", "-c", "script"}, "ccc"}, + {"sh", []string{"+c", "script", "sh"}, "cc-"}, + {"sh", []string{"-c", "--", "script", "sh"}, "ccc-"}, + {"sh", []string{"-c", "-", "script"}, "ccc"}, + {"bash", []string{"--norc", "--noprofile", "-c", "script"}, "cccc"}, + {"bash", []string{"--rcfile", "rc", "-c", "script", "bash"}, "cccc-"}, + // A shell run through env or busybox. + {"env", []string{"bash", "-c", "script", "bash"}, "-cc-"}, + {"/usr/bin/env", []string{"-i", "PATH=/bin", "sh", "-c", "script"}, "---cc"}, + {"env", []string{"-u", "HOME", "--", "A=$NAME", "sh", "-c", "script"}, "-----cc"}, + {"busybox", []string{"sh", "-c", "script", "sh"}, "-cc-"}, + {"env", []string{"busybox", "ash", "-c", "script"}, "--cc"}, + {"env", []string{"-C", "/tmp", "sh", "-c", "script"}, "---cc"}, + {"env", []string{"-iu", "HOME", "sh", "-c", "script"}, "---cc"}, + {"env", []string{"-iuS", "sh", "-c", "script"}, "--cc"}, + // No -c script: a script file and its arguments, or not a shell. + {"sh", []string{"script.sh", "$NAME"}, "--"}, + {"sh", []string{"script.sh", "-c", "$NAME"}, "---"}, + {"sh", []string{"-c"}, "-"}, + {"python3", []string{"-c", "print('$NAME')"}, "--"}, + {"env", []string{"python3", "-c", "x"}, "---"}, + {"env", []string{"-C", "/tmp", "python3", "x.py", "$NAME"}, "-----"}, + // env -S builds a command line of its own and expands ${VAR} in it: + // a program reading its own command line, like python3 -c. + {"env", []string{"-S", "sh -c script"}, "--"}, + {"env", []string{"-vS", "sh -c script"}, "--"}, + {"env", []string{"--split-string=sh -c script"}, "-"}, + // Options k10s cannot read with certainty: every argument. + {"bash", []string{"-O", "extglob", "-c", "script", "bash", "$NAME"}, "cccccc"}, + {"sh", []string{"-oc", "errexit", "script"}, "ccc"}, + {"sh", []string{"-c", "-o"}, "cc"}, + {"zsh", []string{"--no-rcs", "-c", "script"}, "ccc"}, + {"sh", []string{"+", "-c", "script"}, "ccc"}, + {"env", []string{"-x", "sh", "-c", "script"}, "cccc"}, + } { + var got strings.Builder + for _, code := range shellCode(tc.command, tc.args) { + if code { + got.WriteByte('c') + } else { + got.WriteByte('-') + } + } + if got.String() != tc.marks { + t.Errorf("%s %q: marks %s, want %s", tc.command, tc.args, got.String(), tc.marks) + } + } +} + +func TestShellOptionsAfterDashCDoNotLetValuesIntoTheScript(t *testing.T) { + script := `printf '%s' "$COL-STATUS" > "$OUT"` + for _, tc := range []struct { + command string + args []string + }{ + {"sh", []string{"-c", "-x", script}}, + {"sh", []string{"-c", "-o", "errexit", script}}, + {"sh", []string{"-c", "--", script}}, + {"sh", []string{"+x", "-c", script}}, + {"bash", []string{"-euo", "pipefail", "-c", script}}, + {"env", []string{"sh", "-c", script}}, + } { + t.Run(tc.command+" "+strings.Join(tc.args[:len(tc.args)-1], " "), func(t *testing.T) { + dir := t.TempDir() + out := filepath.Join(dir, "out") + pwned := filepath.Join(dir, "pwned") + t.Setenv("OUT", out) + payload := `"; touch ` + pwned + `; # $(touch ignored)` + p := Plugin{Command: tc.command, Args: tc.args} + + if err := p.ExecCommand(Vars{Columns: map[string]string{"STATUS": payload}}).Run(); err != nil { + t.Fatalf("Run: %v", err) + } + if _, err := os.Stat(pwned); !os.IsNotExist(err) { + t.Fatalf("cluster value was reparsed as shell syntax; pwned stat err=%v", err) + } + got, err := os.ReadFile(out) + if err != nil { + t.Fatal(err) + } + if string(got) != payload { + t.Errorf("shell received %q, want literal payload %q", got, payload) + } + }) + } +}