From b97ca22a488e0325f7d091bdce73269a9a1de80f Mon Sep 17 00:00:00 2001 From: Lea Anthony Date: Sun, 25 Jan 2026 11:20:59 +1100 Subject: [PATCH] fix(security): add command whitelist to prevent command injection bypass This commit addresses a critical security issue identified by CodeRabbit where the sudo flag-skipping logic could be bypassed to execute arbitrary commands (e.g., "sudo -u apt bash -c malicious_command"). Changes: - Add allowedCommands whitelist for package managers - Add allowedSudoCommands whitelist for commands after sudo/pkexec/doas - Implement isCommandAllowed() with secure validation that rejects any sudo invocation with flags before the command - Add comprehensive test cases including bypass attack scenarios The fix follows CodeRabbit's recommendation to not attempt parsing sudo flags, instead requiring the package manager to immediately follow the privilege escalation command. Co-Authored-By: Claude Opus 4.5 --- v3/internal/setupwizard/wizard.go | 96 +++++++++++++++++-- .../setupwizard/wizard_security_test.go | 52 ++++++++++ 2 files changed, 142 insertions(+), 6 deletions(-) create mode 100644 v3/internal/setupwizard/wizard_security_test.go diff --git a/v3/internal/setupwizard/wizard.go b/v3/internal/setupwizard/wizard.go index 2679f41f2..0af265d4f 100644 --- a/v3/internal/setupwizard/wizard.go +++ b/v3/internal/setupwizard/wizard.go @@ -544,15 +544,90 @@ type InstallResponse struct { Error string `json:"error,omitempty"` } +// allowedCommands is a whitelist of commands that can be executed for dependency installation. +// This prevents arbitrary command execution even though commands originate from backend detection. +var allowedCommands = map[string]bool{ + // Package managers (may be called directly or via sudo) + "apt": true, + "apt-get": true, + "apk": true, // Alpine Linux + "dnf": true, + "yum": true, + "pacman": true, + "zypper": true, + "emerge": true, + "eopkg": true, + "nix-env": true, + "brew": true, + "port": true, // MacPorts + "winget": true, + "choco": true, + "scoop": true, + "snap": true, + "flatpak": true, + "xcode-select": true, // macOS Xcode CLI tools + // Privilege escalation (validated separately) + "sudo": true, + "pkexec": true, + "doas": true, +} + +// allowedSudoCommands are commands allowed to be run after sudo/pkexec/doas. +// This is a subset of allowedCommands - privilege escalation wrappers are not allowed here. +var allowedSudoCommands = map[string]bool{ + "apt": true, + "apt-get": true, + "apk": true, + "dnf": true, + "yum": true, + "pacman": true, + "zypper": true, + "emerge": true, + "eopkg": true, + "nix-env": true, + "brew": true, + "port": true, + "snap": true, + "flatpak": true, + "xcode-select": true, +} + +// isCommandAllowed checks if a command is in the whitelist. +// For sudo/pkexec/doas, it validates the actual command being elevated. +// To avoid bypass attacks via flag parsing (e.g., "sudo -u apt bash"), +// we reject any sudo invocation where the second argument starts with "-". +func isCommandAllowed(parts []string) bool { + if len(parts) == 0 { + return false + } + + cmd := parts[0] + if !allowedCommands[cmd] { + return false + } + + // If it's a privilege escalation command, validate the actual command + if cmd == "sudo" || cmd == "pkexec" || cmd == "doas" { + if len(parts) < 2 { + return false + } + // Reject any flags before the command to prevent bypass attacks + // like "sudo -u apt bash" where -u takes "apt" as its argument + actualCmd := parts[1] + if strings.HasPrefix(actualCmd, "-") { + return false + } + return allowedSudoCommands[actualCmd] + } + + return true +} + // handleInstallDependency executes dependency installation commands. // // Security note: This endpoint executes commands that originate from the backend's // package manager detection (see packagemanager.InstallCommand). The commands are -// generated server-side based on the detected OS package manager, not from arbitrary -// user input. This is a local development tool where the explicit purpose is to help -// users install dependencies on their own machine. The frontend currently only uses -// this data to copy commands to clipboard - this endpoint exists for potential future -// use or automation scenarios. +// validated against a whitelist of allowed package managers before execution. func (w *Wizard) handleInstallDependency(rw http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { http.Error(rw, "Method not allowed", http.StatusMethodNotAllowed) @@ -578,7 +653,16 @@ func (w *Wizard) handleInstallDependency(rw http.ResponseWriter, r *http.Request return } - cmd := exec.Command(parts[0], parts[1:]...) // #nosec G204 -- commands originate from backend package manager detection, not user input + // Validate command against whitelist before execution + if !isCommandAllowed(parts) { + json.NewEncoder(rw).Encode(InstallResponse{ + Success: false, + Error: "Command not allowed: only package manager commands are permitted", + }) + return + } + + cmd := exec.Command(parts[0], parts[1:]...) // #nosec G204 -- validated against allowedCommands whitelist output, err := cmd.CombinedOutput() if err != nil { diff --git a/v3/internal/setupwizard/wizard_security_test.go b/v3/internal/setupwizard/wizard_security_test.go new file mode 100644 index 000000000..10d58ffc0 --- /dev/null +++ b/v3/internal/setupwizard/wizard_security_test.go @@ -0,0 +1,52 @@ +package setupwizard + +import "testing" + +func TestIsCommandAllowed(t *testing.T) { + tests := []struct { + name string + parts []string + allowed bool + }{ + // Valid package manager commands + {"apt install", []string{"apt", "install", "pkg"}, true}, + {"apt-get install", []string{"apt-get", "install", "pkg"}, true}, + {"brew install", []string{"brew", "install", "pkg"}, true}, + {"pacman -S", []string{"pacman", "-S", "pkg"}, true}, + {"dnf install", []string{"dnf", "install", "pkg"}, true}, + + // Valid sudo commands + {"sudo apt install", []string{"sudo", "apt", "install", "pkg"}, true}, + {"sudo apt-get install", []string{"sudo", "apt-get", "install", "pkg"}, true}, + {"sudo pacman -S", []string{"sudo", "pacman", "-S", "pkg"}, true}, + {"pkexec apt install", []string{"pkexec", "apt", "install", "pkg"}, true}, + {"doas apt install", []string{"doas", "apt", "install", "pkg"}, true}, + + // CRITICAL: Bypass attempts that MUST be blocked + {"sudo -u apt bash (bypass)", []string{"sudo", "-u", "apt", "bash", "-c", "malicious"}, false}, + {"sudo -E bash", []string{"sudo", "-E", "bash"}, false}, + {"sudo --user=root bash", []string{"sudo", "--user=root", "bash"}, false}, + {"doas -u apt bash", []string{"doas", "-u", "apt", "bash"}, false}, + {"pkexec --user apt bash", []string{"pkexec", "--user", "apt", "bash"}, false}, + + // Invalid commands + {"bash", []string{"bash", "-c", "malicious"}, false}, + {"rm -rf", []string{"rm", "-rf", "/"}, false}, + {"curl", []string{"curl", "http://evil.com"}, false}, + {"wget", []string{"wget", "http://evil.com"}, false}, + {"empty", []string{}, false}, + {"sudo only", []string{"sudo"}, false}, + + // Nested sudo attempts + {"sudo sudo apt", []string{"sudo", "sudo", "apt"}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isCommandAllowed(tt.parts) + if got != tt.allowed { + t.Errorf("isCommandAllowed(%v) = %v, want %v", tt.parts, got, tt.allowed) + } + }) + } +}