Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions pkg/cli/runner_guard.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,21 @@ func runRunnerGuardOnDirectory(workflowDir string, verbose bool, strict bool) er
if workflowDir != "" {
relDir, relErr := filepath.Rel(gitRoot, workflowDir)
if relErr == nil && relDir != ".." && !strings.HasPrefix(relDir, ".."+string(filepath.Separator)) {
scanPath = relDir
scanPath = filepath.Clean(relDir)
}
}

// Prefix with "./" and convert host separators to forward slashes for the Linux container.
// This prevents option injection: without the prefix a workflowDir such as "--help" would
// produce a scanPath beginning with "-", which runner-guard could interpret as a flag.
containerScanPath := "./" + filepath.ToSlash(scanPath)

// Build the Docker command
// docker run --rm -v "$gitRoot:/workdir" -w /workdir ghcr.io/vigilant-llc/runner-guard:latest scan <path> --format json
// #nosec G204 -- gitRoot comes from git rev-parse (trusted source) and is validated as absolute path.
// exec.Command with separate args (not shell execution) prevents command injection.
// #nosec G204 -- gitRoot is validated as an absolute path above (from git rev-parse, a trusted
// source). containerScanPath is derived from filepath.Rel(gitRoot, workflowDir), cleaned with
// filepath.Clean, validated to not escape the repository root (no ".." prefix), and prefixed
// with "./" to prevent option injection. exec.Command passes args directly to the OS (no shell).
cmd := exec.Command(
"docker",
"run",
Expand All @@ -73,7 +80,7 @@ func runRunnerGuardOnDirectory(workflowDir string, verbose bool, strict bool) er
"-w", "/workdir",
RunnerGuardImage,
"scan",
scanPath,
containerScanPath,
"--format", "json",
)

Expand All @@ -83,7 +90,7 @@ func runRunnerGuardOnDirectory(workflowDir string, verbose bool, strict bool) er
// In verbose mode, also show the command that users can run directly
if verbose {
dockerCmd := fmt.Sprintf("docker run --rm -v \"%s:/workdir\" -w /workdir %s scan %s --format json",
gitRoot, RunnerGuardImage, scanPath)
gitRoot, RunnerGuardImage, containerScanPath)
fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage("Run runner-guard directly: "+dockerCmd))
}

Expand Down
11 changes: 11 additions & 0 deletions pkg/cli/upgrade_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,17 @@ func relaunchWithSameArgs(extraFlag string, exeOverride string) error {
newArgs := append(append([]string(nil), os.Args[1:]...), extraFlag)
upgradeLog.Printf("Re-launching with new binary: %s %v", exe, newArgs)

// Validate that exe is an absolute path before executing it (defense-in-depth).
// exe is always derived from os.Executable() or the pre-rename installPath which is
// itself obtained from os.Executable() + filepath.EvalSymlinks (all trusted OS calls).
if !filepath.IsAbs(exe) {
return fmt.Errorf("executable path is not absolute: %s", exe)
}

// #nosec G204 -- exe is validated as an absolute path above and originates from
// os.Executable() or the known install path. newArgs forwards os.Args[1:] (user-controlled)
// plus a hardcoded flag; exec.Command passes arguments directly to execve(2) without
// invoking a shell, so shell-injection (CWE-78) is not possible regardless of argv content.
cmd := exec.Command(exe, newArgs...)
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
Expand Down
3 changes: 3 additions & 0 deletions pkg/gitutil/gitutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,9 @@ func ReadFileFromHEAD(filePath, gitRoot string) (string, error) {

gitutilLog.Printf("Reading %q from git HEAD (relative path: %s)", filePath, relPath)

// #nosec G204 -- relPath is derived from filepath.Rel(gitRoot, absPath), validated to not start
// with ".." (path-traversal check above), and is not user-controlled shell input.
// exec.Command uses argv directly (no shell), so no shell injection is possible.
cmd := exec.Command("git", "-C", gitRoot, "show", "HEAD:"+relPath)
output, err := cmd.Output()
if err != nil {
Expand Down
7 changes: 7 additions & 0 deletions pkg/workflow/dependabot.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,11 +331,18 @@ func (c *Compiler) generatePackageLock(workflowDir string) error {
return errors.New("npm command not found - cannot generate package-lock.json. Install Node.js/npm to enable this feature")
}

// Validate that exec.LookPath returned an absolute path (defense-in-depth).
if !filepath.IsAbs(npmPath) {
return fmt.Errorf("npm path is not absolute: %s", npmPath)
}

if c.verbose {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Running npm install --package-lock-only..."))
}

// Run npm install --package-lock-only
// #nosec G204 -- npmPath is resolved by exec.LookPath and validated as an absolute path above;
// the fixed arguments "install" and "--package-lock-only" contain no user-controlled data.
cmd := exec.Command(npmPath, "install", "--package-lock-only")
cmd.Dir = workflowDir

Expand Down
2 changes: 2 additions & 0 deletions pkg/workflow/pip_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ func (c *Compiler) validatePythonPackagesWithPip(packages []string, packageType

// Use pip index to check if package exists on PyPI
// Include --pre flag to check for pre-release versions (alpha, beta, rc)
// #nosec G204 -- pipCmd is one of the hardcoded values "pip" or "pip3"; pkgName is
// validated above by validatePipPackageName against the strict PyPI PEP 508 allowlist.
cmd := exec.Command(pipCmd, "index", "versions", pkgName, "--pre")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] pipCmd is used as a bare command name ("pip" / "pip3"), relying on PATH resolution at exec time — unlike dependabot.go, which calls exec.LookPath and validates filepath.IsAbs before the #nosec. The #nosec rationale here says the values are "hardcoded" but omits that they are resolved via PATH, making the defense-in-depth story inconsistent across files.

💡 Suggested fix

Apply the same pattern used in dependabot.go: resolve with exec.LookPath, validate with filepath.IsAbs, then pass the resolved absolute path to exec.Command.

@copilot please address this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pipCmd resolved via PATH without exec.LookPath, inconsistent with the npmPath pattern added in this same PR: a tampered PATH could cause an attacker-controlled binary to be executed.

💡 Suggested fix

Mirror the dependabot.go treatment from this PR:

resolvedPip, err := exec.LookPath(pipCmd)
if err != nil {
    return fmt.Errorf("pip command %q not found: %w", pipCmd, err)
}
if !filepath.IsAbs(resolvedPip) {
    return fmt.Errorf("pip path is not absolute: %s", resolvedPip)
}
cmd := exec.Command(resolvedPip, "index", "versions", pkgName, "--pre")

The nosec comment correctly notes pipCmd is a hardcoded string, but the resolved binary is still PATH-dependent. The defense-in-depth absolute-path validation applied to npmPath in dependabot.go in this same PR should be applied here consistently to satisfy the scanner and reduce risk from a compromised environment.

output, err := cmd.CombinedOutput()

Expand Down
Loading