Merge pull request #25 from github/support_trigger_events

New `On` and `Event` classes
2026-01-07 19:50:22 +01:00 · 2024-05-13 09:18:53 +02:00
parent a30c2aa5de 510cefecbe
commit b2d7c823b3
12 changed files with 246 additions and 49 deletions
--- a/ql/lib/codeql/actions/Ast.qll
+++ b/ql/lib/codeql/actions/Ast.qll
@@ -254,13 +254,13 @@ class Workflow extends AstNode instanceof WorkflowImpl {

  Job getJob(string jobId) { result = super.getJob(jobId) }

-  predicate hasTriggerEvent(string trigger) { super.hasTriggerEvent(trigger) }
-
-  string getATriggerEvent() { result = super.getATriggerEvent() }
+  Event getATriggerEvent() { result = super.getATriggerEvent() }

  Permissions getPermissions() { result = super.getPermissions() }

  Strategy getStrategy() { result = super.getStrategy() }
+
+  On getOn() { result = super.getOn() }
 }

 class ReusableWorkflow extends Workflow instanceof ReusableWorkflowImpl {
@@ -305,6 +305,20 @@ class Needs extends AstNode instanceof NeedsImpl {
  Job getANeededJob() { result = super.getANeededJob() }
 }

+class On extends AstNode instanceof OnImpl {
+  Event getAnEvent() { result = super.getAnEvent() }
+}
+
+class Event extends AstNode instanceof EventImpl {
+  string getName() { result = super.getName() }
+
+  string getAnActivityType() { result = super.getAnActivityType() }
+
+  string getAPropertyValue(string prop) { result = super.getAPropertyValue(prop) }
+
+  predicate hasProperty(string prop) { super.hasProperty(prop) }
+}
+
 /**
 * An Actions job within a workflow.
 * See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobs.
@@ -328,9 +342,7 @@ abstract class Job extends AstNode instanceof JobImpl {

  Permissions getPermissions() { result = super.getPermissions() }

-  predicate hasTriggerEvent(string trigger) { super.hasTriggerEvent(trigger) }
-
-  string getATriggerEvent() { result = super.getATriggerEvent() }
+  Event getATriggerEvent() { result = super.getATriggerEvent() }

  Strategy getStrategy() { result = super.getStrategy() }

--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -68,6 +68,16 @@ private newtype TAstNode =
  TStrategyNode(YamlMapping n) { exists(YamlMapping m | m.lookup("strategy") = n) } or
  TNeedsNode(YamlMappingLikeNode n) { exists(YamlMapping m | m.lookup("needs") = n) } or
  TJobNode(YamlMapping n) { exists(YamlMapping w | w.lookup("jobs").(YamlMapping).lookup(_) = n) } or
+  TOnNode(YamlMappingLikeNode n) { exists(YamlMapping w | w.lookup("on") = n) } or
+  TEventNode(YamlScalar event, YamlMappingLikeNode n) {
+    exists(OnImpl o |
+      o.getNode().(YamlMapping).maps(event, n)
+      or
+      o.getNode().(YamlSequence).getAChildNode() = event and event = n
+      or
+      o.getNode().(YamlScalar) = n and event = n
+    )
+  } or
  TStepNode(YamlMapping n) {
    exists(YamlMapping m | m.lookup("steps").(YamlSequence).getElementNode(_) = n)
  } or
@@ -308,6 +318,9 @@ class WorkflowImpl extends AstNodeImpl, TWorkflowNode {

  override YamlMapping getNode() { result = n }

+  /** Gets the `on` trigger events for this workflow. */
+  OnImpl getOn() { result.getNode() = n.lookup("on") }
+
  /** Gets the 'global' `env` mapping in this workflow. */
  EnvImpl getEnv() { result.getNode() = n.lookup("env") }

@@ -323,15 +336,8 @@ class WorkflowImpl extends AstNodeImpl, TWorkflowNode {
  /** Gets the permissions granted to this workflow. */
  PermissionsImpl getPermissions() { result.getNode() = n.lookup("permissions") }

-  /** Workflow is triggered by given trigger event */
-  predicate hasTriggerEvent(string trigger) {
-    exists(YamlNode y | y = n.lookup("on").(YamlMappingLikeNode).getNode(trigger))
-  }
-
  /** Gets the trigger event that starts this workflow. */
-  string getATriggerEvent() {
-    exists(YamlNode y | y = n.lookup("on").(YamlMappingLikeNode).getNode(result))
-  }
+  EventImpl getATriggerEvent() { this.getOn().getAnEvent() = result }

  /** Gets the strategy for this workflow. */
  StrategyImpl getStrategy() { result.getNode() = n.lookup("strategy") }
@@ -573,6 +579,66 @@ class NeedsImpl extends AstNodeImpl, TNeedsNode {
  }
 }

+class OnImpl extends AstNodeImpl, TOnNode {
+  YamlMappingLikeNode n;
+
+  OnImpl() { this = TOnNode(n) }
+
+  override string toString() { result = n.toString() }
+
+  override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
+
+  override WorkflowImpl getParentNode() { result.getAChildNode() = this }
+
+  override string getAPrimaryQlClass() { result = "OnImpl" }
+
+  override Location getLocation() { result = n.getLocation() }
+
+  override YamlMappingLikeNode getNode() { result = n }
+
+  /** Gets an event that triggers the workflow. */
+  EventImpl getAnEvent() { result.getParentNode() = this }
+}
+
+class EventImpl extends AstNodeImpl, TEventNode {
+  YamlScalar e;
+  YamlMappingLikeNode n;
+
+  EventImpl() { this = TEventNode(e, n) }
+
+  override string toString() { result = e.getValue() }
+
+  override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
+
+  override OnImpl getParentNode() { result.getAChildNode() = this }
+
+  override string getAPrimaryQlClass() { result = "EventImpl" }
+
+  override Location getLocation() { result = e.getLocation() }
+
+  override YamlScalar getNode() { result = e }
+
+  /** Gets the name of the event that triggers the workflow. */
+  string getName() { result = e.getValue() }
+
+  /** Gets the Yaml Node associated with the event if any */
+  YamlMappingLikeNode getValueNode() { result = n }
+
+  /** Gets an activity type */
+  string getAnActivityType() {
+    result =
+      n.(YamlMapping).lookup("types").(YamlMappingLikeNode).getNode(_).(YamlScalar).getValue()
+  }
+
+  /** Gets a string value for any property (eg: branches, branches-ignore, etc.) */
+  string getAPropertyValue(string prop) {
+    result = n.(YamlMapping).lookup(prop).(YamlMappingLikeNode).getNode(_).(YamlScalar).getValue()
+  }
+
+  /** Holds if the event has a property with the given name */
+  predicate hasProperty(string prop) { exists(this.getAPropertyValue(prop)) }
+}
+
 class JobImpl extends AstNodeImpl, TJobNode {
  YamlMapping n;
  string jobId;
@@ -686,7 +752,7 @@ class JobImpl extends AstNodeImpl, TJobNode {
    // For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission unless the permissions key is specified and the workflow can access secrets, even when it is triggered from a fork.
    // The Job is triggered by an event other than `pull_request`
    count(this.getATriggerEvent()) = 1 and
-    not this.getATriggerEvent() = ["pull_request", "workflow_call"]
+    not this.getATriggerEvent().getName() = ["pull_request", "workflow_call"]
    or
    // The Workflow is only triggered by `workflow_call` and there is
    // a caller workflow triggered by an event other than `pull_request`
@@ -701,16 +767,11 @@ class JobImpl extends AstNodeImpl, TJobNode {
    count(this.getATriggerEvent()) > 1
  }

-  /** Workflow is triggered by given trigger event */
-  predicate hasTriggerEvent(string trigger) {
-    exists(YamlNode y | y = n.lookup("on").(YamlMappingLikeNode).getNode(trigger))
-  }
-
  /** Gets the trigger event that starts this workflow. */
-  string getATriggerEvent() { result = this.getEnclosingWorkflow().getATriggerEvent() }
+  EventImpl getATriggerEvent() { result = this.getEnclosingWorkflow().getATriggerEvent() }

  private predicate hasSingleTrigger(string trigger) {
-    this.getATriggerEvent() = trigger and
+    this.getATriggerEvent().getName() = trigger and
    count(this.getATriggerEvent()) = 1
  }

--- a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll
+++ b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll
@@ -10,6 +10,49 @@ string defaultBranchTriggerEvent() {
    ]
 }

+string defaultBranchNames() { result = ["main", "master", "default"] }
+
+predicate runsOnDefaultBranch(Job j) {
+  exists(Event e |
+    j.getATriggerEvent() = e and
+    (
+      e.getName() = defaultBranchTriggerEvent() and
+      not e.getName() = "pull_request_target"
+      or
+      e.getName() = "push" and
+      e.getAPropertyValue("branches") = defaultBranchNames()
+      or
+      e.getName() = "pull_request_target" and
+      (
+        // no filtering
+        not e.hasProperty("branches") and not e.hasProperty("branches-ignore")
+        or
+        // only branches-ignore filter
+        e.hasProperty("branches-ignore") and
+        not e.hasProperty("branches") and
+        not e.getAPropertyValue("branches-ignore") = defaultBranchNames()
+        or
+        // only branches filter
+        e.hasProperty("branches") and
+        not e.hasProperty("branches-ignore") and
+        e.getAPropertyValue("branches") = defaultBranchNames()
+        or
+        // branches and branches-ignore filters
+        e.hasProperty("branches") and
+        e.hasProperty("branches-ignore") and
+        e.getAPropertyValue("branches") = defaultBranchNames() and
+        not e.getAPropertyValue("branches-ignore") = defaultBranchNames()
+      )
+    )
+  )
+  or
+  j.getATriggerEvent().getName() = "workflow_call" and
+  exists(ExternalJob call |
+    call.getCallee() = j.getLocation().getFile().getRelativePath() and
+    runsOnDefaultBranch(call)
+  )
+}
+
 abstract class CacheWritingStep extends Step { }

 class CacheActionUsesStep extends CacheWritingStep, UsesStep {
--- a/ql/src/Security/CWE-284/CodeExecutionOnSelfHostedRunner.ql
+++ b/ql/src/Security/CWE-284/CodeExecutionOnSelfHostedRunner.ql
@@ -21,7 +21,7 @@ import codeql.actions.dataflow.ExternalFlow
 */
 predicate staticallyIdentifiedSelfHostedRunner(Job job) {
  exists(string label |
-    job.getEnclosingWorkflow().getATriggerEvent() =
+    job.getEnclosingWorkflow().getATriggerEvent().getName() =
      ["pull_request", "pull_request_review", "pull_request_review_comment", "pull_request_target"] and
    label = job.getARunsOnLabel() and
    // source: https://github.com/boostsecurityio/poutine/blob/main/opa/rego/poutine/utils.rego#L49C3-L49C136
--- a/ql/src/Security/CWE-349/CachePoisoning.ql
+++ b/ql/src/Security/CWE-349/CachePoisoning.ql
@@ -21,19 +21,7 @@ where
  // Excluding privileged workflows since they can be easily exploited in similar circumstances
  not j.isPrivileged() and
  // The workflow runs in the context of the default branch
-  // TODO: (require to collect trigger types)
-  // - add push to default branch?
-  // - exclude pull_request_target when branches_ignore includes default branch or when branches does not include the default branch
-  (
-    j.getEnclosingWorkflow().hasTriggerEvent(defaultBranchTriggerEvent())
-    or
-    j.getEnclosingWorkflow().hasTriggerEvent("workflow_call") and
-    exists(ExternalJob call, Workflow caller |
-      call.getCallee() = j.getLocation().getFile().getRelativePath() and
-      caller = call.getWorkflow() and
-      caller.hasTriggerEvent(defaultBranchTriggerEvent())
-    )
-  ) and
+  runsOnDefaultBranch(j) and
  // The job checkouts untrusted code from a pull request
  j.getAStep() = checkout and
  (
--- a/ql/src/Security/CWE-349/CachePoisoningByCodeInjection.ql
+++ b/ql/src/Security/CWE-349/CachePoisoningByCodeInjection.ql
@@ -21,21 +21,10 @@ from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink, Local
 where
  CodeInjectionFlow::flowPath(source, sink) and
  j = sink.getNode().asExpr().getEnclosingJob() and
+  // Excluding privileged workflows since they can be easily exploited in similar circumstances
  not j.isPrivileged() and
  // The workflow runs in the context of the default branch
-  // TODO: (require to collect trigger types)
-  // - add push to default branch?
-  // - exclude pull_request_target when branches_ignore includes default branch or when branches does not include the default branch
-  (
-    j.getEnclosingWorkflow().hasTriggerEvent(defaultBranchTriggerEvent())
-    or
-    j.getEnclosingWorkflow().hasTriggerEvent("workflow_call") and
-    exists(ExternalJob call, Workflow caller |
-      call.getCallee() = j.getLocation().getFile().getRelativePath() and
-      caller = call.getWorkflow() and
-      caller.hasTriggerEvent(defaultBranchTriggerEvent())
-    )
-  )
+  runsOnDefaultBranch(j)
 select sink.getNode(), source, sink,
  "Unprivileged code injection in $@, which may lead to cache poisoning.", sink,
  sink.getNode().asExpr().(Expression).getRawExpression()
--- a/ql/test/library-tests/test.expected
+++ b/ql/test/library-tests/test.expected
@@ -98,6 +98,10 @@ runStepChildren
 | .github/workflows/test.yml:39:9:40:53 | Run Step: sink | .github/workflows/test.yml:40:14:40:52 | echo ${{needs.job1.outputs.job_output}} |
 parentNodes
 | .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment | .github/workflows/expression_nodes.yml:1:1:21:47 | on: issue_comment |
+| .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment | .github/workflows/expression_nodes.yml:1:1:21:47 | on: issue_comment |
+| .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment | .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment |
+| .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment | .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment |
+| .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment | .github/workflows/expression_nodes.yml:1:5:1:17 | issue_comment |
 | .github/workflows/expression_nodes.yml:5:5:21:47 | Job: echo-chamber | .github/workflows/expression_nodes.yml:1:1:21:47 | on: issue_comment |
 | .github/workflows/expression_nodes.yml:5:14:5:26 | ubuntu-latest | .github/workflows/expression_nodes.yml:1:1:21:47 | on: issue_comment |
 | .github/workflows/expression_nodes.yml:5:14:5:26 | ubuntu-latest | .github/workflows/expression_nodes.yml:5:5:21:47 | Job: echo-chamber |
@@ -136,8 +140,14 @@ parentNodes
 | .github/workflows/expression_nodes.yml:20:14:21:46 | LINE 1 echo '${{ github.event.comment.body }}' echo '${{github.event.issue.body}}' | .github/workflows/expression_nodes.yml:20:9:21:47 | Run Step |
 | .github/workflows/expression_nodes.yml:20:14:21:46 | github.event.comment.body | .github/workflows/expression_nodes.yml:20:14:21:46 | LINE 1 echo '${{ github.event.comment.body }}' echo '${{github.event.issue.body}}' |
 | .github/workflows/expression_nodes.yml:20:14:21:46 | github.event.issue.body | .github/workflows/expression_nodes.yml:20:14:21:46 | LINE 1 echo '${{ github.event.comment.body }}' echo '${{github.event.issue.body}}' |
+| .github/workflows/multiline.yml:2:3:2:14 | workflow_run | .github/workflows/multiline.yml:2:3:5:18 | workflow_run: |
+| .github/workflows/multiline.yml:2:3:5:18 | workflow_run: | .github/workflows/multiline.yml:1:1:33:14 | on:  |
 | .github/workflows/multiline.yml:3:17:3:22 | Prev | .github/workflows/multiline.yml:1:1:33:14 | on:  |
+| .github/workflows/multiline.yml:3:17:3:22 | Prev | .github/workflows/multiline.yml:2:3:2:14 | workflow_run |
+| .github/workflows/multiline.yml:3:17:3:22 | Prev | .github/workflows/multiline.yml:2:3:5:18 | workflow_run: |
 | .github/workflows/multiline.yml:5:9:5:17 | completed | .github/workflows/multiline.yml:1:1:33:14 | on:  |
+| .github/workflows/multiline.yml:5:9:5:17 | completed | .github/workflows/multiline.yml:2:3:2:14 | workflow_run |
+| .github/workflows/multiline.yml:5:9:5:17 | completed | .github/workflows/multiline.yml:2:3:5:18 | workflow_run: |
 | .github/workflows/multiline.yml:9:5:33:14 | Job: Test | .github/workflows/multiline.yml:1:1:33:14 | on:  |
 | .github/workflows/multiline.yml:9:14:9:26 | ubuntu-latest | .github/workflows/multiline.yml:1:1:33:14 | on:  |
 | .github/workflows/multiline.yml:9:14:9:26 | ubuntu-latest | .github/workflows/multiline.yml:9:5:33:14 | Job: Test |
@@ -163,6 +173,10 @@ parentNodes
 | .github/workflows/multiline.yml:30:14:33:14 | cat <<-"EOF" > event.json\n  ${{ toJson(github.event) }}\nEOF\n | .github/workflows/multiline.yml:30:9:33:14 | Run Step |
 | .github/workflows/multiline.yml:32:13:32:39 | toJson(github.event) | .github/workflows/multiline.yml:30:14:33:14 | cat <<-"EOF" > event.json\n  ${{ toJson(github.event) }}\nEOF\n |
 | .github/workflows/test.yml:1:5:1:8 | push | .github/workflows/test.yml:1:1:40:53 | on: push |
+| .github/workflows/test.yml:1:5:1:8 | push | .github/workflows/test.yml:1:1:40:53 | on: push |
+| .github/workflows/test.yml:1:5:1:8 | push | .github/workflows/test.yml:1:5:1:8 | push |
+| .github/workflows/test.yml:1:5:1:8 | push | .github/workflows/test.yml:1:5:1:8 | push |
+| .github/workflows/test.yml:1:5:1:8 | push | .github/workflows/test.yml:1:5:1:8 | push |
 | .github/workflows/test.yml:5:5:31:2 | Job: job1 | .github/workflows/test.yml:1:1:40:53 | on: push |
 | .github/workflows/test.yml:5:14:5:26 | ubuntu-latest | .github/workflows/test.yml:1:1:40:53 | on: push |
 | .github/workflows/test.yml:5:14:5:26 | ubuntu-latest | .github/workflows/test.yml:5:5:31:2 | Job: job1 |
--- a/ql/test/query-tests/Security/CWE-349/.github/workflows/test13.yml
+++ b/ql/test/query-tests/Security/CWE-349/.github/workflows/test13.yml
@@ -0,0 +1,22 @@
+name: Cache Poisoning
+
+on:
+  pull_request_target:
+    branches:
+      - foo
+
+permissions: read-all
+
+jobs:
+  poison:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+      - uses: actions/cache@v2
+        with:
+          path: ./poison
+          key: poison_key
+      - run: |
+          cat poison
--- a/ql/test/query-tests/Security/CWE-349/.github/workflows/test14.yml
+++ b/ql/test/query-tests/Security/CWE-349/.github/workflows/test14.yml
@@ -0,0 +1,22 @@
+name: Cache Poisoning
+
+on:
+  pull_request_target:
+    branches-ignore:
+      - main
+
+permissions: read-all
+
+jobs:
+  poison:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+      - uses: actions/cache@v2
+        with:
+          path: ./poison
+          key: poison_key
+      - run: |
+          cat poison
--- a/ql/test/query-tests/Security/CWE-349/.github/workflows/test15.yml
+++ b/ql/test/query-tests/Security/CWE-349/.github/workflows/test15.yml
@@ -0,0 +1,22 @@
+name: Cache Poisoning
+
+on:
+  pull_request_target:
+    branches:
+      - main
+
+permissions: read-all
+
+jobs:
+  poison:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+      - uses: actions/cache@v2
+        with:
+          path: ./poison
+          key: poison_key
+      - run: |
+          cat poison
--- a/ql/test/query-tests/Security/CWE-349/.github/workflows/test16.yml
+++ b/ql/test/query-tests/Security/CWE-349/.github/workflows/test16.yml
@@ -0,0 +1,22 @@
+name: Cache Poisoning
+
+on:
+  pull_request_target:
+    branches-ignore:
+      - foo
+
+permissions: read-all
+
+jobs:
+  poison:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}
+      - uses: actions/cache@v2
+        with:
+          path: ./poison
+          key: poison_key
+      - run: |
+          cat poison
--- a/ql/test/query-tests/Security/CWE-349/CachePoisoning.expected
+++ b/ql/test/query-tests/Security/CWE-349/CachePoisoning.expected
@@ -6,3 +6,5 @@
 | .github/workflows/test8.yml:12:9:15:6 | Uses Step | Potential cache poisoning in the context of the default branch on step $@. | .github/workflows/test8.yml:15:9:17:2 | Run Step | Run Step |
 | .github/workflows/test8.yml:23:9:26:6 | Uses Step | Potential cache poisoning in the context of the default branch on step $@. | .github/workflows/test8.yml:26:9:28:2 | Uses Step | Uses Step |
 | .github/workflows/test8.yml:34:9:37:6 | Uses Step | Potential cache poisoning in the context of the default branch on step $@. | .github/workflows/test8.yml:37:9:37:75 | Run Step | Run Step |
+| .github/workflows/test15.yml:14:9:17:6 | Uses Step | Potential cache poisoning in the context of the default branch on step $@. | .github/workflows/test15.yml:17:9:21:6 | Uses Step | Uses Step |
+| .github/workflows/test16.yml:14:9:17:6 | Uses Step | Potential cache poisoning in the context of the default branch on step $@. | .github/workflows/test16.yml:17:9:21:6 | Uses Step | Uses Step |