Harden Docker scanner exec paths against argv/path injection#56333
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #56333 does not have the 'implementation' label and has only 25 new lines of code in business logic directories (threshold: 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The hardening intent is fine, but one of the new argument checks now rejects a valid Docker mount form that the code itself generates, so this can break the zizmor scanner on Windows-style repository paths.
Blocking themes
pkg/cli/zizmor.go: the newvalidateExecArgument(volumeMount)call rejectsC:/repo:/workdirbecausevalidateExecArgumentbans any argument starting with-, and the generated mount string can legally contain a drive-letter colon form that does not need this extra check.- The added executable-path revalidation is redundant rather than harmful, but the mount-argument validation needs to be narrowed so it does not reject already-validated Docker syntax.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 6.02 AIC · ⌖ 7.82 AIC · ⊞ 4.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two issues.
📋 Key Themes & Highlights
Issues Found
-
Doc-contract mismatch (
grype.go:269,zizmor.go:192):validateExecArgumentis called on constant-derived values (grypeImageRef,zizmorImageRef), directly contradicting the function's own doc comment which says the function must not be applied to hardcoded literals. Either the calls should be removed or the doc comment must be updated to document the defence-in-depth intent. -
Missing test coverage (
zizmor.go:185): the newvalidateExecArgument(volumeMount)guard has no corresponding test that exercises the rejection path. Without it the guard is invisible to the test suite.
Positive Highlights
- ✅ The
validateExecArgumentfunction itself is well-tested (TestValidateExecArgument) with good coverage of the key injection vectors (flag injection, null bytes, control characters). - ✅ Re-validating the resolved Docker path via
ValidateExecutablePathimmediately before command construction is a solid defence-in-depth improvement. - ✅ Per-file
containerPathiteration inzizmor.gocloses a real injection surface that was previously unguarded. - ✅ Error messages now include the rejected argument value, which meaningfully speeds up triage.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 37.8 AIC · ⌖ 10.1 AIC · ⊞ 7.6K
Comment /matt to run again
| if err != nil { | ||
| return nil, fmt.Errorf("invalid grype scanner image reference %q: %w", GrypeImage, err) | ||
| } | ||
| if err := validateExecArgument(grypeImageRef); err != nil { |
There was a problem hiding this comment.
[/codebase-design] validateExecArgument is applied to a constant-derived value here, contradicting the function's own doc comment (lines 316–319) that says it must not be applied to hardcoded literals.
grypeImageRef is the result of validateDockerImageRef(GrypeImage) where GrypeImage is a compile-time constant. The value is already validated by validateDockerImageRef; wrapping it in validateExecArgument conflates two different validation layers and muddies the intended contract for future readers.
💡 Suggestion
Either remove this call, or update the validateExecArgument doc comment to explicitly state that constant-derived values are also acceptable as defence-in-depth. Leaving the code contradicting its comment makes the rule ambiguous.
@copilot please address this.
| if err != nil { | ||
| return nil, nil, nil, fmt.Errorf("zizmor scanner image reference %q is invalid; expected a registry reference. Example: ghcr.io/owner/image:tag: %w", ZizmorImage, err) | ||
| } | ||
| if err := validateExecArgument(zizmorImageRef); err != nil { |
There was a problem hiding this comment.
[/codebase-design] Same constant-derived value concern as in grype.go — zizmorImageRef comes from validateDockerImageRef(ZizmorImage) where ZizmorImage is a package-level constant. The extra validateExecArgument call is inconsistent with the stated contract of the function.
💡 Suggestion
Align with the grype.go fix: either remove this call or update the doc comment to document the defence-in-depth intent for constant-origin values.
@copilot please address this.
| if err != nil { | ||
| return nil, nil, nil, fmt.Errorf("docker mount path for git root %q is invalid; expected an absolute host path. Example: /home/user/repo: %w", gitRoot, err) | ||
| } | ||
| if err := validateExecArgument(volumeMount); err != nil { |
There was a problem hiding this comment.
[/tdd] The validateExecArgument(volumeMount) call on the volume mount is new, but there is no test for the case where buildDockerVolumeMount returns a value that starts with - or contains control characters. The existing TestBuildZizmorCommand* tests don't exercise this rejection path.
💡 Suggested test skeleton
func TestBuildZizmorCommand_RejectsMalformedVolumeMount(t *testing.T) {
// Arrange: provide a gitRoot whose resolved path would produce a mount
// string starting with '-' or containing a control character.
// Act: call buildZizmorCommand with that root.
// Assert: err != nil and error message contains "invalid docker volume mount".
}Without this test the new guard is invisible to the test suite and could be silently deleted.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Hardens Grype and Zizmor Docker command construction through point-of-use argument and executable-path validation.
Changes:
- Validates scanner images, mounts, config paths, and scan paths.
- Revalidates resolved Docker executable paths.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/grype.go |
Adds validation before Grype Docker execution. |
pkg/cli/zizmor.go |
Adds validation before Zizmor Docker execution. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| dockerPath, err = fileutil.ValidateExecutablePath(dockerPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("resolved docker executable path is invalid: %w", err) |
| dockerPath, err = fileutil.ValidateExecutablePath(dockerPath) | ||
| if err != nil { | ||
| return nil, nil, nil, fmt.Errorf("resolved docker executable path is invalid: %w", err) |
There was a problem hiding this comment.
The PR correctly adds validateExecArgument guards on variable-origin Docker argv values (image refs, volume mounts, container paths) and ValidateExecutablePath on the resolved Docker binary path. The approach is sound.
One non-blocking correctness issue: Both grypeRunOnImage and buildZizmorCommand call fileutil.ValidateExecutablePath on the value already returned by fileutil.ResolveExecutablePath. Since ResolveExecutablePath calls ValidateExecutablePath internally as its last step (see pkg/fileutil/executable.go:71), these second calls are redundant — they repeat the symlink resolution and stat check on an already-validated path. This won't cause a bug but adds dead code and may mislead future readers about the contract of ResolveExecutablePath. See inline comments for the specific lines.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 34.4 AIC · ⌖ 9.24 AIC · ⊞ 6.2K
| if err != nil { | ||
| return nil, fmt.Errorf("docker command not found: %w", err) | ||
| } | ||
| dockerPath, err = fileutil.ValidateExecutablePath(dockerPath) |
There was a problem hiding this comment.
Redundant ValidateExecutablePath call — fileutil.ResolveExecutablePath already calls ValidateExecutablePath internally before returning (see pkg/fileutil/executable.go line 71). Calling it again on the already-validated result is dead code and may confuse future readers into thinking a plain ResolveExecutablePath result is unvalidated.
Suggested fix: remove the second ValidateExecutablePath call and rely on the guarantee provided by ResolveExecutablePath.
dockerPath, err := fileutil.ResolveExecutablePath("docker")
if err != nil {
return nil, fmt.Errorf("docker command not found: %w", err)
}
// ResolveExecutablePath already validates — no second call needed.@copilot please address this.
| if err != nil { | ||
| return nil, nil, nil, fmt.Errorf("docker command not found: %w", err) | ||
| } | ||
| dockerPath, err = fileutil.ValidateExecutablePath(dockerPath) |
There was a problem hiding this comment.
Same redundant ValidateExecutablePath call — fileutil.ResolveExecutablePath (line 173) already calls ValidateExecutablePath internally. Adding it again on the returned path performs duplicate filesystem stat/symlink work and misleads readers.
Remove lines 177–180 to keep the code consistent with ResolveExecutablePath's documented contract.
@copilot please address this.
|
@copilot run pr finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed the PR-finisher pass. Addressed the actionable scanner-validation regressions in 11b30c0 and completed local validation; CI must be re-triggered by a maintainer for the new HEAD. |
Sighthound flagged critical command-injection risks in scanner command construction, especially around dynamic docker arguments and executable path trust. This change tightens the two Docker-based scanner callsites by adding explicit argument/path validation at point-of-use.
Scope: grype and zizmor scanner execution paths
validateExecArgument(...)checks for dynamic values beforeexec.Command(...).Executable path hardening
fileutil.ValidateExecutablePath(...)immediately before command construction.Error surface improvements