fix(security): improve command injection protection for CodeQL

- Refactor whitelist validation to use getSafeCommand() which returns
  safe command names from a static lookup table instead of user input
- This allows CodeQL to trace that executed commands come from a
  known-safe whitelist rather than tainted user input
- Add comprehensive tests for the new getSafeCommand function
- Add lgtm[go/path-injection] comments for CodeQL suppression on the
  example file where paths are properly validated

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Lea Anthony 2026-01-25 11:46:29 +11:00
commit 4fc28b9d61
3 changed files with 164 additions and 19 deletions

View file

@ -59,10 +59,10 @@ func main() {
return return
} }
// Path is validated to be within assetsDir above // Path is validated to be within assetsDir above (lines 55-60)
if _, err := os.Stat(fullPath); err == nil { // #nosec G304 -- path validated if _, err := os.Stat(fullPath); err == nil { // #nosec G304 // lgtm[go/path-injection] -- path validated above
// Serve file from disk to make testing easy // 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 { } else {
// Passthrough to the default asset handler if file not found on disk // Passthrough to the default asset handler if file not found on disk
next.ServeHTTP(w, r) next.ServeHTTP(w, r)

View file

@ -592,35 +592,76 @@ var allowedSudoCommands = map[string]bool{
"xcode-select": true, "xcode-select": true,
} }
// isCommandAllowed checks if a command is in the whitelist. // safeCommandLookup maps user input command names to safe executable names.
// For sudo/pkexec/doas, it validates the actual command being elevated. // This allows CodeQL to trace that executed commands come from a static whitelist.
// To avoid bypass attacks via flag parsing (e.g., "sudo -u apt bash"), var safeCommandLookup = map[string]string{
// we reject any sudo invocation where the second argument starts with "-". "apt": "apt",
func isCommandAllowed(parts []string) bool { "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 { if len(parts) == 0 {
return false return "", "", nil, false
} }
cmd := parts[0] cmd := parts[0]
if !allowedCommands[cmd] { safeCmd, ok = safeCommandLookup[cmd]
return false if !ok || !allowedCommands[cmd] {
return "", "", nil, false
} }
// If it's a privilege escalation command, validate the actual command // If it's a privilege escalation command, validate the actual command
if cmd == "sudo" || cmd == "pkexec" || cmd == "doas" { if cmd == "sudo" || cmd == "pkexec" || cmd == "doas" {
if len(parts) < 2 { if len(parts) < 2 {
return false return "", "", nil, false
} }
// Reject any flags before the command to prevent bypass attacks // Reject any flags before the command to prevent bypass attacks
// like "sudo -u apt bash" where -u takes "apt" as its argument // like "sudo -u apt bash" where -u takes "apt" as its argument
actualCmd := parts[1] actualCmd := parts[1]
if strings.HasPrefix(actualCmd, "-") { 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. // handleInstallDependency executes dependency installation commands.
@ -653,8 +694,10 @@ func (w *Wizard) handleInstallDependency(rw http.ResponseWriter, r *http.Request
return return
} }
// Validate command against whitelist before execution // Get safe command from whitelist - this ensures the executable comes from
if !isCommandAllowed(parts) { // a static whitelist, not from user input
safeCmd, safeElevatedCmd, remainingArgs, ok := getSafeCommand(parts)
if !ok {
json.NewEncoder(rw).Encode(InstallResponse{ json.NewEncoder(rw).Encode(InstallResponse{
Success: false, Success: false,
Error: "Command not allowed: only package manager commands are permitted", 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 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() output, err := cmd.CombinedOutput()
if err != nil { if err != nil {

View file

@ -1,6 +1,9 @@
package setupwizard package setupwizard
import "testing" import (
"reflect"
"testing"
)
func TestIsCommandAllowed(t *testing.T) { func TestIsCommandAllowed(t *testing.T) {
tests := []struct { 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)
}
})
}
}