fix(plugin): find a shell's -c script after its options and behind env

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

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_<hex>}`, 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.

internal/plugin/plugin.go

@@ -253,15 +253,193 @@ 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:
 		return false
 	}
 }
 
+// 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)

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)
+			}
+		})
+	}
+}