Merge pull request #38 from github/expr_trigger_mapping

Ensure event sources are available for triggering events
microsoft · May 17, 2024 · 0456dcd · 0456dcd
2 parents 47a66e1 + f325d40
commit 0456dcd
Show file tree

Hide file tree

Showing 9 changed files with 154 additions and 50 deletions.
diff --git a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll
@@ -2,19 +2,42 @@ private import internal.ExternalFlowExtensions as Extensions
 private import codeql.actions.DataFlow
 private import actions
 
+/**
+ * MaD models for workflow details
+ * Fields:
+ *    - path: Path to the workflow file
+ *    - trigger: Trigger for the workflow
+ *    - job: Job name
+ *    - secrets_source: Source of secrets
+ *    - permissions: Permissions for the workflow
+ *    - runner: Runner info for the workflow
+ */
 predicate workflowDataModel(
-  string path, string trigger, string job, string secrets_source, string permissions,
-  string runner
+  string path, string trigger, string job, string secrets_source, string permissions, string runner
 ) {
   Extensions::workflowDataModel(path, trigger, job, secrets_source, permissions, runner)
 }
 
-predicate repositoryDataModel(
-  string visibility, string default_branch_name
-) {
+/**
+ * MaD models for repository details
+ * Fields:
+ *    - visibility: Visibility of the repository
+ *    - default_branch_name: Default branch name
+ */
+predicate repositoryDataModel(string visibility, string default_branch_name) {
   Extensions::repositoryDataModel(visibility, default_branch_name)
 }
 
+/**
+ * MaD models for context/trigger mapping
+ * Fields:
+ *    - trigger: Trigger for the workflow
+ *    - context_prefix: Prefix for the context
+ */
+predicate contextTriggerDataModel(string trigger, string context_prefix) {
+  Extensions::contextTriggerDataModel(trigger, context_prefix)
+}
+
 /**
  * MaD sources
  * Fields:

diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll
@@ -95,8 +95,7 @@ private predicate branchEvent(string context) {
         // - They cannot contain a \
         // eg: zzz";echo${IFS}"hello";# would be a valid branch name
         "github\\.event\\.pull_request\\.head\\.repo\\.default_branch",
-        "github\\.event\\.pull_request\\.head\\.ref", "github\\.head_ref",
-        "github\\.event\\.workflow_run\\.head_branch",
+        "github\\.event\\.pull_request\\.head\\.ref", "github\\.event\\.workflow_run\\.head_branch",
         "github\\.event\\.workflow_run\\.pull_requests\\[[0-9]+\\]\\.head\\.ref",
         "github\\.event\\.merge_group\\.head_ref",
       ]
@@ -165,7 +164,8 @@ private predicate pathEvent(string context) {
     reg =
       [
         // filename
-        "github\\.event\\.workflow\\.path",
+        "github\\.event\\.workflow\\.path", "github\\.event\\.workflow_run\\.path",
+        "github\\.event\\.workflow_run\\.referenced_workflows\\.path",
       ]
   |
     Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
@@ -197,11 +197,33 @@ private predicate jsonEvent(string context) {
   )
 }
 
-class EventSource extends RemoteFlowSource {
+class GitHubSource extends RemoteFlowSource {
   string flag;
 
-  EventSource() {
-    exists(Expression e, string context | this.asExpr() = e and context = e.getExpression() |
+  GitHubSource() {
+    exists(Expression e, string context, string context_prefix |
+      this.asExpr() = e and
+      context = e.getExpression() and
+      Utils::normalizeExpr(context) = "github.head_ref" and
+      contextTriggerDataModel(e.getEnclosingWorkflow().getATriggerEvent().getName(), context_prefix) and
+      Utils::normalizeExpr(context).matches("%" + context_prefix + "%") and
+      flag = "branch"
+    )
+  }
+
+  override string getSourceType() { result = flag }
+}
+
+class GitHubEventSource extends RemoteFlowSource {
+  string flag;
+
+  GitHubEventSource() {
+    exists(Expression e, string context, string context_prefix |
+      this.asExpr() = e and
+      context = e.getExpression() and
+      contextTriggerDataModel(e.getEnclosingWorkflow().getATriggerEvent().getName(), context_prefix) and
+      Utils::normalizeExpr(context).matches("%" + context_prefix + "%")
+    |
       titleEvent(context) and flag = "title"
       or
       urlEvent(context) and flag = "url"
@@ -217,8 +239,33 @@ class EventSource extends RemoteFlowSource {
       usernameEvent(context) and flag = "username"
       or
       pathEvent(context) and flag = "filename"
-      or
-      jsonEvent(context) and flag = "json"
+    )
+  }
+
+  override string getSourceType() { result = flag }
+}
+
+class GitHubEventJsonSource extends RemoteFlowSource {
+  string flag;
+
+  GitHubEventJsonSource() {
+    exists(Expression e, string context |
+      this.asExpr() = e and
+      context = e.getExpression() and
+      (
+        jsonEvent(context) and
+        (
+          exists(string context_prefix |
+            contextTriggerDataModel(e.getEnclosingWorkflow().getATriggerEvent().getName(),
+              context_prefix) and
+            Utils::normalizeExpr(context).matches("%" + context_prefix + "%")
+          )
+          or
+          contextTriggerDataModel(e.getEnclosingWorkflow().getATriggerEvent().getName(), _) and
+          Utils::normalizeExpr(context).regexpMatch(".*\\bgithub.event\\b.*")
+        )
+      ) and
+      flag = "json"
     )
   }
 

diff --git a/ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll b/ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll
@@ -23,11 +23,19 @@ extensible predicate sinkModel(
   string action, string version, string input, string kind, string provenance
 );
 
+/**
+ * Holds if workflow data model exists for the given parameters.
+ */
 extensible predicate workflowDataModel(
-  string path, string trigger, string job, string secrets_source, string permissions,
-  string runner
+  string path, string trigger, string job, string secrets_source, string permissions, string runner
 );
 
-extensible predicate repositoryDataModel(
-  string visibility, string default_branch_name
-);
+/**
+ * Holds if repository data model exists for the given parameters.
+ */
+extensible predicate repositoryDataModel(string visibility, string default_branch_name);
+
+/**
+ * Holds if context/trigger mapping exists for the given parameters.
+ */
+extensible predicate contextTriggerDataModel(string trigger, string context_prefix);
diff --git a/ql/lib/ext/workflow-models/workflow-models.yml b/ql/lib/ext/workflow-models/workflow-models.yml
@@ -7,3 +7,51 @@ extensions:
       pack: githubsecuritylab/actions-all
       extensible: workflowDataModel
     data: []
+  - addsTo:
+      pack: githubsecuritylab/actions-all
+      extensible: contextTriggerDataModel 
+    data:
+      # This predicate maps triggering events with the github event context available for that event
+      - ["commit_comment", "github.event.comment"] 
+      - ["discussion", "github.event.discussion"]
+      - ["discussion_comment", "github.event.comment"] 
+      - ["discussion_comment", "github.event.discussion"]
+      - ["issues", "github.event.issue"] 
+      - ["issue_comment", "github.event.issue"] 
+      - ["issue_comment", "github.event.comment"] 
+      - ["gollum", "github.event.pages"] 
+      - ["merge_group", "github.event.merge_group"] 
+      - ["pull_request", "github.event.pull_request"] 
+      - ["pull_request", "github.head_ref"] 
+      - ["pull_request_comment", "github.event.comment"] 
+      - ["pull_request_comment", "github.event.pull_request"] 
+      - ["pull_request_comment", "github.head_ref"] 
+      - ["pull_request_review", "github.event.pull_request"] 
+      - ["pull_request_review", "github.event.review"] 
+      - ["pull_request_review", "github.head_ref"] 
+      - ["pull_request_review_comment", "github.event.comment"] 
+      - ["pull_request_review_comment", "github.event.pull_request"] 
+      - ["pull_request_review_comment", "github.event.review"] 
+      - ["pull_request_review_comment", "github.head_ref"] 
+      - ["pull_request_target", "github.event.pull_request"] 
+      - ["pull_request_target", "github.head_ref"] 
+      - ["push", "github.event.commits"]
+      - ["push", "github.event.head_commit"]
+      - ["repository_dispatch", "github.event.client_payload"]
+      - ["workflow_dispatch", "github.event.inputs"]
+      - ["workflow_run", "github.event.workflow"]
+      - ["workflow_run", "github.event.workflow_run"]
+      # workflow_call receives the same event payload as the calling workflow
+      - ["workflow_call", "github.event.client_payload"]
+      - ["workflow_call", "github.event.comment"] 
+      - ["workflow_call", "github.event.commits"]
+      - ["workflow_call", "github.event.discussion"]
+      - ["workflow_call", "github.event.head_commit"]
+      - ["workflow_call", "github.event.inputs"]
+      - ["workflow_call", "github.event.issue"] 
+      - ["workflow_call", "github.event.merge_group"] 
+      - ["workflow_call", "github.event.pages"] 
+      - ["workflow_call", "github.event.pull_request"] 
+      - ["workflow_call", "github.event.review"] 
+      - ["workflow_call", "github.event.workflow"]
+      - ["workflow_call", "github.event.workflow_run"]
diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/pull_request_target.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/pull_request_target.yml
@@ -4,8 +4,8 @@ jobs:
   echo-chamber:
     runs-on: ubuntu-latest
     steps:
-    - run: echo '${{ github.event.issue.title }}' # not defined for this trigger, but we will still report it
-    - run: echo '${{ github.event.issue.body }}' # not defined for this trigger, but we will still report it
+    - run: echo '${{ github.event.issue.title }}' # not defined for this trigger, so we should not report it
+    - run: echo '${{ github.event.issue.body }}' # not defined for this trigger, so we should not report it
     - run: echo '${{ github.event.pull_request.title }}'
     - run: echo '${{ github.event.pull_request.body }}'
     - run: echo '${{ github.event.pull_request.head.label }}'
@@ -14,3 +14,4 @@ jobs:
     - run: echo '${{ github.event.pull_request.head.repo.homepage }}'
     - run: echo '${{ github.event.pull_request.head.ref }}'
     - run: echo '${{ github.head_ref }}'
+
diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/self_needs.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/self_needs.yml
@@ -13,7 +13,7 @@ jobs:
       - id: source
         uses: mad9000/actions-find-and-replace-string@3
         with:
-          source: ${{ github.event['head_commit']['message'] }}
+          source: ${{ github.event['comment']['body'] }}
           find: 'foo'
           replace: ''
       - run: ${{ steps.source.outputs.value }}

diff --git a/ql/test/query-tests/Security/CWE-094/CodeInjection.expected b/ql/test/query-tests/Security/CWE-094/CodeInjection.expected
@@ -50,7 +50,7 @@ edges
 | .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value | .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] |
 | .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value |
 | .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value |
-| .github/workflows/self_needs.yml:16:20:16:64 | github.event['head_commit']['message'] | .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] |
+| .github/workflows/self_needs.yml:16:20:16:57 | github.event['comment']['body'] | .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] |
 | .github/workflows/simple1.yml:8:9:14:6 | Uses Step: summary [value] | .github/workflows/simple1.yml:16:18:16:49 | steps.summary.outputs.value |
 | .github/workflows/simple1.yml:11:20:11:58 | github.event.head_commit.message | .github/workflows/simple1.yml:8:9:14:6 | Uses Step: summary [value] |
 | .github/workflows/simple2.yml:14:9:18:6 | Uses Step: source | .github/workflows/simple2.yml:22:20:22:64 | steps.source.outputs.all_changed_files |
@@ -183,8 +183,6 @@ nodes
 | .github/workflows/pull_request_review_comment.yml:12:19:12:69 | github.event.pull_request.head.repo.homepage | semmle.label | github.event.pull_request.head.repo.homepage |
 | .github/workflows/pull_request_review_comment.yml:13:19:13:59 | github.event.pull_request.head.ref | semmle.label | github.event.pull_request.head.ref |
 | .github/workflows/pull_request_review_comment.yml:14:19:14:50 | github.event.comment.body | semmle.label | github.event.comment.body |
-| .github/workflows/pull_request_target.yml:7:19:7:49 | github.event.issue.title | semmle.label | github.event.issue.title |
-| .github/workflows/pull_request_target.yml:8:19:8:48 | github.event.issue.body | semmle.label | github.event.issue.body |
 | .github/workflows/pull_request_target.yml:9:19:9:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
 | .github/workflows/pull_request_target.yml:10:19:10:55 | github.event.pull_request.body | semmle.label | github.event.pull_request.body |
 | .github/workflows/pull_request_target.yml:11:19:11:61 | github.event.pull_request.head.label | semmle.label | github.event.pull_request.head.label |
@@ -206,7 +204,7 @@ nodes
 | .github/workflows/self_needs.yml:11:7:12:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] |
 | .github/workflows/self_needs.yml:11:20:11:52 | steps.source.outputs.value | semmle.label | steps.source.outputs.value |
 | .github/workflows/self_needs.yml:13:9:19:6 | Uses Step: source [value] | semmle.label | Uses Step: source [value] |
-| .github/workflows/self_needs.yml:16:20:16:64 | github.event['head_commit']['message'] | semmle.label | github.event['head_commit']['message'] |
+| .github/workflows/self_needs.yml:16:20:16:57 | github.event['comment']['body'] | semmle.label | github.event['comment']['body'] |
 | .github/workflows/self_needs.yml:19:15:19:47 | steps.source.outputs.value | semmle.label | steps.source.outputs.value |
 | .github/workflows/self_needs.yml:20:15:20:51 | needs.test1.outputs.job_output | semmle.label | needs.test1.outputs.job_output |
 | .github/workflows/simple1.yml:8:9:14:6 | Uses Step: summary [value] | semmle.label | Uses Step: summary [value] |
@@ -254,12 +252,10 @@ nodes
 | .github/workflows/workflow_run.yml:14:19:14:77 | github.event.workflow_run.head_commit.committer.name | semmle.label | github.event.workflow_run.head_commit.committer.name |
 | .github/workflows/workflow_run.yml:15:19:15:62 | github.event.workflow_run.head_branch | semmle.label | github.event.workflow_run.head_branch |
 | .github/workflows/workflow_run.yml:16:19:16:78 | github.event.workflow_run.head_repository.description | semmle.label | github.event.workflow_run.head_repository.description |
-| action1/action.yml:14:19:14:50 | github.event.comment.body | semmle.label | github.event.comment.body |
 subpaths
 #select
 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | ${{ steps.changed-files1.outputs.all_changed_files }} |
 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | ${{ steps.changed-files3.outputs.all_changed_files }} |
 | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | .github/workflows/changed-files.yml:53:9:56:6 | Uses Step: changed-files5 | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | ${{ steps.changed-files5.outputs.all_changed_files }} |
 | .github/workflows/changelog.yml:58:26:58:39 | env.log | .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changelog.yml:58:26:58:39 | env.log | ${{ env.log }} |
 | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | ${{ env.log }} |
-| action1/action.yml:14:19:14:50 | github.event.comment.body | action1/action.yml:14:19:14:50 | github.event.comment.body | action1/action.yml:14:19:14:50 | github.event.comment.body | Potential code injection in $@, which may be controlled by an external user. | action1/action.yml:14:19:14:50 | github.event.comment.body | ${{ github.event.comment.body }} |