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
18 changes: 9 additions & 9 deletions pkg/cli/docker_args_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,44 +63,44 @@ func validateHostMountPath(hostPath string) (string, error) {

func validateDockerImageRef(imageRef string) (string, error) {
if imageRef == "" {
return "", errors.New("grant image reference cannot be empty. Example: ghcr.io/example/image:tag")
return "", errors.New("docker image reference cannot be empty. Example: ghcr.io/example/image:tag")
}
// Image refs disallow all Unicode whitespace, while containsControlCharacters also rejects
// non-whitespace spoofing characters such as bidi overrides and other format controls.
if containsControlCharacters(imageRef) || strings.IndexFunc(imageRef, unicode.IsSpace) >= 0 {
dockerArgsValidationLog.Printf("rejected grant image reference with invalid whitespace/control characters: %q", imageRef)
return "", fmt.Errorf("grant image reference contains invalid whitespace/control characters. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
dockerArgsValidationLog.Printf("rejected docker image reference with invalid whitespace/control characters: %q", imageRef)
return "", fmt.Errorf("docker image reference contains invalid whitespace/control characters. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
}
if strings.HasPrefix(imageRef, "-") {
return "", fmt.Errorf("grant image reference cannot start with '-'. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
return "", fmt.Errorf("docker image reference cannot start with '-'. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
}

imageRefWithoutDigest := imageRef
if strings.Count(imageRef, "@") > 1 {
return "", fmt.Errorf("grant image reference has multiple digest separators. Example: ghcr.io/example/image@sha256:<digest>. Got: %q", imageRef)
return "", fmt.Errorf("docker image reference has multiple digest separators. Example: ghcr.io/example/image@sha256:<digest>. Got: %q", imageRef)
}
nameWithOptionalTag, digest, hasDigest := strings.Cut(imageRef, "@")
if hasDigest {
if digest == "" || !isAllowedDockerImageDigest(digest) {
return "", fmt.Errorf("grant image reference has an invalid digest format. Example: ghcr.io/example/image@sha256:<digest>. Got: %q", imageRef)
return "", fmt.Errorf("docker image reference has an invalid digest format. Example: ghcr.io/example/image@sha256:<digest>. Got: %q", imageRef)
}
imageRefWithoutDigest = nameWithOptionalTag
}
if imageRefWithoutDigest == "" {
return "", fmt.Errorf("grant image reference is missing an image name. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
return "", fmt.Errorf("docker image reference is missing an image name. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
}

imageName := imageRefWithoutDigest
if colon := strings.LastIndex(imageRefWithoutDigest, ":"); colon > strings.LastIndex(imageRefWithoutDigest, "/") {
tag := imageRefWithoutDigest[colon+1:]
if !dockerImageTagPattern.MatchString(tag) {
return "", fmt.Errorf("grant image reference has an invalid tag format. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
return "", fmt.Errorf("docker image reference has an invalid tag format. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
}
imageName = imageRefWithoutDigest[:colon]
}

if imageName == "" || strings.HasSuffix(imageName, "/") || !dockerImageNamePattern.MatchString(imageName) {
return "", fmt.Errorf("grant image reference must match an allow-listed image pattern. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
return "", fmt.Errorf("docker image reference must match an allow-listed image pattern. Example: ghcr.io/example/image:tag. Got: %q", imageRef)
}
return imageRef, nil
}
Expand Down
26 changes: 20 additions & 6 deletions pkg/cli/grype.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"sync"

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/fileutil"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/workflow"
)
Expand Down Expand Up @@ -224,20 +225,33 @@ func grypeRunOnImage(imageRef string, verbose bool) (*grypeOutput, error) {

grypeLog.Printf("Scanning %s with grype", imageRef)

// #nosec G204 -- imageRef is extracted from the gh-aw-manifest in compiled lock files,
// which are produced by this tool from trusted markdown sources. exec.Command passes
// args directly to the OS without shell interpretation, preventing command injection.
// Validate the image reference before it reaches docker: lock-file manifests can carry
// attacker-influenced content, and an image reference starting with "-" (or containing
// control characters) would otherwise be interpreted as a docker/grype option.
validatedImageRef, err := validateDockerImageRef(imageRef)
if err != nil {
return nil, err
}

dockerPath, err := fileutil.ResolveExecutablePath("docker")
if err != nil {
return nil, fmt.Errorf("docker command not found: %w", err)
}

// #nosec G204 -- dockerPath is resolved from the fixed executable name "docker" and
// validatedImageRef is allow-list validated above. exec.Command passes args directly to
// the OS without shell interpretation, preventing command injection.
cmd := exec.Command(
"docker",
dockerPath,
"run",
"--rm",
GrypeImage,
imageRef,
validatedImageRef,
"-o", "json",
)

if verbose {
dockerCmd := fmt.Sprintf("docker run --rm %s %s -o json", GrypeImage, imageRef)
dockerCmd := shellJoinArgs([]string{"docker", "run", "--rm", GrypeImage, validatedImageRef, "-o", "json"})
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Run grype directly: "+dockerCmd))
}

Expand Down
46 changes: 46 additions & 0 deletions pkg/cli/grype_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ package cli
import (
"errors"
"os"
"path/filepath"
"strings"
"testing"
)

Expand Down Expand Up @@ -292,3 +294,47 @@ func makeGrypeFinding(id, severity, pkgName, pkgVersion string, fixVersions []st
f.Artifact.Version = pkgVersion
return f
}

func TestGrypeRunOnImage_RejectsUnsafeImageRef(t *testing.T) {
tests := []struct {
name string
imageRef string
}{
{"option flag", "--entrypoint=/bin/sh"},
{"embedded newline", "alpine:latest\n--privileged"},
{"semicolon", "ghcr.io/org/im;age:latest"},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
_, err := grypeRunOnImage(tt.imageRef, false)
if err == nil {
t.Fatalf("Expected error for unsafe image reference %q", tt.imageRef)
}
if !strings.Contains(err.Error(), "docker image reference") {
t.Errorf("Expected image reference validation error, got: %v", err)

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.

[/tdd] Missing positive-path test: there's no assertion that a well-formed image ref (e.g. ghcr.io/anchore/grype:v0.80.0) is not rejected. Without it, an over-strict regex change could silently break valid scans.

💡 Suggested addition
func TestGrypeRunOnImage_AcceptsValidImageRef(t *testing.T) {
    // Should reach docker (not fail validation); we expect a docker-not-found
    // or network error, NOT a validation error.
    _, err := grypeRunOnImage("ghcr.io/anchore/grype:v0.80.0", false)
    if err != nil && strings.Contains(err.Error(), "docker image reference") {
        t.Fatalf("Valid image ref was rejected: %v", err)
    }
}

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 0ad7a0c47c: added valid image-ref coverage using a fake docker executable, so the test verifies validation allows well-formed refs without contacting real Docker or the network.

}
})
}
}

func TestGrypeRunOnImage_AcceptsValidImageRef(t *testing.T) {
prependFakeDockerToPath(t, `{"matches":[]}`)

_, err := grypeRunOnImage("ghcr.io/anchore/grype:v0.80.0", false)
if err != nil {
t.Fatalf("Expected valid image reference to reach docker, got: %v", err)
}
}

func prependFakeDockerToPath(t *testing.T, stdout string) {
t.Helper()

binDir := t.TempDir()
dockerPath := filepath.Join(binDir, "docker")
script := "#!/bin/sh\nprintf '%s' '" + stdout + "'\n"
if err := os.WriteFile(dockerPath, []byte(script), 0o755); err != nil {
t.Fatalf("Failed to write fake docker executable: %v", err)
}
t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH"))
}
25 changes: 20 additions & 5 deletions pkg/cli/syft.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/fileutil"
"github.com/github/gh-aw/pkg/logger"
)

Expand Down Expand Up @@ -103,20 +104,34 @@ func runSyftOnLockFiles(lockFiles []string, verbose bool, strict bool) error {
func runSyftOnImage(ctx context.Context, imageRef, sbomDir string, verbose bool) (*SyftScanResult, error) {
syftLog.Printf("Scanning %s with syft", imageRef)

// #nosec G204 -- imageRef comes from compiled lock-file manifests and is passed
// as a direct process argument (no shell interpolation).
// Validate the image reference before it reaches docker: lock-file manifests can carry
// attacker-influenced content, and an image reference starting with "-" (or containing
// control characters) would otherwise be interpreted as a docker/syft option.
validatedImageRef, err := validateDockerImageRef(imageRef)
if err != nil {
return nil, err
}

dockerPath, err := fileutil.ResolveExecutablePath("docker")
if err != nil {
return nil, fmt.Errorf("docker command not found: %w", err)
}

// #nosec G204 -- dockerPath is resolved from the fixed executable name "docker" and
// validatedImageRef is allow-list validated above. exec.CommandContext passes args
// directly to the OS without shell interpretation, preventing command injection.
cmd := exec.CommandContext(
ctx,
"docker",
dockerPath,
"run",
"--rm",
SyftImage,
imageRef,
validatedImageRef,
"-o", "syft-json",
)

if verbose {
dockerCmd := fmt.Sprintf("docker run --rm %s %s -o syft-json", SyftImage, imageRef)
dockerCmd := shellJoinArgs([]string{"docker", "run", "--rm", SyftImage, validatedImageRef, "-o", "syft-json"})
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Run syft directly: "+dockerCmd))
}

Expand Down
36 changes: 36 additions & 0 deletions pkg/cli/syft_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"encoding/json"
"os"
"path/filepath"
"strings"
"testing"

"github.com/github/gh-aw/pkg/constants"
Expand Down Expand Up @@ -198,3 +199,38 @@ jobs:
t.Errorf("Expected no error in non-strict mode, got: %v", err)
}
}

func TestRunSyftOnImage_RejectsUnsafeImageRef(t *testing.T) {
tests := []struct {
name string
imageRef string
}{
{"option flag", "--entrypoint=/bin/sh"},
{"embedded newline", "alpine:latest\n--privileged"},
{"semicolon", "ghcr.io/org/im;age:latest"},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
_, err := runSyftOnImage(context.Background(), tt.imageRef, t.TempDir(), false)
if err == nil {
t.Fatalf("Expected error for unsafe image reference %q", tt.imageRef)
}
if !strings.Contains(err.Error(), "docker image reference") {
t.Errorf("Expected image reference validation error, got: %v", err)
}
})
}
}

func TestRunSyftOnImage_AcceptsValidImageRef(t *testing.T) {
prependFakeDockerToPath(t, `{"artifacts":[]}`)

result, err := runSyftOnImage(context.Background(), "ghcr.io/anchore/syft:v1.0.0", t.TempDir(), false)
if err != nil {
t.Fatalf("Expected valid image reference to reach docker, got: %v", err)
}
if result.ImageRef != "ghcr.io/anchore/syft:v1.0.0" {
t.Fatalf("Expected result image ref to match input, got %q", result.ImageRef)
}
}
Loading