Cross-workflow taint analysis audit - #4
Merged
Conversation
New check category `cross-workflow` that catches data flows across workflow
files -- something zizmor does not currently audit. Four scenarios:
* artifact-flow: PR-triggered uploader feeds a privileged
`workflow_run` downloader (tj-actions / Ultralytics pattern)
* unguarded-workflow_run: `workflow_run` trigger without a
`workflows:` allowlist or `types: [completed]` guard
* workflow_run-event-taint: privileged `workflow_run` workflow
reads `github.event.workflow_run.*` fields that are attacker-
controlled when the upstream was PR-triggered
Checks build a small graph from the already-parsed workflow docs,
so analysis lives alongside the existing per-file audits in
audit::run() and rides the existing scanner -> launcher IPC without
protocol changes.
Threaded through CheckConfig, PartialCheckConfig, KNOWN_CHECK_NAMES,
drift detection, set_all_checks_deny, has_any_enabled_check,
check_name_for_finding, and the policy parser. Governed by the
`cross-workflow` policy knob and forced deny under --paranoid.
Tests: 5 unit + 4 integration, all green; 320 total tests pass;
clippy clean; hasp self-scan produces no cross-workflow findings.
Follow-up to 401efa9 fixing ten issues identified in the initial audit. Adds an on-disk fixture matrix (tests/fixtures/cross_workflow/) and a structure-generating fuzz harness (fuzz_invariants_hold_across_random_pairs, fuzz_idempotent_add_safe_unrelated_workflow) that explore the shape space and assert structural invariants. CRIT-1 types: [completed] was treated as a conclusion-success gate, but "completed" is the event-timing filter - it fires on success AND failure AND cancelled AND skipped. A privileged sink with types: [completed] and no if: check deployed every conclusion, attacker included. Replaced the heuristic with yaml_contains_conclusion_gate + expr_is_conclusion_gate that walk real if: expressions at top-level / job / step for github.event.workflow_run.conclusion == 'success' (preferred) or != 'failure' (tolerated), with either quote style and any whitespace. HIGH-1 yaml_references_workflow_run_event fired on any github.event.workflow_run.* read - including .conclusion, which is GitHub-set and is the exact mitigation the artifact-flow CRIT's detail text recommends. Introduced DANGEROUS_WORKFLOW_RUN_FIELDS allowlist (head_branch, head_sha, head_repository, head_commit, display_title, pull_requests, event, triggering_actor, actor, referenced_workflows). .conclusion, .status, .id, .run_number, .workflow_id, .created_at etc. no longer trigger event-taint. HIGH-2 Detection was a hard-coded match on actions/upload-artifact and actions/download-artifact, missing dawidd6/action-download-artifact - the primary third-party lever in the tj-actions / Ultralytics family of attacks. Expanded artifact_op_from_uses to recognise common forks explicitly plus any path ending in /download-artifact or /action-download-artifact. artifact_name_from_with now also reads pattern: (v4). HIGH-3 A run: step running gh run download $RUN_ID bypassed the check entirely - only uses: was inspected. Added run_step_implies_artifact_download which matches gh run download / gh artifact download and synthesises a wildcard-name ArtifactOp, making the shell-level flow visible. MED-1 Missing permissions: is still treated as the paranoid maximum (write-all) - GitHub's default-for-new-repos is contents: read, but org settings can flip to permissive and users can opt in per repo, so a paranoid scanner should assume the worst. Extracted PermissionSummary::paranoid_default() and documented the assumption in a block comment + finding detail. TODO: .hasp.yml knob cross_workflow.assume_default_token_write: false for users with an org-wide restricted default. MED-2 on.workflow_run.workflows: entries like .github/workflows/ci.yml did not resolve; the sink fell back to every repo workflow with any untrusted trigger - over-broad. Added candidate_parent_ids + index_forms_for_self: each workflow is indexed under name: / filename / stem / path-without-ext, and parent references are tried in those forms in order of specificity. LOW-1 Graph<'a> + PhantomData<&'a ()> was always used as Graph<'static>. Dropped the lifetime parameter and marker. LOW-2 parse_permissions_value returned default (no writes) for any string shorthand other than "write-all" - including typos and future GitHub extensions. Explicit read-all / read / none / "" now return no-writes; any other string returns paranoid default. LOW-3 check_unguarded_workflow_run emitted two HIGHs when both the workflows: allowlist and the conclusion gate were missing. Merged into a single compound finding; title still contains the substrings check_name_for_finding expects. PERF-1 check_artifact_flow was O(W^2 * D * U). Added UploadIndex (by_name: HashMap<String, Vec<(w, op)>> + wildcard: Vec<..>) built once per run. Each sink download now does one hash lookup plus a scan over wildcard uploads (typically empty) - O(W*D) with a small constant. Also filters source_idx == sink_idx so a workflow downloading its own upload isn't a cross-workflow shape. Tests ----- * 8 on-disk fixtures under tests/fixtures/cross_workflow/ covering the previously-failing shapes. * matrix_regression_guards iterates PASSING_CASES; the empty KNOWN_BUGS table is kept as scaffolding for future regressions. * fuzz_invariants_hold_across_random_pairs generates random (source, sink) pairs from typed spec enums and checks determinism, title allowlist, file attribution, severity ordering, and that sink-only findings are never attributed to the source file. * fuzz_idempotent_add_safe_unrelated_workflow is a metamorphic property: adding an unrelated push-only workflow must not change findings on the original pair - catches parent-resolution leakage. * HASP_FUZZ_SEED=<hex|dec> reproduces a failure; HASP_FUZZ_ITERS=<n> stresses harder. Verified clean at 10 000 iters on the default seed and at 2 000 iters across six additional seeds. Full suite: 328 passed, 6 ignored. hasp self-scan produces no cross-workflow findings. Clippy clean on the PR's files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The pre_exec SIGSTOP hook deadlocked Command::spawn: Rust waits for the child to exec (signaled by CLOEXEC pipe close), but SIGSTOP suspends the child before exec, so the pipe never closes and the parent blocks indefinitely. Latent since the initial commit; only triggered once the sudo fallback was widened to Proxy/Verifier modes (commit 6d3b1d5), which made CI start engaging the cgroup sandbox path. Fix: SIGSTOP from the parent after spawn returns, then move into cgroup, then SIGCONT. Brief pre-cgroup window where the child runs unsandboxed, but proxy/verifier do only env reads + Landlock/seccomp setup before any network call. Also drop a few clippy 1.95 nits (sort_by_key, from_mins, ?-operator).
- checkout-taint detector: scan with.{ref,repository,path,token}; CHECKOUT_ACTIONS allowlist
- artifact_op_from_uses/step_uses_tainted_checkout: lowercase action head (case-insensitive match)
- conclusion gate: only == 'success'; pattern_negated counts trailing ! for !!/!!!
- expr_references_workflow_run_field: word-boundary check
- check_artifact_flow: dispatch-only sources downgrade Critical → High
- check_workflow_run_event_taint: skip when checkout-taint already fires
- Graph::by_identity: HashMap<String, Vec<usize>> so case-collisions don't shadow
- index_forms_for_self: .yml↔.yaml swap only at filename level
- candidate_parent_ids: lowercase before path-prefix strip
- triggers: PullRequestReview, ReviewComment, Discussion, DiscussionComment, RepositoryDispatch
- run_step_implies_artifact_download: gh api .../artifacts/, curl api.github.com
- malformed permissions → paranoid_default
- detail string: remove '!= failure' from recommended gate
netguard: HASP_AWAIT_SANDBOX self-stop protocol
- StopProtocol::{SelfStop, ParentStop}; step runner ParentStop, forward proxy SelfStop
- main.rs: parent_exe_matches_self via /proc/<ppid>/exe vs /proc/self/exe — ignore env var from untrusted parent
- wait_for_stop: EINTR retry loop
- stop_and_migrate failure: SIGCONT before kill for clean state
- data/scrubbed_env_vars.txt: add HASP_AWAIT_SANDBOX
- tests/netguard.rs: integration test for untrusted-parent refuse-to-stop
CI: drop HASP_TRACE + --nocapture/--test-threads=1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New
cross-workflowaudit category that runs during--paranoid.What it detects:
workflow_runworkflows (CRIT) — the tj-actions / Ultralytics exploit shape.workflow_runtriggers without an explicitworkflows:allowlist ortypes: [completed]guard in a privileged workflow (HIGH).workflow_runworkflows that readgithub.event.workflow_run.*fields that are attacker-controlled when the upstream was PR-triggered (HIGH).Files added / modified:
src/audit/cross_workflow.rs— audit implementation (~480 LoC)src/audit/mod.rs— dispatcher wiringsrc/policy/mod.rs+src/policy/parse.rs—cross-workflowpolicy knobtests/cross_workflow_taint.rs— integration coverageTests: 5 unit + 4 integration. Full suite passes; clippy clean.