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