Merge pull request #2083 from dgageot/secu

fix: prevent PATH hijacking and TOCTOU in shell/binary resolution

Author: dgageot

pkg/environment/cmd_provider.go

@@ -11,6 +11,8 @@ import (
 
 // runCommand executes a command and returns its trimmed stdout.
 // Returns ("", false) if the command fails or is not found.
+// The name parameter should be a fully-qualified path to the binary
+// (as returned by lookupBinary) to prevent PATH hijacking (CWE-426).
 func runCommand(ctx context.Context, logLabel, name string, args ...string) (string, bool) {
 	var stdout, stderr bytes.Buffer
 
@@ -26,15 +28,17 @@ func runCommand(ctx context.Context, logLabel, name string, args ...string) (str
 	return strings.TrimSpace(stdout.String()), true
 }
 
-// lookupBinary checks if a binary is available on the system PATH.
-// Returns a non-nil error if the binary is not found.
-func lookupBinary(name string, notFoundErr error) error {
+// lookupBinary checks if a binary is available on the system PATH and returns
+// its absolute path. Returns a non-nil error if the binary is not found.
+// The returned path should be stored and reused (rather than the bare name)
+// to avoid TOCTOU races and PATH hijacking.
+func lookupBinary(name string, notFoundErr error) (string, error) {
 	path, err := exec.LookPath(name)
 	if err != nil && !errors.Is(err, exec.ErrNotFound) {
 		slog.Warn("failed to lookup `"+name+"` binary", "error", err)
 	}
 	if path == "" {
-		return notFoundErr
+		return "", notFoundErr
 	}
-	return nil
+	return path, nil
 }

pkg/environment/keychain.go

@@ -4,7 +4,11 @@ import "context"
 
 // KeychainProvider is a provider that retrieves secrets using the macOS keychain
 // via the `security` command-line tool.
-type KeychainProvider struct{}
+type KeychainProvider struct {
+	// binaryPath is the absolute path to the `security` binary, resolved at
+	// construction time to avoid TOCTOU races and PATH hijacking (CWE-426).
+	binaryPath string
+}
 
 type KeychainNotAvailableError struct{}
 
@@ -13,16 +17,18 @@ func (KeychainNotAvailableError) Error() string {
 }
 
 // NewKeychainProvider creates a new KeychainProvider instance.
-// It verifies that the `security` command is available on the system.
+// It verifies that the `security` command is available on the system and
+// stores the resolved absolute path for later use.
 func NewKeychainProvider() (*KeychainProvider, error) {
-	if err := lookupBinary("security", KeychainNotAvailableError{}); err != nil {
+	path, err := lookupBinary("security", KeychainNotAvailableError{})
+	if err != nil {
 		return nil, err
 	}
-	return &KeychainProvider{}, nil
+	return &KeychainProvider{binaryPath: path}, nil
 }
 
 // Get retrieves the value of a secret by its service name from the macOS keychain.
 // It uses the `security find-generic-password -w -s <name>` command to fetch the password.
 func (p *KeychainProvider) Get(ctx context.Context, name string) (string, bool) {
-	return runCommand(ctx, "keychain", "security", "find-generic-password", "-w", "-s", name)
+	return runCommand(ctx, "keychain", p.binaryPath, "find-generic-password", "-w", "-s", name)
 }

pkg/environment/pass.go

@@ -4,7 +4,11 @@ import "context"
 
 // PassProvider is a provider that retrieves secrets using the `pass` password
 // manager.
-type PassProvider struct{}
+type PassProvider struct {
+	// binaryPath is the absolute path to the `pass` binary, resolved at
+	// construction time to avoid TOCTOU races and PATH hijacking (CWE-426).
+	binaryPath string
+}
 
 type PassNotAvailableError struct{}
 
@@ -13,15 +17,17 @@ func (PassNotAvailableError) Error() string {
 }
 
 // NewPassProvider creates a new PassProvider instance.
+// It verifies that `pass` is available and stores the resolved absolute path.
 func NewPassProvider() (*PassProvider, error) {
-	if err := lookupBinary("pass", PassNotAvailableError{}); err != nil {
+	path, err := lookupBinary("pass", PassNotAvailableError{})
+	if err != nil {
 		return nil, err
 	}
-	return &PassProvider{}, nil
+	return &PassProvider{binaryPath: path}, nil
 }
 
 // Get retrieves the value of a secret by its name using the `pass` CLI.
 // The name corresponds to the path in the `pass` store.
 func (p *PassProvider) Get(ctx context.Context, name string) (string, bool) {
-	return runCommand(ctx, "pass", "pass", "show", name)
+	return runCommand(ctx, "pass", p.binaryPath, "show", name)
 }

pkg/hooks/executor.go

@@ -2,19 +2,18 @@ package hooks
 
 import (
 	"bytes"
-	"cmp"
 	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"log/slog"
 	"maps"
-	"os"
 	"os/exec"
 	"regexp"
-	"runtime"
 	"strings"
 	"sync"
+
+	"github.com/docker/docker-agent/pkg/shellpath"
 )
 
 // Executor handles the execution of hooks
@@ -64,24 +63,11 @@ func NewExecutor(config *Config, workingDir string, env []string) *Executor {
 	return e
 }
 
-// initShell initializes the shell configuration based on the OS
+// initShell initializes the shell configuration based on the OS.
+// It uses shellpath.DetectShell which resolves shell binaries via absolute
+// paths to prevent PATH hijacking (CWE-426).
 func (e *Executor) initShell() {
-	if runtime.GOOS == "windows" {
-		// Prefer PowerShell when available
-		if path, err := exec.LookPath("pwsh.exe"); err == nil {
-			e.shell = path
-			e.shellArgsPrefix = []string{"-NoProfile", "-NonInteractive", "-Command"}
-		} else if path, err := exec.LookPath("powershell.exe"); err == nil {
-			e.shell = path
-			e.shellArgsPrefix = []string{"-NoProfile", "-NonInteractive", "-Command"}
-		} else {
-			e.shell = cmp.Or(os.Getenv("ComSpec"), "cmd.exe")
-			e.shellArgsPrefix = []string{"/C"}
-		}
-	} else {
-		e.shell = cmp.Or(os.Getenv("SHELL"), "/bin/sh")
-		e.shellArgsPrefix = []string{"-c"}
-	}
+	e.shell, e.shellArgsPrefix = shellpath.DetectShell()
 }
 
 // compileMatchers pre-compiles all matcher regex patterns

pkg/shellpath/shellpath.go

@@ -0,0 +1,73 @@
+// Package shellpath provides safe shell binary resolution to prevent
+// PATH hijacking attacks (CWE-426).
+package shellpath
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"runtime"
+)
+
+// WindowsCmdExe returns the absolute path to cmd.exe on Windows using the
+// SystemRoot environment variable (e.g. C:\Windows\System32\cmd.exe).
+// This avoids resolving cmd.exe through PATH, which would be vulnerable
+// to untrusted search path attacks (CWE-426).
+//
+// If the ComSpec environment variable is set, its value is returned as-is
+// (ComSpec is typically set by Windows itself to the correct cmd.exe path).
+//
+// As a last resort, if neither ComSpec nor SystemRoot is set, it falls back
+// to the bare "cmd.exe" name (should never happen on a normal Windows system).
+func WindowsCmdExe() string {
+	if comspec := os.Getenv("ComSpec"); comspec != "" {
+		return comspec
+	}
+	if systemRoot := os.Getenv("SystemRoot"); systemRoot != "" {
+		return filepath.Join(systemRoot, "System32", "cmd.exe")
+	}
+	return "cmd.exe"
+}
+
+// DetectShell returns the appropriate shell binary and its argument prefix
+// for the current platform.
+//
+// On Windows, it prefers PowerShell (pwsh.exe or powershell.exe) resolved
+// via exec.LookPath (which returns an absolute path on success), falling
+// back to cmd.exe resolved through [WindowsCmdExe].
+//
+// On Unix, it uses the SHELL environment variable or /bin/sh.
+func DetectShell() (shell string, argsPrefix []string) {
+	if runtime.GOOS == "windows" {
+		return DetectWindowsShell()
+	}
+
+	return defaultUnixShell(), []string{"-c"}
+}
+
+// DetectWindowsShell returns the shell binary and argument prefix for Windows.
+// It prefers PowerShell (resolved via LookPath, which returns an absolute path),
+// falling back to cmd.exe via [WindowsCmdExe].
+func DetectWindowsShell() (shell string, argsPrefix []string) {
+	powershellArgs := []string{"-NoProfile", "-NonInteractive", "-Command"}
+	for _, ps := range []string{"pwsh.exe", "powershell.exe"} {
+		if path, err := exec.LookPath(ps); err == nil {
+			return path, powershellArgs
+		}
+	}
+	return WindowsCmdExe(), []string{"/C"}
+}
+
+// DetectUnixShell returns the user's shell from the SHELL environment variable,
+// falling back to /bin/sh.
+func DetectUnixShell() string {
+	return defaultUnixShell()
+}
+
+// defaultUnixShell returns the user's shell from SHELL or /bin/sh.
+func defaultUnixShell() string {
+	if shell := os.Getenv("SHELL"); shell != "" {
+		return shell
+	}
+	return "/bin/sh"
+}

pkg/shellpath/shellpath_test.go

@@ -0,0 +1,103 @@
+package shellpath
+
+import (
+	"path/filepath"
+	"runtime"
+	"testing"
+)
+
+func TestWindowsCmdExe_ComSpec(t *testing.T) {
+	t.Setenv("ComSpec", `C:\Custom\cmd.exe`)
+	got := WindowsCmdExe()
+	if got != `C:\Custom\cmd.exe` {
+		t.Errorf("WindowsCmdExe() = %q, want %q", got, `C:\Custom\cmd.exe`)
+	}
+}
+
+func TestWindowsCmdExe_SystemRoot(t *testing.T) {
+	t.Setenv("ComSpec", "")
+	t.Setenv("SystemRoot", `C:\Windows`)
+	got := WindowsCmdExe()
+	want := `C:\Windows` + string(filepath.Separator) + filepath.Join("System32", "cmd.exe")
+	if got != want {
+		t.Errorf("WindowsCmdExe() = %q, want %q", got, want)
+	}
+}
+
+func TestWindowsCmdExe_Fallback(t *testing.T) {
+	t.Setenv("ComSpec", "")
+	t.Setenv("SystemRoot", "")
+	got := WindowsCmdExe()
+	if got != "cmd.exe" {
+		t.Errorf("WindowsCmdExe() = %q, want %q", got, "cmd.exe")
+	}
+}
+
+func TestDetectShell_Unix(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Unix-only test")
+	}
+
+	t.Setenv("SHELL", "/bin/zsh")
+	shell, args := DetectShell()
+	if shell != "/bin/zsh" {
+		t.Errorf("DetectShell() shell = %q, want /bin/zsh", shell)
+	}
+	if len(args) != 1 || args[0] != "-c" {
+		t.Errorf("DetectShell() args = %v, want [-c]", args)
+	}
+}
+
+func TestDetectShell_Unix_Fallback(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Unix-only test")
+	}
+
+	t.Setenv("SHELL", "")
+	shell, _ := DetectShell()
+	if shell != "/bin/sh" {
+		t.Errorf("DetectShell() shell = %q, want /bin/sh", shell)
+	}
+}
+
+func TestDefaultUnixShell(t *testing.T) {
+	t.Setenv("SHELL", "/usr/local/bin/fish")
+	got := defaultUnixShell()
+	if got != "/usr/local/bin/fish" {
+		t.Errorf("defaultUnixShell() = %q, want /usr/local/bin/fish", got)
+	}
+
+	t.Setenv("SHELL", "")
+	got = defaultUnixShell()
+	if got != "/bin/sh" {
+		t.Errorf("defaultUnixShell() = %q, want /bin/sh", got)
+	}
+}
+
+func TestWindowsCmdExe_PrefersComSpecOverSystemRoot(t *testing.T) {
+	// When both are set, ComSpec should take priority
+	t.Setenv("ComSpec", `D:\Tools\cmd.exe`)
+	t.Setenv("SystemRoot", `C:\Windows`)
+	got := WindowsCmdExe()
+	if got != `D:\Tools\cmd.exe` {
+		t.Errorf("WindowsCmdExe() = %q, want %q (ComSpec should take priority)", got, `D:\Tools\cmd.exe`)
+	}
+}
+
+func TestDetectWindowsShell_FallbackUsesAbsolutePath(t *testing.T) {
+	// Simulate an environment where no PowerShell is found:
+	// set PATH to empty so LookPath won't find pwsh.exe or powershell.exe
+	t.Setenv("PATH", "")
+
+	t.Setenv("ComSpec", "")
+	t.Setenv("SystemRoot", `C:\Windows`)
+
+	shell, args := DetectWindowsShell()
+	want := `C:\Windows` + string(filepath.Separator) + filepath.Join("System32", "cmd.exe")
+	if shell != want {
+		t.Errorf("DetectWindowsShell() shell = %q, want %q", shell, want)
+	}
+	if len(args) != 1 || args[0] != "/C" {
+		t.Errorf("DetectWindowsShell() args = %v, want [/C]", args)
+	}
+}

pkg/tools/builtin/shell.go

@@ -10,14 +10,14 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"runtime"
 	"strings"
 	"sync"
 	"sync/atomic"
 	"time"
 
 	"github.com/docker/docker-agent/pkg/concurrent"
 	"github.com/docker/docker-agent/pkg/config"
+	"github.com/docker/docker-agent/pkg/shellpath"
 	"github.com/docker/docker-agent/pkg/tools"
 )
 
@@ -356,22 +356,10 @@ func NewShellTool(env []string, runConfig *config.RuntimeConfig) *ShellTool {
 }
 
 // detectShell returns the appropriate shell and arguments based on the platform.
+// It delegates to shellpath.DetectShell which uses absolute paths to prevent
+// PATH hijacking (CWE-426).
 func detectShell() (shell string, argsPrefix []string) {
-	if runtime.GOOS == "windows" {
-		return detectWindowsShell()
-	}
-
-	return cmp.Or(os.Getenv("SHELL"), "/bin/sh"), []string{"-c"}
-}
-
-func detectWindowsShell() (shell string, argsPrefix []string) {
-	powershellArgs := []string{"-NoProfile", "-NonInteractive", "-Command"}
-	for _, ps := range []string{"pwsh.exe", "powershell.exe"} {
-		if path, err := exec.LookPath(ps); err == nil {
-			return path, powershellArgs
-		}
-	}
-	return cmp.Or(os.Getenv("ComSpec"), "cmd.exe"), []string{"/C"}
+	return shellpath.DetectShell()
 }
 
 // resolveWorkDir returns the effective working directory.