feat(unpinned-images): detect docker/podman commands in run: steps#1677
feat(unpinned-images): detect docker/podman commands in run: steps#1677jfagoagas wants to merge 9 commits intozizmorcore:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Extends the unpinned-images audit to detect unpinned Docker/Podman image references inside run: steps by parsing shell scripts with tree-sitter, complementing the existing container: and services: image checks.
Changes:
- Add tree-sitter bash-based detection of
docker/podmanpull|run|createinvocations inrun:steps (and composite action steps) and emit findings based on pinning policy. - Add new integration test workflow fixture and snapshot-based tests for the new detection behavior.
- Update audit documentation to describe the new
run:-step coverage and remediation guidance.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| docs/audits.md | Documents the new run:-step Docker/Podman detection and adds an additional remediation example. |
| crates/zizmor/src/audit/unpinned_images.rs | Implements tree-sitter-based parsing of run: scripts to extract Docker/Podman image references and audit them. |
| crates/zizmor/tests/integration/test-data/unpinned-images/run-docker.yml | Adds workflow test data covering docker/podman commands in run: steps (including multiline scripts and flags). |
| crates/zizmor/tests/integration/audit/unpinned_images.rs | Adds snapshot tests (regular + pedantic personas) for the new fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
woodruffw
left a comment
There was a problem hiding this comment.
Thanks for opening a PR @jfagoagas.
I'm interested in this feature, but I'm concerned about this approach's stability/brittleness; I've left some comments to that effect.
Separately, this PR is probably too big to review -- anything above roughly ~500 lines takes me much longer to read and understand, so it'd be ideal to break this up into several smaller PRs (each with tests, where possible). If you're interested in taking that on, we can discuss what an MVP/tractable design would look like here.
| /// Tree-sitter query matching `docker` and `podman` command invocations in bash. | ||
| /// | ||
| /// NOTE: `@span` is required by [`utils::SpannedQuery`] and can be used in the | ||
| /// future to produce more precise finding locations within multiline scripts. | ||
| const BASH_DOCKER_CMD_QUERY: &str = r#" | ||
| (command | ||
| name: (command_name) @cmd | ||
| argument: (_)* @args | ||
| (#match? @cmd "^(docker|podman)$") | ||
| ) @span | ||
| "#; | ||
|
|
||
| /// Boolean short flags for docker/podman pull/run/create that do NOT consume | ||
| /// the next argument. | ||
| /// | ||
| /// NOTE: `-a` is intentionally excluded — it means `--all-tags` for `pull` | ||
| /// (boolean) but `--attach` for `run`/`create` (value-consuming). Treating it | ||
| /// as value-consuming is the safer default to avoid false negatives. | ||
| const BOOLEAN_SHORT_FLAGS: &[&str] = &["-d", "-i", "-t", "-P", "-q"]; | ||
|
|
||
| /// Boolean long flags for docker/podman pull/run/create that do NOT consume | ||
| /// the next argument. | ||
| /// | ||
| /// This is the union of boolean flags across all three subcommands for both | ||
| /// Docker and Podman. Unknown flags default to value-consuming, which is the | ||
| /// safe choice for a security tool (may miss findings, but won't produce false | ||
| /// positives). | ||
| const BOOLEAN_LONG_FLAGS: &[&str] = &[ | ||
| // docker pull/run/create | ||
| "--all-tags", | ||
| "--detach", | ||
| "--disable-content-trust", | ||
| "--init", | ||
| "--interactive", | ||
| "--no-healthcheck", | ||
| "--oom-kill-disable", | ||
| "--privileged", | ||
| "--publish-all", | ||
| "--quiet", | ||
| "--read-only", | ||
| "--rm", | ||
| "--sig-proxy", | ||
| "--tty", | ||
| "--use-api-socket", | ||
| // podman-only booleans (pull) | ||
| "--tls-verify", | ||
| // podman-only booleans (run/create) | ||
| "--env-host", | ||
| "--http-proxy", | ||
| "--no-hostname", | ||
| "--no-hosts", | ||
| "--passwd", | ||
| "--read-only-tmpfs", | ||
| "--replace", | ||
| "--rmi", | ||
| "--rootfs", | ||
| ]; |
There was a problem hiding this comment.
I don't think this approach is sustainable long-term, unfortunately -- we either need a lighter-weight way to detect docker images/invocations in run: blocks, or something that's more formal and testable (e.g. explicitly modeling these tools using clap).
(As is, this is going to be brittle on any CLI changes to either docker or podman, which in practice is going to mean a lot of false positive reports.)
There was a problem hiding this comment.
I think both CLIs are stable enough but we can explore other options to make this simpler.
| let text = node.utf8_text(source).ok()?; | ||
| text.strip_prefix('\'')?.strip_suffix('\'') | ||
| } | ||
| _ => node.utf8_text(source).ok(), |
There was a problem hiding this comment.
This fallback seems risky -- I think you want to match "word" explicitly here and then have unknown node types here be _ => None?
| "string" => { | ||
| // For double-quoted strings, extract the string_content child | ||
| // to avoid the surrounding quotes. | ||
| if node.named_child_count() == 1 { |
There was a problem hiding this comment.
I think this should explicitly match the string_content node, rather than assuming that any child of string is string_content -- the latter is probably not guaranteed to be true.
For example, "$(abc)" is string > command_substitution instead of string > string_content.
| if let Some(image_ref) = Self::extract_docker_image(&args) { | ||
| // Skip arguments containing shell variable expansions or | ||
| // GitHub Actions expressions — we can't statically resolve those. | ||
| if !image_ref.contains('$') { |
There was a problem hiding this comment.
Noting: this might not be solvable in a clean way (because of actions expressions), but ideally we'd do this filtering via the CST instead (since $ could be a valid part of an image identifier, in principle).
There was a problem hiding this comment.
I need to look into this in depth. Rust is not one of my preferred/known languages.
| /// | ||
| /// `args` contains all arguments after the command name (docker/podman). | ||
| /// Handles global flags before the subcommand (e.g. `docker --context foo run alpine`). | ||
| fn extract_docker_image<'a>(args: &[&'a str]) -> Option<&'a str> { |
There was a problem hiding this comment.
Per the comment above, I think this is unfortunately going to be too brittle. If the goal is to parse Docker CLI arguments, we should use clap or another fully-equipped parser that can track positionals/consumption states for us.
There was a problem hiding this comment.
(This is going to be nontrivial, however, since Docker and Podman have large CLIs.)
There was a problem hiding this comment.
I wasn't aware of clap but definitely something to take a look to track this. We can make this simpler covering some scenarios.
| /// | ||
| /// NOTE: `@span` is required by [`utils::SpannedQuery`] and can be used in the | ||
| /// future to produce more precise finding locations within multiline scripts. | ||
| const BASH_DOCKER_CMD_QUERY: &str = r#" |
There was a problem hiding this comment.
What about pwsh, etc.? We generally try to support those too since they're common enough (unfortunately) in GitHub Actions.
(Those don't need to be part of an MVP here, but curious if you have thoughts on them.)
There was a problem hiding this comment.
I thought about this but I didn't want to add more to this PR. We can add this as part of the plan.
| }); | ||
|
|
||
| for image_str in self.docker_images_in_run(run, shell)? { | ||
| let location = step.location().primary().with_keys(["run".into()]); |
There was a problem hiding this comment.
This probably needs to use a subfeature to get the exact command/argument span, since run: blocks can be very large.
Thank you for maintaining such a great tool 🙌
I'll take a look and reply inline.
I thought this would happen when I was creating it. I saw this change requires a refactor in the audit, so please let me know what's your plan to split this up! We can create a chain of PRs where each one points another and can be merged independently. |
Thank you for the kind words! Yeah, my preference here would be to come up with a sequence of PRs that add this, each no more than a few hundred lines. Ideally we'd start with just Docker, and do:
I think a sequence roughly like that would make this tractable for me to review. |
Sounds like a plan. I'll start with the first one once I get the chance to, most likely during the weekend. I'll keep this PR as a reference if you don't mind until the feature is implemented. |
|
Yeah, no worries. Thanks for contributing! |
|
@woodruffw before jumping into writing code I'd like to validate the following plan with you: Plan: Split unpinned-images-run-block into reviewable PR chain PR chainPR 1: Docker CLI models with clap (~350-400 lines, from main)
PR 2: Audit integration (~300-400 lines, net negative from deletions)
PR 3: Podman CLI model (~200-250 lines)
PR 4: Documentation (final)
Open questions: PR review workflowTwo options for how to structure the chain: Option A — Sequential merge into main: Each PR targets main. PR #2 waits for PR #1 to merge, PR #3 waits for PR #2. You review and merge one at a time. Option B — Stacked PRs: PR 1 targets main, PR 2 targets PR 1's branch, PR 3 targets PR 2's branch. You can review the full chain at once, then merge bottom-up. Which do you prefer? |
|
Thanks @jfagoagas, that plan sounds reasonable to me. I prefer to do sequential merges for this, as long as that works for you! (I have some small nitpicks on each, but they're minor and can be addressed on-the-fly.) |
Pre-submission checks
Please check these boxes:
Mandatory: This PR corresponds to an issue (if not, please create
one first).
I hereby disclose the use of an LLM or other AI coding assistant in the
creation of this PR. PRs will not be rejected for using AI tools, but
will be rejected for undisclosed use.
If a checkbox is not applicable, you can leave it unchecked.
Motivation
GitHub's agentic workflows are increasingly generating
run:step scripts that includedocker pull/docker runcommands. Unlike human-authored workflows where a developer might remember to pin images, agent-generated scripts almost never pin to a digest. This turns every unpinneddocker pullinto a supply chain injection point — an attacker who hijacks a tag on a public registry gets code execution inside the workflow.The existing
unpinned-imagesaudit only covered declarativecontainer:andservices:blocks, missing the imperativedockercommands that are becoming more common as CI pipelines get more dynamic.This also opens the door for future audits targeting other agentic workflow patterns: unvalidated tool outputs piped into shell commands, dynamic action references constructed from agent suggestions, or secrets passed to agent-spawned containers.
Summary
Closes #738.
Extends the
unpinned-imagesaudit to detect unpinned image references indocker pull,docker run,docker create, and theirpodmanequivalents withinrun:steps, in addition to the existingcontainer:andservices:checks.github-envaudit pattern) to identify docker/podman command invocations, avoiding false positives from strings and comments (e.g.echo "docker pull ubuntu")-dit), and--flag=valuesyntax to correctly extract the image argument$IMAGE) that can't be statically resolvedaudit_stepfor workflow steps andaudit_composite_stepfor action stepsTest Plan
cargo buildcompilescargo test -p zizmor -- unpinned_images— existing snapshot tests unchanged, new tests passcargo insta review— accept new snapshots fortest_run_docker_pedanticandtest_run_docker_regularcargo clippy— no new warnings