Repository: woodruffw/zizmor (rust)

Issue:
[BUG]: `template-injection` false-positive
### Pre-submission checks

- [x] I am **not** filing a feature request. These should be filed via the feature request form instead.
- [x] I have looked through the [open issues](https://github.com/woodruffw/zizmor/issues?q=is%3Aissue+is%3Aopen+) for a duplicate report.

### Expected behavior

I expected zizmor to understand the common GHA ternary idiom of `${{ expr && '' || '' }}`.

### Actual behavior

Actually, it flagged this code: `${{ matrix.target == 'x86_64-linux-gnu' && '-Demit-man-pages' || '' }}`

### Reproduction steps

1. Add a staticlly-defined matrix to a job
2. Try to add a build flag dependent on the target

### Logs

```plain text
DEBUG zizmor::state: using cache directory: "/Users/dukese01/.cache/zizmor"
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::gitignore: opened gitignore file: /Users/dukese01/.config/git/ignore    
DEBUG collect_inputs:collect_from_dir{input_path="."}: globset: glob converted to regex: Glob { glob: "**/._*", re: "(?-u)^(?:/?|.*/)\\._[^/]*$", opts: GlobOptions { case_insensitive: false, literal_separator: true, backslash_escape: true, empty_alternates: false }, tokens: Tokens([RecursivePrefix, Literal('.'), Literal('_'), ZeroOrMore]) }    
DEBUG collect_inputs:collect_from_dir{input_path="."}: globset: built glob set; 5 literals, 31 basenames, 11 extensions, 0 prefixes, 0 suffixes, 2 required extensions, 1 regexes    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::gitignore: opened gitignore file: ./.gitignore    
DEBUG collect_inputs:collect_from_dir{input_path="."}: globset: built glob set; 0 literals, 4 basenames, 0 extensions, 0 prefixes, 0 suffixes, 0 required extensions, 0 regexes    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::gitignore: opened gitignore file: ./.git/info/exclude    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::walk: ignoring ./mise.local.toml: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("/Users/dukese01/.config/git/ignore"), original: "mise.local.toml", actual: "**/mise.local.toml", is_whitelist: false, is_only_dir: false })))    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::walk: ignoring ./.zig-cache: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("./.gitignore"), original: ".zig-cache", actual: "**/.zig-cache", is_whitelist: false, is_only_dir: false })))    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::walk: ignoring ./zig-out: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("./.gitignore"), original: "zig-out", actual: "**/zig-out", is_whitelist: false, is_only_dir: false })))    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::gitignore: opened gitignore file: ./.jj/.gitignore    
DEBUG collect_inputs:collect_from_dir{input_path="."}: globset: glob converted to regex: Glob { glob: "*", re: "(?-u)^[^/]*$", opts: GlobOptions { case_insensitive: false, literal_separator: true, backslash_escape: true, empty_alternates: false }, tokens: Tokens([ZeroOrMore]) }    
DEBUG collect_inputs:collect_from_dir{input_path="."}: globset: built glob set; 0 literals, 0 basenames, 0 extensions, 0 prefixes, 0 suffixes, 0 required extensions, 1 regexes    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::walk: ignoring ./.jj/reviews.json: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("./.jj/.gitignore"), original: "/*", actual: "*", is_whitelist: false, is_only_dir: false })))    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::walk: ignoring ./.jj/.gitignore: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("./.jj/.gitignore"), original: "/*", actual: "*", is_whitelist: false, is_only_dir: false })))    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::walk: ignoring ./.jj/working_copy: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("./.jj/.gitignore"), original: "/*", actual: "*", is_whitelist: false, is_only_dir: false })))    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::walk: ignoring ./.jj/repo: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("./.jj/.gitignore"), original: "/*", actual: "*", is_whitelist: false, is_only_dir: false })))    
DEBUG collect_inputs:collect_from_dir{input_path="."}: ignore::walk: ignoring ./.vscode: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("./.gitignore"), original: ".vscode/", actual: "**/.vscode", is_whitelist: false, is_only_dir: true })))    
DEBUG zizmor::config: no config discovered; loading default
DEBUG zizmor::config: loaded config: Config { rules: {} }
 INFO zizmor: skipping impostor-commit: can't run without a GitHub API token
 INFO zizmor: skipping ref-confusion: can't run without a GitHub API token
 INFO zizmor: skipping known-vulnerable-actions: can't run without a GitHub API token
 INFO audit: zizmor: 🌈 completed ./.github/workflows/ci.yaml
note[template-injection]: code injection via template expansion
  --> ./.github/workflows/ci.yaml:55:9
   |
55 |       - name: 🔧 Build
   |         -------------- note: this step
56 |         run: zig build -Dtarget=${{ matrix.target }} ${{ matrix.target == 'x86_64-linux-gnu' && '-Demit-man-pages' || '' }}
   |         ------------------------------------------------------------------------------------------------------------------- note: ${{ matrix.target == 'x86_64-linux-gnu' && '-Demit-man-pages' || '' }} may expand into attacker-controllable code
   |
   = note: audit confidence → Unknown

1 finding: 1 unknown, 0 informational, 0 low, 0 medium, 0 high
```

### Additional context

As I think about it, this workflow is better served with an `include` key on the matrix.

Pull request description:
feat: basic dataflow for context expansion
Fixes #624.

This will make audits like `template-injection` more precise.

Signed-off-by: William Woodruff <william@yossarian.net>

Patch:
diff --git a/docs/release-notes.md b/docs/release-notes.md
index 2c1cdf8..ae8e72a 100644
--- a/docs/release-notes.md
+++ b/docs/release-notes.md
@@ -13,6 +13,9 @@ of `zizmor`.
 
 * The SARIF output format now marks each rule as a "security" rule,
   which helps GitHub's presentation of the results (#631)
+* The [template-injection] audit is now performs dataflow analysis
+  to determine whether contexts actually expand in an unsafe manner,
+  making it significantly more accurate (#640)
 
 ### Bug Fixes 🐛
 
diff --git a/docs/snippets/trophies.md b/docs/snippets/trophies.md
index 4dda622..b1bf8bb 100644
--- a/docs/snippets/trophies.md
+++ b/docs/snippets/trophies.md
@@ -190,6 +190,14 @@
         - ethereum/hevm#615
 
 
+-   ![](https://github.com/fatiando.png?size=40){ width="40" loading=lazy align=left } fatiando
+
+    ---
+
+    ??? example "Examples"
+        - fatiando/harmonica#550
+
+
 -   ![](https://github.com/foyer-rs.png?size=40){ width="40" loading=lazy align=left } foyer-rs
 
     ---
@@ -464,6 +472,14 @@
         - mfussenegger/nvim-lint#710
 
 
+-   ![](https://github.com/microsoft.png?size=40){ width="40" loading=lazy align=left } microsoft
+
+    ---
+
+    ??? example "Examples"
+        - microsoft/linux-package-repositories#208
+
+
 -   ![](https://github.com/mkuf.png?size=40){ width="40" loading=lazy align=left } mkuf
 
     ---
@@ -881,6 +897,14 @@
         - simpeg/simpeg#1592
 
 
+-   ![](https://github.com/TecharoHQ.png?size=40){ width="40" loading=lazy align=left } TecharoHQ
+
+    ---
+
+    ??? example "Examples"
+        - TecharoHQ/anubis#188
+
+
 -   ![](https://github.com/termcolor.png?size=40){ width="40" loading=lazy align=left } termcolor
 
     ---
@@ -1003,6 +1027,14 @@
         - zcash/librustzcash#1679
 
 
+-   ![](https://github.com/ZcashFoundation.png?size=40){ width="40" loading=lazy align=left } ZcashFoundation
+
+    ---
+
+    ??? example "Examples"
+        - ZcashFoundation/zebra#9110
+
+
 -   ![](https://github.com/zkonduit.png?size=40){ width="40" loading=lazy align=left } zkonduit
 
     ---
diff --git a/docs/snippets/trophies.txt b/docs/snippets/trophies.txt
index 3e794f5..85fd1f3 100644
--- a/docs/snippets/trophies.txt
+++ b/docs/snippets/trophies.txt
@@ -34,6 +34,7 @@ earthobservations/wetterdienst#1440
 Electric-Coin-Company/zcash-light-client-ffi#186
 esl/MongooseIM#4435
 ethereum/hevm#615
+fatiando/harmonica#550
 foyer-rs/foyer#859
 fpgmaas/deptry#972
 freeCodeCamp/devdocs#2386
@@ -98,6 +99,7 @@ maxmind/mmdbwriter#104
 maxmind/GeoIP2-node#1387
 MeteorDevelopment/meteor-client#5091
 mfussenegger/nvim-lint#710
+microsoft/linux-package-repositories#208
 mkuf/prind#183
 mne-tools/mne-python#13011
 MoarVM/MoarVM#1875
@@ -172,6 +174,7 @@ sigstore/gitsign#602
 sigstore/model-transparency#369
 sigstore/sigstore-rs#424
 simpeg/simpeg#1592
+TecharoHQ/anubis#188
 termcolor/termcolor#89
 termux/termux-packages#22519
 theupdateframework/python-tuf#2798
@@ -189,5 +192,6 @@ vlang/v#22681
 wagtail/wagtail-localize#843
 wntrblm/nox#925
 zcash/librustzcash#1679
+ZcashFoundation/zebra#9110
 zkonduit/ezkl#907
 zkonduit/ezkl#906
diff --git a/src/audit/overprovisioned_secrets.rs b/src/audit/overprovisioned_secrets.rs
index a7dd9fe..b6a5f03 100644
--- a/src/audit/overprovisioned_secrets.rs
+++ b/src/audit/overprovisioned_secrets.rs
@@ -65,7 +65,7 @@ impl OverprovisionedSecrets {
                 // TODO: Consider any function call that accepts bare `secrets`
                 // to be a finding? Are there any other functions that users
                 // would plausibly call with the entire `secrets` object?
-                if func.eq_ignore_ascii_case("toJSON")
+                if func == "toJSON"
                     && args
                         .iter()
                         .any(|arg| matches!(arg, Expr::Context(ctx) if ctx == "secrets"))
diff --git a/src/audit/template_injection.rs b/src/audit/template_injection.rs
index c43ca49..713e6c9 100644
--- a/src/audit/template_injection.rs
+++ b/src/audit/template_injection.rs
@@ -187,7 +187,7 @@ impl TemplateInjection {
                 continue;
             }
 
-            for context in parsed.contexts() {
+            for context in parsed.dataflow_contexts() {
                 if context.child_of("secrets") {
                     // While not ideal, secret expansion is typically not exploitable.
                     continue;
diff --git a/src/audit/unredacted_secrets.rs b/src/audit/unredacted_secrets.rs
index dfcfeb4..307e57d 100644
--- a/src/audit/unredacted_secrets.rs
+++ b/src/audit/unredacted_secrets.rs
@@ -70,7 +70,7 @@ impl UnredactedSecrets {
 
         match expr {
             Expr::Call { func, args } => {
-                if func.eq_ignore_ascii_case("fromJSON")
+                if func == "fromJSON"
                     && args
                         .iter()
                         .any(|arg| matches!(arg, Expr::Context(ctx) if ctx.child_of("secrets")))
diff --git a/src/expr/mod.rs b/src/expr/mod.rs
index 1edb795..f0e7fa4 100644
--- a/src/expr/mod.rs
+++ b/src/expr/mod.rs
@@ -93,6 +93,20 @@ impl PartialEq<str> for Context<'_> {
     }
 }
 
+#[derive(Debug)]
+pub(crate) struct Function<'src>(pub(crate) &'src str);
+
+impl PartialEq for Function<'_> {
+    fn eq(&self, other: &Self) -> bool {
+        self.0.eq_ignore_ascii_case(other.0)
+    }
+}
+impl PartialEq<str> for Function<'_> {
+    fn eq(&self, other: &str) -> bool {
+        self.0.eq_ignore_ascii_case(other)
+    }
+}
+
 #[derive(Debug)]
 pub(crate) struct Identifier<'src>(&'src str);
 
@@ -140,7 +154,7 @@ pub(crate) enum Expr<'src> {
     Star,
     /// A function call.
     Call {
-        func: &'src str,
+        func: Function<'src>,
         args: Vec<Expr<'src>>,
     },
     /// A context identifier component, e.g. `github` in `github.actor`.
@@ -173,41 +187,48 @@ impl<'src> Expr<'src> {
         Self::Context(Context::new(r, components))
     }
 
-    /// Returns all of the contexts used in this expression, regardless
-    /// of dataflow.
+    /// Returns the contexts in this expression that directly flow into the
+    /// expression's evaluation.
     ///
-    /// **IMPORTANT**: This only returns "well-known" contexts, i.e. ones
-    /// that are defined at the top level. In other words, if an expression
-    /// contains both `foo.bar` and `foo().bar`, only the former will be
-    /// returned, as `foo()` can be anything and thus the `bar` access against
-    /// it has no particular meaning.
-    pub(crate) fn contexts(&self) -> Vec<&Context> {
+    /// For example `${{ foo.bar }}` returns `foo.bar` since the value
+    /// of `foo.bar` flows into the evaluation. On the other hand,
+    /// `${{ foo.bar == 'abc' }}` returns no expanded contexts,
+    /// since the value of `foo.bar` flows into a boolean evaluation
+    /// that gets expanded.
+    pub(crate) fn dataflow_contexts(&self) -> Vec<&Context> {
         let mut contexts = vec![];
 
         match self {
-            Expr::Call { func: _, args } => {
-                for arg in args {
-                    contexts.extend(arg.contexts());
+            Expr::Call { func, args } => {
+                // These functions, when evaluated, produce an evaluation
+                // that includes some or all of the contexts listed in
+                // their arguments.
+                if func == "toJSON" || func == "format" || func == "join" {
+                    for arg in args {
+                        contexts.extend(arg.dataflow_contexts());
+                    }
                 }
             }
-            Expr::Index(expr) => contexts.extend(expr.contexts()),
-            Expr::Context(ctx) => {
-                if matches!(ctx.components[0], Expr::Call { .. }) {
-                    // If the context looks something like `foo(args).a.b.c`, then
-                    // we need to check each of `args` for contexts but skip
-                    // the trailing identifiers, since those aren't well-known.
-                    contexts.extend(ctx.components[0].contexts());
-                } else {
-                    // Otherwise, if the context looks like a normal context,
-                    // we include it in its entirety.
-                    contexts.push(ctx)
+            // NOTE: We intentionally don't handle the `func(...).foo.bar`
+            // case differently here, since a call followed by a
+            // context access *can* flow into the evaluation.
+            // For example, `${{ fromJSON(something) }}` evaluates to
+            // `Object` but `${{ fromJSON(something).foo }}` evaluates
+            // to the contents of `something.foo`.
+            Expr::Context(ctx) => contexts.push(ctx),
+            Expr::BinOp { lhs, op, rhs } => match op {
+                // With && only the RHS can flow into the evaluation as a context
+                // (rather than a boolean).
+                BinOp::And => {
+                    contexts.extend(rhs.dataflow_contexts());
                 }
-            }
-            Expr::BinOp { lhs, op: _, rhs } => {
-                contexts.extend(lhs.contexts());
-                contexts.extend(rhs.contexts());
-            }
-            Expr::UnOp { op: _, expr } => contexts.extend(expr.contexts()),
+                // With || either the LHS or RHS can flow into the evaluation as a context.
+                BinOp::Or => {
+                    contexts.extend(lhs.dataflow_contexts());
+                    contexts.extend(rhs.dataflow_contexts());
+                }
+                _ => (),
+            },
             _ => (),
         }
 
@@ -351,7 +372,7 @@ impl<'src> Expr<'src> {
                         .collect::<Result<_, _>>()?;
 
                     Ok(Expr::Call {
-                        func: identifier.as_str(),
+                        func: Function(identifier.as_str()),
                         args,
                     }
                     .into())
@@ -391,11 +412,21 @@ mod tests {
     use pest::Parser as _;
     use pretty_assertions::assert_eq;
 
-    use super::{BinOp, Expr, ExprParser, Rule, UnOp};
+    use super::{BinOp, Context, Expr, ExprParser, Function, Rule, UnOp};
+
+    #[test]
+    fn test_function_eq() {
+        let func = Function("foo");
+        assert_eq!(&func, "foo");
+        assert_eq!(&func, "FOO");
+        assert_eq!(&func, "Foo");
+
+        assert_eq!(func, Function("FOO"));
+    }
 
     #[test]
     fn test_context_eq() {
-        let ctx = super::Context::try_from("foo.bar.baz").unwrap();
+        let ctx = Context::try_from("foo.bar.baz").unwrap();
         assert_eq!(&ctx, "foo.bar.baz");
         assert_eq!(&ctx, "FOO.BAR.BAZ");
         assert_eq!(&ctx, "Foo.Bar.Baz");
@@ -403,7 +434,7 @@ mod tests {
 
     #[test]
     fn test_context_child_of() {
-        let ctx = super::Context::try_from("foo.bar.baz").unwrap();
+        let ctx = Context::try_from("foo.bar.baz").unwrap();
 
         for (case, child) in &[
             // Trivial child cases.
@@ -432,7 +463,7 @@ mod tests {
 
     #[test]
     fn test_context_pop_if() {
-        let ctx = super::Context::try_from("foo.bar.baz").unwrap();
+        let ctx = Context::try_from("foo.bar.baz").unwrap();
 
         for (case, expected) in &[
             ("foo", Some("bar.baz")),
@@ -585,7 +616,7 @@ mod tests {
             (
                 "foo(1, 2, 3)",
                 Expr::Call {
-                    func: "foo",
+                    func: Function("foo"),
                     args: vec![Expr::Number(1.0), Expr::Number(2.0), Expr::Number(3.0)],
                 },
             ),
@@ -693,16 +724,41 @@ mod tests {
     }
 
     #[test]
-    fn test_expr_contexts() {
-        let expr = Expr::parse(
-            "foo.bar && abc && d.e.f && andThis(should.work).except.this && but().not.this",
-        )
-        .unwrap();
+    fn test_expr_dataflow_contexts() -> Result<()> {
+        // Trivial cases.
+        let expr = Expr::parse("foo.bar")?;
+        assert_eq!(expr.dataflow_contexts(), ["foo.bar"]);
+
+        let expr = Expr::parse("foo.bar[1]")?;
+        assert_eq!(expr.dataflow_contexts(), ["foo.bar[1]"]);
 
-        assert_eq!(expr.contexts(), ["foo.bar", "abc", "d.e.f", "should.work"]);
+        // No dataflow due to a boolean expression.
+        let expr = Expr::parse("foo.bar == 'bar'")?;
+        assert!(expr.dataflow_contexts().is_empty());
 
-        let expr = Expr::parse("fromJson(steps.runs.outputs.data).workflow_runs[0].id").unwrap();
+        // ||: all contexts potentially expand into the evaluation.
+        let expr = Expr::parse("foo.bar || abc || d.e.f")?;
+        assert_eq!(expr.dataflow_contexts(), ["foo.bar", "abc", "d.e.f"]);
 
-        assert_eq!(expr.contexts(), ["steps.runs.outputs.data"])
+        // &&: only the RHS context(s) expand into the evaluation.
+        let expr = Expr::parse("foo.bar && abc && d.e.f")?;
+        assert_eq!(expr.dataflow_contexts(), ["d.e.f"]);
+
+        let expr = Expr::parse("foo.bar == 'bar' && foo.bar || 'false'")?;
+        assert_eq!(expr.dataflow_contexts(), ["foo.bar"]);
+
+        let expr = Expr::parse("foo.bar == 'bar' && foo.bar || foo.baz")?;
+        assert_eq!(expr.dataflow_contexts(), ["foo.bar", "foo.baz"]);
+
+        let expr = Expr::parse("fromJson(steps.runs.outputs.data).workflow_runs[0].id")?;
+        assert_eq!(
+            expr.dataflow_contexts(),
+            ["fromJson(steps.runs.outputs.data).workflow_runs[0].id"]
+        );
+
+        let expr = Expr::parse("format('{0} {1} {2}', foo.bar, tojson(github), toJSON(github))")?;
+        assert_eq!(expr.dataflow_contexts(), ["foo.bar", "github", "github"]);
+
+        Ok(())
     }
 }


Test patch:
diff --git a/tests/integration/snapshots/integration__snapshot__template_injection-6.snap b/tests/integration/snapshots/integration__snapshot__template_injection-6.snap
index aae2609..37e2620 100644
--- a/tests/integration/snapshots/integration__snapshot__template_injection-6.snap
+++ b/tests/integration/snapshots/integration__snapshot__template_injection-6.snap
@@ -1,7 +1,6 @@
 ---
 source: tests/integration/snapshot.rs
-expression: "zizmor().input(workflow_under_test(\"template-injection/issue-339-repro.yml\")).run()?"
-snapshot_kind: text
+expression: "zizmor().input(input_under_test(\"template-injection/issue-339-repro.yml\")).run()?"
 ---
 info[template-injection]: code injection via template expansion
   --> @@INPUT@@:27:9
@@ -11,7 +10,7 @@ info[template-injection]: code injection via template expansion
 28 |           id: run-id
 29 | /         run: |
 30 | |           echo "run-id=${{ fromJson(steps.runs.outputs.data).workflow_runs[0].id }}" >> "$GITHUB_OUTPUT"
-   | |_________________________________________________________________________________________________________- info: steps.runs.outputs.data may expand into attacker-controllable code
+   | |_________________________________________________________________________________________________________- info: fromJson(steps.runs.outputs.data).workflow_runs[0].id may expand into attacker-controllable code
    |
    = note: audit confidence → Low
 
diff --git a/tests/integration/test-data/template-injection/dataflow.yml b/tests/integration/test-data/template-injection/dataflow.yml
new file mode 100644
index 0000000..b93bd93
--- /dev/null
+++ b/tests/integration/test-data/template-injection/dataflow.yml
@@ -0,0 +1,29 @@
+name: template-injection-dataflow
+
+on: pull_request
+
+permissions: {}
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+
+    steps:
+      # OK: dangerous context but without dataflow
+      - name: ok
+        run: |
+          echo "issue is foo: ${{ github.event.pull_request.title == 'foo' }}"
+          echo "issue contains foo: ${{ contains(github.event.pull_request.title, 'foo') }}"
+
+      # NOT OK: function call propagates dataflow
+      - name: notok
+        run: |
+          echo "${{ toJSON(github.event.pull_request.title) }}"
+          echo "${{ format('{0}', github.event.pull_request.title) }}"
+          echo "${{ join(github.event.pull_request.labels.*.name) }}"
+
+      # NOT OK: control flow propagates dataflow
+      - name: notok-2
+        run: |
+          echo "${{ github.event.pull_request.title || github.event.issue.title }}"
+          echo "${{ false || github.event.issue.title }}"