Harden scanner/relaunch exec paths against command and argument injection

docs/adr/49063-harden-docker-argument-validation-against-injection.md

@@ -0,0 +1,52 @@
+# ADR-49063: Harden Docker Argument Validation Against Command Injection
+
+**Date**: 2026-07-30
+**Status**: Accepted
+**Deciders**: pelikhan, Copilot
+
+---
+
+### Context
+
+Sighthound's static security scan flagged 33 Critical CWE-78 (OS Command Injection) findings in the `pkg/cli` package. Five high-severity findings were in the Docker exec paths of `runner_guard.go`, `grant.go`, `poutine.go`, and the self-relaunch path of `upgrade_command.go`. In these paths, Docker volume mount strings were assembled by direct string concatenation (for example, `gitRoot + ":/workdir"`), and some callers still relied on `exec.Command("docker", ...)` PATH resolution at launch time. While `exec.Command` avoids shell interpretation, unsanitized mount arguments can still be re-parsed by Docker's colon-delimited `-v` syntax, and copyable verbose commands can become unsafe when user-controlled values are interpolated without shell quoting. The fix needed to be applied consistently across multiple scanners with minimal duplication.
+
+### Decision
+
+We will centralize Docker `-v` argument construction into shared helpers (`buildDockerVolumeMount`, `buildDockerReadonlyFileMount`) in `pkg/cli/docker_args_validation.go` that enforce absolute-path validation, container-path normalization, rejection of control characters and colon-delimiter injection, and file-type checks. On the host side, callers will reject mount paths containing unsupported `:` characters while preserving Windows drive-letter paths; on the container side, callers will reject any colon because Docker would reinterpret it as an extra mount segment. All Docker exec sites in `grant.go`, `poutine.go`, and `runner_guard.go` will use these helpers instead of string concatenation, and every scanner will resolve the Docker executable through `fileutil.ResolveExecutablePath("docker")` before launch. The self-relaunch path in `upgrade_command.go` will reject any argument containing a NUL byte before passing it to `exec.Command`. Verbose "run this directly" hints for the Grant and Poutine scanners will be rendered with shell-escaped arguments so they remain copyable without reintroducing injection risk in documentation output.
+
+### Alternatives Considered
+
+#### Alternative 1: Per-function inline validation (status quo + extensions)
+
+Each scanner function continues to perform its own validation inline, as `runner_guard.go` already did for absolute-path checks. Validation logic would be duplicated across `grant.go`, `poutine.go`, and `runner_guard.go`. Not chosen because it produces inconsistent coverage (each site may miss different edge cases), makes auditing harder, and does not consolidate error handling in a single testable unit.
+
+#### Alternative 2: Switch all scanners to Docker `--mount` syntax
+
+Replace every `-v host:container[:ro]` mount with `--mount type=bind,src=...,dst=...[,readonly]` so colons in host paths do not need special-case handling. This was rejected for this fix because the existing scanners already share `-v`-style call sites and test expectations, and the smallest safe patch was to harden those exact paths by rejecting ambiguous colon-delimited inputs. A broader `--mount` migration remains possible later if we want to permit more host-path shapes.
+
+#### Alternative 3: Shell-escape / shlex sanitization before concatenation
+
+Apply a shlex-style escaping function to volume mount components before string concatenation, similar to how some container runtimes sanitize arguments. Not chosen because `exec.Command` already avoids shell interpretation — the risk is not shell metacharacters but rather structural injection into Docker's `-v` argument parsing and NUL bytes in argument vectors. Escaping addresses the wrong threat model and masks invalid input rather than rejecting it.
+
+### Consequences
+
+#### Positive
+- Eliminates the CWE-78 Critical findings in Docker exec paths by ensuring all volume mount strings are constructed from validated, normalized, absolute paths.
+- Centralizes validation logic into a single package-internal file, making future audits and policy changes a one-location edit.
+- Prevents ambiguous Docker `-v` parsing by rejecting colon-bearing container paths and unsupported host-path colons before launch, instead of relying on Docker's own mount parser to fail safely.
+- Aligns Poutine with Grant and Runner Guard by resolving the Docker executable path explicitly before `exec.Command`, closing the remaining PATH-hijack gap in this hardening pass.
+- Adds explicit NUL byte rejection in the self-relaunch path, preventing argument injection in process re-exec scenarios.
+- Adds unit tests for the shared helpers, colon/control-character rejection paths, verbose command quoting, and the image reference / NUL byte validations, increasing coverage of security-critical paths.
+
+#### Negative
+- Each `buildDockerReadonlyFileMount` call now performs an `os.Stat` syscall to verify the host file is a regular file; this adds a small overhead on every Docker scanner invocation.
+- The new helpers return errors that all callers must propagate, increasing the error-path surface in functions that previously could not fail at mount-construction time.
+- Callers must now satisfy stricter pre-conditions (normalized absolute paths and unambiguous `-v` mount components); any future refactor that produces relative paths or colon-bearing mount targets will fail at runtime rather than silently passing an invalid mount string.
+
+#### Neutral
+- The `pkg/cli` package gains a new internal dependency on `pkg/fileutil` (already a transitive dependency via other files in the package).
+- `grant.go` and `poutine.go` now resolve the `docker` binary via `fileutil.ResolveExecutablePath` rather than relying on the ambient PATH; this is a behavioral change that could surface errors on systems where `docker` is not on PATH.
+
+---
+
+*Accepted after implementation and conformance validation.*

pkg/cli/audit_test.go

@@ -527,6 +527,9 @@ func TestAuditCachingBehavior(t *testing.T) {
 	if _, err := os.Stat(summaryPath); os.IsNotExist(err) {
 		t.Fatalf("Run summary file should exist after saveRunSummary")
 	}
+	if err := markArtifactDownloaded(runOutputDir, string(ArtifactSetAll)); err != nil {
+		t.Fatalf("markArtifactDownloaded: %v", err)
+	}
 
 	// Load the summary back
 	loadedSummary, ok := loadRunSummary(runOutputDir, false)

pkg/cli/docker_args_validation.go

@@ -0,0 +1,80 @@
+package cli
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"path"
+	"strings"
+
+	"github.com/github/gh-aw/pkg/fileutil"
+)
+
+func validateContainerMountPath(containerPath string) (string, error) {
+	if containerPath == "" {
+		return "", errors.New("container path cannot be empty. Example: /workdir")
+	}
+	if strings.ContainsAny(containerPath, "\x00\r\n:") {
+		return "", errors.New("container path contains invalid control characters or reserved characters. Example: /workdir")
+	}
+	if !path.IsAbs(containerPath) {
+		return "", fmt.Errorf("container path must be absolute. Example: /workdir. Got: %s", containerPath)
+	}
+	cleanPath := path.Clean(containerPath)
+	if cleanPath != containerPath {
+		return "", fmt.Errorf("container path must be normalized. Example: /workdir/config. Got: %s", containerPath)
+	}
+	return cleanPath, nil
+}
+
+func validateHostMountPath(hostPath string) (string, error) {
+	cleanHostPath, err := fileutil.ValidateAbsolutePath(hostPath)
+	if err != nil {
+		return "", fmt.Errorf("invalid host path %q: %w", hostPath, err)
+	}
+	if strings.Contains(cleanHostPath[2:], ":") || (!isWindowsDrivePath(cleanHostPath) && strings.Contains(cleanHostPath, ":")) {
+		return "", fmt.Errorf("host path contains unsupported ':' for docker -v mount syntax. Example: /tmp/repo or C:/repo. Got: %s", cleanHostPath)
+	}
+	return cleanHostPath, nil
+}
+
+func isWindowsDrivePath(hostPath string) bool {
+	if len(hostPath) < 3 {
+		return false
+	}
+	driveLetter := hostPath[0]
+	return ((driveLetter >= 'a' && driveLetter <= 'z') || (driveLetter >= 'A' && driveLetter <= 'Z')) &&
+		hostPath[1] == ':' &&
+		(hostPath[2] == '\\' || hostPath[2] == '/')
+}
+
+func buildDockerVolumeMount(hostPath, containerPath string) (string, error) {
+	cleanHostPath, err := validateHostMountPath(hostPath)
+	if err != nil {
+		return "", err
+	}
+	cleanContainerPath, err := validateContainerMountPath(containerPath)
+	if err != nil {
+		return "", err
+	}
+	return cleanHostPath + ":" + cleanContainerPath, nil
+}
+
+func buildDockerReadonlyFileMount(hostFile, containerPath string) (string, error) {
+	cleanHostFile, err := validateHostMountPath(hostFile)
+	if err != nil {
+		return "", fmt.Errorf("invalid host file %q: %w", hostFile, err)
+	}
+	info, err := os.Stat(cleanHostFile)
+	if err != nil {
+		return "", fmt.Errorf("failed to stat host file %q: %w", cleanHostFile, err)
+	}
+	if !info.Mode().IsRegular() {
+		return "", fmt.Errorf("host file is not a regular file: %s", cleanHostFile)
+	}
+	cleanContainerPath, err := validateContainerMountPath(containerPath)
+	if err != nil {
+		return "", err
+	}
+	return cleanHostFile + ":" + cleanContainerPath + ":ro", nil
+}

pkg/cli/docker_args_validation_test.go

@@ -0,0 +1,64 @@
+//go:build !integration
+
+package cli
+
+import (
+	"os"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestBuildDockerVolumeMount(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	mount, err := buildDockerVolumeMount(tmpDir, "/workdir")
+	require.NoError(t, err)
+	require.Equal(t, tmpDir+":/workdir", mount)
+
+	_, err = buildDockerVolumeMount("relative/path", "/workdir")
+	require.Error(t, err)
+	require.ErrorContains(t, err, "invalid host path")
+
+	_, err = buildDockerVolumeMount(tmpDir, "workdir")
+	require.Error(t, err)
+	require.ErrorContains(t, err, "container path must be absolute")
+
+	_, err = buildDockerVolumeMount(tmpDir, "/work:dir")
+	require.Error(t, err)
+	require.ErrorContains(t, err, "reserved characters")
+
+	_, err = buildDockerVolumeMount(tmpDir, "/work\x00dir")
+	require.Error(t, err)
+	require.ErrorContains(t, err, "invalid control characters")
+
+	if runtime.GOOS != "windows" {
+		_, err = buildDockerVolumeMount("/tmp/repo:fixture", "/workdir")
+		require.Error(t, err)
+		require.ErrorContains(t, err, "unsupported ':'")
+	}
+}
+
+func TestBuildDockerReadonlyFileMount(t *testing.T) {
+	tmpDir := t.TempDir()
+	policyFile := filepath.Join(tmpDir, ".grant.yaml")
+	require.NoError(t, os.WriteFile(policyFile, []byte("policy: true\n"), 0o644))
+
+	mount, err := buildDockerReadonlyFileMount(policyFile, "/tmp/policy.yaml")
+	require.NoError(t, err)
+	require.Equal(t, policyFile+":/tmp/policy.yaml:ro", mount)
+
+	_, err = buildDockerReadonlyFileMount(tmpDir, "/tmp/policy.yaml")
+	require.Error(t, err)
+	require.ErrorContains(t, err, "not a regular file")
+
+	_, err = buildDockerReadonlyFileMount(policyFile, "/tmp/policy:yaml")
+	require.Error(t, err)
+	require.ErrorContains(t, err, "reserved characters")
+
+	_, err = buildDockerReadonlyFileMount(policyFile, "/tmp/policy\x00yaml")
+	require.Error(t, err)
+	require.ErrorContains(t, err, "invalid control characters")
+}

pkg/cli/grant.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 
 	"github.com/github/gh-aw/pkg/console"
+	"github.com/github/gh-aw/pkg/fileutil"
 	"github.com/github/gh-aw/pkg/gitutil"
 	"github.com/github/gh-aw/pkg/logger"
 )
@@ -154,13 +155,31 @@ func grantPolicyFile() (string, error) {
 func grantRunOnImage(imageRef, policyFile string, verbose bool) (*grantOutput, error) {
 	containerPolicyPath := grantContainerPolicyPath
 
+	imageRef = strings.TrimSpace(imageRef)
+	if imageRef == "" {
+		return nil, errors.New("grant image reference cannot be empty. Example: ghcr.io/example/image:tag")
+	}
+	if strings.ContainsAny(imageRef, " \t\r\n\x00") {
+		return nil, fmt.Errorf("grant image reference contains invalid whitespace/control characters. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
+	}
+
+	dockerPath, err := fileutil.ResolveExecutablePath("docker")
+	if err != nil {
+		return nil, fmt.Errorf("docker command not found: %w", err)
+	}
+
+	volumeMount, err := buildDockerReadonlyFileMount(policyFile, containerPolicyPath)
+	if err != nil {
+		return nil, fmt.Errorf("invalid grant policy mount: %w", err)
+	}
+
 	// #nosec G204 -- imageRef and policyFile are derived from compiled lock files and the
 	// current repository checkout. exec.Command passes arguments directly without a shell.
 	cmd := exec.Command(
-		"docker",
+		dockerPath,
 		"run",
 		"--rm",
-		"-v", policyFile+":"+containerPolicyPath+":ro",
+		"-v", volumeMount,
 		GrantImage,
 		"--config", containerPolicyPath,
 		"--output", "json",
@@ -169,8 +188,17 @@ func grantRunOnImage(imageRef, policyFile string, verbose bool) (*grantOutput, e
 	)
 
 	if verbose {
-		dockerCmd := fmt.Sprintf("docker run --rm -v %s:%s:ro %s --config %s --output json check %s",
-			policyFile, containerPolicyPath, GrantImage, containerPolicyPath, imageRef)
+		dockerCmd := shellJoinArgs([]string{
+			"docker",
+			"run",
+			"--rm",
+			"-v", volumeMount,
+			GrantImage,
+			"--config", containerPolicyPath,
+			"--output", "json",
+			"check",
+			imageRef,
+		})
 		fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Run grant directly: "+dockerCmd))
 	}
 

pkg/cli/grant_test.go

@@ -3,8 +3,12 @@
 package cli
 
 import (
+	"os"
 	"path/filepath"
+	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestGrantDisplayFindings_NilOutput(t *testing.T) {
@@ -73,3 +77,52 @@ func TestGrantPolicyFile(t *testing.T) {
 		t.Fatalf("Expected policy file basename %q, got %q", grantPolicyFilename, filepath.Base(policyFile))
 	}
 }
+
+func TestGrantRunOnImageRejectsInvalidImageRef(t *testing.T) {
+	tmpDir := t.TempDir()
+	policyFile := filepath.Join(tmpDir, "policy.yaml")
+	require.NoError(t, os.WriteFile(policyFile, []byte("policy: true\n"), 0o644))
+
+	testCases := []struct {
+		name     string
+		imageRef string
+		want     string
+	}{
+		{name: "whitespace", imageRef: "bad image", want: "invalid whitespace/control characters"},
+		{name: "empty", imageRef: "", want: "cannot be empty"},
+		{name: "null byte", imageRef: "img\x00ref", want: "invalid whitespace/control characters"},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := grantRunOnImage(tt.imageRef, policyFile, false)
+			require.Error(t, err)
+			require.ErrorContains(t, err, tt.want)
+		})
+	}
+}
+
+func TestGrantRunOnImageVerboseCommandEscapesImageRef(t *testing.T) {
+	tmpDir := t.TempDir()
+	policyFile := filepath.Join(tmpDir, "policy file.yaml")
+	require.NoError(t, os.WriteFile(policyFile, []byte("policy: true\n"), 0o644))
+
+	volumeMount, err := buildDockerReadonlyFileMount(policyFile, grantContainerPolicyPath)
+	require.NoError(t, err)
+
+	command := shellJoinArgs([]string{
+		"docker",
+		"run",
+		"--rm",
+		"-v", volumeMount,
+		GrantImage,
+		"--config", grantContainerPolicyPath,
+		"--output", "json",
+		"check",
+		"alpine;id",
+	})
+
+	if !strings.Contains(command, "'alpine;id'") {
+		t.Fatalf("Expected image ref to be shell-escaped in verbose command, got: %s", command)
+	}
+}