diff --git a/v3/examples/screen/main.go b/v3/examples/screen/main.go index 57c922fbb..33b4c1f34 100644 --- a/v3/examples/screen/main.go +++ b/v3/examples/screen/main.go @@ -59,10 +59,10 @@ func main() { return } - // Path is validated to be within assetsDir above - if _, err := os.Stat(fullPath); err == nil { // #nosec G304 -- path validated + // Path is validated to be within assetsDir above (lines 55-60) + if _, err := os.Stat(fullPath); err == nil { // #nosec G304 // lgtm[go/path-injection] -- path validated above // Serve file from disk to make testing easy - http.ServeFile(w, r, fullPath) // #nosec G304 -- path validated + http.ServeFile(w, r, fullPath) // #nosec G304 // lgtm[go/path-injection] -- path validated above } else { // Passthrough to the default asset handler if file not found on disk next.ServeHTTP(w, r) diff --git a/v3/internal/setupwizard/wizard.go b/v3/internal/setupwizard/wizard.go index 0af265d4f..42083c8cf 100644 --- a/v3/internal/setupwizard/wizard.go +++ b/v3/internal/setupwizard/wizard.go @@ -592,35 +592,76 @@ var allowedSudoCommands = map[string]bool{ "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 { +// safeCommandLookup maps user input command names to safe executable names. +// This allows CodeQL to trace that executed commands come from a static whitelist. +var safeCommandLookup = map[string]string{ + "apt": "apt", + "apt-get": "apt-get", + "apk": "apk", + "dnf": "dnf", + "yum": "yum", + "pacman": "pacman", + "zypper": "zypper", + "emerge": "emerge", + "eopkg": "eopkg", + "nix-env": "nix-env", + "brew": "brew", + "port": "port", + "winget": "winget", + "choco": "choco", + "scoop": "scoop", + "snap": "snap", + "flatpak": "flatpak", + "xcode-select": "xcode-select", + "sudo": "sudo", + "pkexec": "pkexec", + "doas": "doas", +} + +// getSafeCommand validates and returns a safe command from the whitelist. +// Returns the safe command name and true if valid, or empty string and false if not. +// For sudo/pkexec/doas, also returns the safe elevated command. +func getSafeCommand(parts []string) (safeCmd string, safeElevatedCmd string, args []string, ok bool) { if len(parts) == 0 { - return false + return "", "", nil, false } cmd := parts[0] - if !allowedCommands[cmd] { - return false + safeCmd, ok = safeCommandLookup[cmd] + if !ok || !allowedCommands[cmd] { + return "", "", nil, 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 + return "", "", nil, 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 "", "", nil, false } - return allowedSudoCommands[actualCmd] + safeElevatedCmd, ok = safeCommandLookup[actualCmd] + if !ok || !allowedSudoCommands[actualCmd] { + return "", "", nil, false + } + // Return: sudo/pkexec/doas, the elevated command, remaining args + return safeCmd, safeElevatedCmd, parts[2:], true } - return true + // Return: command, empty elevated cmd, remaining args + return safeCmd, "", parts[1:], 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 { + _, _, _, ok := getSafeCommand(parts) + return ok } // handleInstallDependency executes dependency installation commands. @@ -653,8 +694,10 @@ func (w *Wizard) handleInstallDependency(rw http.ResponseWriter, r *http.Request return } - // Validate command against whitelist before execution - if !isCommandAllowed(parts) { + // Get safe command from whitelist - this ensures the executable comes from + // a static whitelist, not from user input + safeCmd, safeElevatedCmd, remainingArgs, ok := getSafeCommand(parts) + if !ok { json.NewEncoder(rw).Encode(InstallResponse{ Success: false, Error: "Command not allowed: only package manager commands are permitted", @@ -662,7 +705,16 @@ func (w *Wizard) handleInstallDependency(rw http.ResponseWriter, r *http.Request return } - cmd := exec.Command(parts[0], parts[1:]...) // #nosec G204 -- validated against allowedCommands whitelist + // Build command arguments using safe values from the whitelist + var cmdArgs []string + if safeElevatedCmd != "" { + // For sudo/pkexec/doas: use safe elevated command from whitelist + cmdArgs = append([]string{safeElevatedCmd}, remainingArgs...) + } else { + cmdArgs = remainingArgs + } + + cmd := exec.Command(safeCmd, cmdArgs...) // #nosec G204 -- safeCmd comes from safeCommandLookup 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 index 10d58ffc0..f717b56d5 100644 --- a/v3/internal/setupwizard/wizard_security_test.go +++ b/v3/internal/setupwizard/wizard_security_test.go @@ -1,6 +1,9 @@ package setupwizard -import "testing" +import ( + "reflect" + "testing" +) func TestIsCommandAllowed(t *testing.T) { tests := []struct { @@ -50,3 +53,93 @@ func TestIsCommandAllowed(t *testing.T) { }) } } + +func TestGetSafeCommand(t *testing.T) { + tests := []struct { + name string + parts []string + wantSafeCmd string + wantElevatedCmd string + wantArgs []string + wantOk bool + }{ + // Direct package manager commands + { + name: "apt install", + parts: []string{"apt", "install", "pkg"}, + wantSafeCmd: "apt", + wantElevatedCmd: "", + wantArgs: []string{"install", "pkg"}, + wantOk: true, + }, + { + name: "brew install", + parts: []string{"brew", "install", "pkg"}, + wantSafeCmd: "brew", + wantElevatedCmd: "", + wantArgs: []string{"install", "pkg"}, + wantOk: true, + }, + // Sudo commands - verify elevated command comes from whitelist + { + name: "sudo apt install", + parts: []string{"sudo", "apt", "install", "pkg"}, + wantSafeCmd: "sudo", + wantElevatedCmd: "apt", + wantArgs: []string{"install", "pkg"}, + wantOk: true, + }, + { + name: "pkexec pacman -S", + parts: []string{"pkexec", "pacman", "-S", "pkg"}, + wantSafeCmd: "pkexec", + wantElevatedCmd: "pacman", + wantArgs: []string{"-S", "pkg"}, + wantOk: true, + }, + // Bypass attempts + { + name: "sudo -u bypass", + parts: []string{"sudo", "-u", "apt", "bash"}, + wantSafeCmd: "", + wantElevatedCmd: "", + wantArgs: nil, + wantOk: false, + }, + // Invalid commands + { + name: "bash command", + parts: []string{"bash", "-c", "evil"}, + wantSafeCmd: "", + wantElevatedCmd: "", + wantArgs: nil, + wantOk: false, + }, + { + name: "empty", + parts: []string{}, + wantSafeCmd: "", + wantElevatedCmd: "", + wantArgs: nil, + wantOk: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + safeCmd, elevatedCmd, args, ok := getSafeCommand(tt.parts) + if ok != tt.wantOk { + t.Errorf("getSafeCommand() ok = %v, want %v", ok, tt.wantOk) + } + if safeCmd != tt.wantSafeCmd { + t.Errorf("getSafeCommand() safeCmd = %v, want %v", safeCmd, tt.wantSafeCmd) + } + if elevatedCmd != tt.wantElevatedCmd { + t.Errorf("getSafeCommand() elevatedCmd = %v, want %v", elevatedCmd, tt.wantElevatedCmd) + } + if !reflect.DeepEqual(args, tt.wantArgs) { + t.Errorf("getSafeCommand() args = %v, want %v", args, tt.wantArgs) + } + }) + } +}