Merge pull request #18 from GitHubSecurityLab/triggers

Alvaro Muñoz · web-flow · commit d6f6e1fc0b6f · 2024-02-21T16:51:16.000+01:00
feat(triggers): New query and support for trigger-based severity decisions
diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll
@@ -20,7 +20,12 @@ class AstNode instanceof YamlNode {
  * A statement is a group of expressions and/or statements that you design to carry out a task or an action.
  * Any statement that can return a value is automatically qualified to be used as an expression.
  */
-class Statement extends AstNode { }
+class Statement extends AstNode {
+  /** Gets the workflow that this job is a part of. */
+  WorkflowStmt getEnclosingWorkflowStmt() {
+    this = result.getAChildNode*()
+  }
+}
 
 /**
  * An expression is any word or group of words or symbols that is a value. In programming, an expression is a value, or anything that executes and ends up being a value.
@@ -53,6 +58,14 @@ class WorkflowStmt extends Statement instanceof Actions::Workflow {
   JobStmt getAJobStmt() { result = super.getJob(_) }
 
   JobStmt getJobStmt(string id) { result = super.getJob(id) }
+
+  predicate hasTriggerEvent(string trigger) {
+    exists(YamlNode n | n = super.getOn().(YamlMappingLikeNode).getNode(trigger))
+  }
+
+  string getATriggerEvent() {
+    exists(YamlNode n | n = super.getOn().(YamlMappingLikeNode).getNode(result))
+  }
 }
 
 class ReusableWorkflowStmt extends WorkflowStmt {
@@ -122,9 +135,6 @@ class JobStmt extends Statement instanceof Actions::Job {
    */
   string getId() { result = super.getId() }
 
-  /** Gets the workflow that this job is a part of. */
-  WorkflowStmt getWorkflowStmt() { result = super.getWorkflow() }
-
   /** Gets the step at the given index within this job. */
   StepStmt getStepStmt(int index) { result = super.getStep(index) }
 
@@ -222,7 +232,7 @@ class StepUsesExpr extends StepStmt, UsesExpr {
     )
     or
     exists(Actions::WorkflowEnv env |
-      env.getWorkflow() = this.getJobStmt().getWorkflowStmt() and
+      env.getWorkflow() = this.getJobStmt().getEnclosingWorkflowStmt() and
       env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
     )
   }
@@ -287,7 +297,7 @@ class JobUsesExpr extends UsesExpr instanceof YamlMapping {
     )
     or
     exists(Actions::WorkflowEnv env |
-      env.getWorkflow() = this.getJobStmt().getWorkflowStmt() and
+      env.getWorkflow() = this.getJobStmt().getEnclosingWorkflowStmt() and
       env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
     )
   }
@@ -320,7 +330,7 @@ class RunExpr extends StepStmt, Expression {
     )
     or
     exists(Actions::WorkflowEnv env |
-      env.getWorkflow() = this.getJobStmt().getWorkflowStmt() and
+      env.getWorkflow() = this.getJobStmt().getEnclosingWorkflowStmt() and
       env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
     )
   }
diff --git a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll
@@ -39,8 +39,10 @@ predicate sinkModel(string action, string version, string input, string kind) {
   Extensions::sinkModel(action, version, input, kind)
 }
 
-predicate externallyDefinedSource(DataFlow::Node source, string sourceType, string fieldName) {
-  exists(UsesExpr uses, string action, string version, string trigger, string kind |
+predicate externallyDefinedSource(
+  DataFlow::Node source, string sourceType, string fieldName, string trigger
+) {
+  exists(UsesExpr uses, string action, string version, string kind |
     sourceModel(action, version, fieldName, trigger, kind) and
     uses.getCallee() = action.toLowerCase() and
     (
diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll
@@ -17,6 +17,8 @@ abstract class RemoteFlowSource extends SourceNode {
   /** Gets a string that describes the type of this remote flow source. */
   abstract string getSourceType();
 
+  abstract string getATriggerEvent();
+
   override string getThreatModel() { result = "remote" }
 }
 
@@ -109,31 +111,47 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
 }
 
 private class EventSource extends RemoteFlowSource {
+  string trigger;
+
   EventSource() {
     exists(ExprAccessExpr e, string context | this.asExpr() = e and context = e.getExpression() |
-      isExternalUserControlledIssue(context) or
-      isExternalUserControlledPullRequest(context) or
-      isExternalUserControlledReview(context) or
-      isExternalUserControlledComment(context) or
-      isExternalUserControlledGollum(context) or
-      isExternalUserControlledCommit(context) or
-      isExternalUserControlledDiscussion(context) or
-      isExternalUserControlledWorkflowRun(context)
+      trigger = ["issues", "issue_comment"] and isExternalUserControlledIssue(context)
+      or
+      trigger = ["pull_request_target", "pull_request_review", "pull_request_review_comment"] and
+      isExternalUserControlledPullRequest(context)
+      or
+      trigger = ["pull_request_review"] and isExternalUserControlledReview(context)
+      or
+      trigger = ["pull_request_review_comment", "issue_comment", "discussion_comment"] and
+      isExternalUserControlledComment(context)
+      or
+      trigger = ["gollum"] and isExternalUserControlledGollum(context)
+      or
+      trigger = ["push"] and isExternalUserControlledCommit(context)
+      or
+      trigger = ["discussion", "discussion_comment"] and isExternalUserControlledDiscussion(context)
+      or
+      trigger = ["workflow_run"] and isExternalUserControlledWorkflowRun(context)
     )
   }
 
   override string getSourceType() { result = "User-controlled events" }
+
+  override string getATriggerEvent() { result = trigger }
 }
 
 /**
  * A Source of untrusted data defined in a MaD specification
  */
 private class ExternallyDefinedSource extends RemoteFlowSource {
   string sourceType;
+  string trigger;
 
-  ExternallyDefinedSource() { externallyDefinedSource(this, sourceType, _) }
+  ExternallyDefinedSource() { externallyDefinedSource(this, sourceType, _, trigger) }
 
   override string getSourceType() { result = sourceType }
+
+  override string getATriggerEvent() { result = trigger }
 }
 
 /**
@@ -145,4 +163,6 @@ private class CompositeActionInputSource extends RemoteFlowSource {
   CompositeActionInputSource() { c.getInputsStmt().getInputExpr(_) = this.asExpr() }
 
   override string getSourceType() { result = "Composite action input" }
+
+  override string getATriggerEvent() { result = "*" }
 }
diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll
@@ -173,7 +173,7 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos =
  */
 predicate stepsCtxLocalStep(Node nodeFrom, Node nodeTo) {
   exists(UsesExpr astFrom, StepsCtxAccessExpr astTo |
-    externallyDefinedSource(nodeFrom, _, "output." + astTo.getFieldName()) and
+    externallyDefinedSource(nodeFrom, _, "output." + astTo.getFieldName(), _) and
     astFrom = nodeFrom.asExpr() and
     astTo = nodeTo.asExpr() and
     astTo.getRefExpr() = astFrom
@@ -190,7 +190,7 @@ predicate stepsCtxLocalStep(Node nodeFrom, Node nodeTo) {
  */
 predicate needsCtxLocalStep(Node nodeFrom, Node nodeTo) {
   exists(UsesExpr astFrom, NeedsCtxAccessExpr astTo |
-    externallyDefinedSource(nodeFrom, _, "output." + astTo.getFieldName()) and
+    externallyDefinedSource(nodeFrom, _, "output." + astTo.getFieldName(), _) and
     astFrom = nodeFrom.asExpr() and
     astTo = nodeTo.asExpr() and
     astTo.getRefExpr() = astFrom
@@ -218,7 +218,7 @@ predicate envCtxLocalStep(Node nodeFrom, Node nodeTo) {
     astFrom = nodeFrom.asExpr() and
     astTo = nodeTo.asExpr() and
     (
-      externallyDefinedSource(nodeFrom, _, "env." + astTo.getFieldName()) or
+      externallyDefinedSource(nodeFrom, _, "env." + astTo.getFieldName(), _) or
       astTo.getRefExpr() = astFrom
     )
   )
diff --git a/ql/lib/ext/ahmadnassri_action-changed-files.model.yml b/ql/lib/ext/ahmadnassri_action-changed-files.model.yml
@@ -3,7 +3,5 @@ extensions:
       pack: githubsecuritylab/actions-all
       extensible: sourceModel
     data:
-      - ["ahmadnassri/action-changed-files", "*", "output.files", "pull_request", "PR changed files"]
       - ["ahmadnassri/action-changed-files", "*", "output.files", "pull_request_target", "PR changed files"]
-      - ["ahmadnassri/action-changed-files", "*", "output.json", "pull_request", "PR changed files"]
       - ["ahmadnassri/action-changed-files", "*", "output.json", "pull_request_target", "PR changed files"]
diff --git a/ql/lib/ext/dorny_paths-filter.model.yml b/ql/lib/ext/dorny_paths-filter.model.yml
@@ -3,5 +3,4 @@ extensions:
       pack: githubsecuritylab/actions-all
       extensible: sourceModel
     data:
-      - ["dorny/paths-filter", "*", "output.changes", "pull_request", "PR changed files"]
       - ["dorny/paths-filter", "*", "output.changes", "pull_request_target", "PR changed files"]
diff --git a/ql/lib/ext/jitterbit_get-changed-files.model.yml b/ql/lib/ext/jitterbit_get-changed-files.model.yml
@@ -3,17 +3,10 @@ extensions:
       pack: githubsecuritylab/actions-all
       extensible: sourceModel
     data:
-      - ["jitterbit/get-changed-files", "*", "output.all", "pull_request", "PR changed files"]
       - ["jitterbit/get-changed-files", "*", "output.all", "pull_request_target", "PR changed files"]
-      - ["jitterbit/get-changed-files", "*", "output.added", "pull_request", "PR changed files"]
       - ["jitterbit/get-changed-files", "*", "output.added", "pull_request_target", "PR changed files"]
-      - ["jitterbit/get-changed-files", "*", "output.modified", "pull_request", "PR changed files"]
       - ["jitterbit/get-changed-files", "*", "output.modified", "pull_request_target", "PR changed files"]
-      - ["jitterbit/get-changed-files", "*", "output.removed", "pull_request", "PR changed files"]
       - ["jitterbit/get-changed-files", "*", "output.removed", "pull_request_target", "PR changed files"]
-      - ["jitterbit/get-changed-files", "*", "output.renamed", "pull_request", "PR changed files"]
       - ["jitterbit/get-changed-files", "*", "output.renamed", "pull_request_target", "PR changed files"]
-      - ["jitterbit/get-changed-files", "*", "output.added_modified", "pull_request", "PR changed files"]
       - ["jitterbit/get-changed-files", "*", "output.added_modified", "pull_request_target", "PR changed files"]
-      - ["jitterbit/get-changed-files", "*", "output.deleted", "pull_request", "PR changed files"]
       - ["jitterbit/get-changed-files", "*", "output.deleted", "pull_request_target", "PR changed files"]
diff --git a/ql/lib/ext/tj-actions_branch-names.model.yml b/ql/lib/ext/tj-actions_branch-names.model.yml
@@ -0,0 +1,10 @@
+extensions:
+  - addsTo:
+      pack: githubsecuritylab/actions-all
+      extensible: sourceModel
+    data:
+      # https://github.com/tj-actions/branch-names
+      - ["tj-actions/branch-names", "*", "output.current_branch", "pull_request_target", "PR current branch"]
+      - ["tj-actions/branch-names", "*", "output.head_ref_branch", "pull_request_target", "PR head branch"]
+      - ["tj-actions/branch-names", "*", "output.ref_branch", "pull_request_target", "Branch tirggering workflow run"]
+
diff --git a/ql/lib/ext/tj-actions_changed-files.model.yml b/ql/lib/ext/tj-actions_changed-files.model.yml
@@ -3,37 +3,20 @@ extensions:
       pack: githubsecuritylab/actions-all
       extensible: sourceModel
     data:
-      - ["tj-actions/changed-files", "*", "output.added_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.added_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.copied_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.copied_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.deleted_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.deleted_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.modified_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.modified_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.renamed_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.renamed_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.all_old_new_renamed_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.all_old_new_renamed_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.type_changed_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.type_changed_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.unmerged_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.unmerged_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.unknown_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.unknown_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.all_changed_and_modified_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.all_changed_and_modified_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.all_changed_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.all_changed_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.other_changed_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.other_changed_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.all_modified_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.all_modified_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.other_modified_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.other_modified_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.other_deleted_files", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.other_deleted_files", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.modified_keys", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.modified_keys", "pull_request_target", "PR changed files"]
-      - ["tj-actions/changed-files", "*", "output.changed_keys", "pull_request", "PR changed files"]
       - ["tj-actions/changed-files", "*", "output.changed_keys", "pull_request_target", "PR changed files"]
diff --git a/ql/lib/ext/tj-actions_verify-changed-files.model.yml b/ql/lib/ext/tj-actions_verify-changed-files.model.yml
@@ -3,5 +3,4 @@ extensions:
       pack: githubsecuritylab/actions-all
       extensible: sourceModel
     data:
-      - ["tj-actions/verify-changed-files", "*", "output.changed-files", "pull_request", "PR changed files"]
       - ["tj-actions/verify-changed-files", "*", "output.changed-files", "pull_request_target", "PR changed files"]
diff --git a/ql/src/Security/CWE-094/CriticalExpressionInjection.ql b/ql/src/Security/CWE-094/CriticalExpressionInjection.ql
@@ -0,0 +1,47 @@
+/**
+ * @name Expression injection in Actions
+ * @description Using user-controlled GitHub Actions contexts like `run:` or `script:` may allow a malicious
+ *              user to inject code into the GitHub action.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 9
+ * @precision high
+ * @id actions/critical-expression-injection
+ * @tags actions
+ *       security
+ *       external/cwe/cwe-094
+ */
+
+import actions
+import codeql.actions.TaintTracking
+import codeql.actions.dataflow.FlowSources
+import codeql.actions.dataflow.ExternalFlow
+
+private class ExpressionInjectionSink extends DataFlow::Node {
+  ExpressionInjectionSink() {
+    exists(RunExpr e | e.getScriptExpr() = this.asExpr()) or
+    externallyDefinedSink(this, "expression-injection")
+  }
+}
+
+private module MyConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof ExpressionInjectionSink }
+}
+
+module MyFlow = TaintTracking::Global<MyConfig>;
+
+import MyFlow::PathGraph
+
+from MyFlow::PathNode source, MyFlow::PathNode sink
+where
+  MyFlow::flowPath(source, sink) and
+  source
+      .getNode()
+      .asExpr()
+      .(Statement)
+      .getEnclosingWorkflowStmt()
+      .hasTriggerEvent(source.getNode().(RemoteFlowSource).getATriggerEvent())
+select sink.getNode(), source, sink,
+  "Potential expression injection, which may be controlled by an external user."
diff --git a/ql/src/Security/CWE-094/ExpressionInjection.ql b/ql/src/Security/CWE-094/ExpressionInjection.ql
@@ -4,7 +4,7 @@
  *              user to inject code into the GitHub action.
  * @kind path-problem
  * @problem.severity warning
- * @security-severity 9.3
+ * @security-severity 5.0
  * @precision high
  * @id actions/expression-injection
  * @tags actions
diff --git a/ql/src/test/.github/workflows/simple2.yml b/ql/src/test/.github/workflows/simple2.yml
@@ -1,9 +1,6 @@
 name: CI
 
-on:
-  pull_request:
-    branches:
-      - main
+on: [pull_request_target, pull_request]
 
 jobs:
   changed_files:

Original file line number	Diff line number	Diff line change
`@@ -39,8 +39,10 @@ predicate sinkModel(string action, string version, string input, string kind) {`
`39`	`39`	`Extensions::sinkModel(action, version, input, kind)`
`40`	`40`	`}`
`41`	`41`
`42`		`-predicate externallyDefinedSource(DataFlow::Node source, string sourceType, string fieldName) {`
`43`		`- exists(UsesExpr uses, string action, string version, string trigger, string kind \|`
	`42`	`+predicate externallyDefinedSource(`
	`43`	`+ DataFlow::Node source, string sourceType, string fieldName, string trigger`
	`44`	`+) {`
	`45`	`+ exists(UsesExpr uses, string action, string version, string kind \|`
`44`	`46`	`sourceModel(action, version, fieldName, trigger, kind) and`
`45`	`47`	`uses.getCallee() = action.toLowerCase() and`
`46`	`48`	`(`