From a2210dca79e5bbd4bda0b1bbe3d965c58facfdc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 20 Feb 2024 21:48:29 +0100 Subject: [PATCH 1/2] feat(triggers): Add getEnclosingWorkflowStmt to Statement class --- ql/lib/codeql/actions/Ast.qll | 22 ++++++--- .../CWE-094/CriticalExpressionInjection.ql | 47 +++++++++++++++++++ .../Security/CWE-094/ExpressionInjection.ql | 2 +- ql/src/test/.github/workflows/simple2.yml | 5 +- 4 files changed, 64 insertions(+), 12 deletions(-) create mode 100644 ql/src/Security/CWE-094/CriticalExpressionInjection.ql diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll index 605f658b263..5037a55d632 100644 --- a/ql/lib/codeql/actions/Ast.qll +++ b/ql/lib/codeql/actions/Ast.qll @@ -20,7 +20,10 @@ 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() { exists(WorkflowStmt w | w.getAChildNode*() = result) } +} /** * 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 +56,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 +133,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 +230,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 +295,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 +328,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/src/Security/CWE-094/CriticalExpressionInjection.ql b/ql/src/Security/CWE-094/CriticalExpressionInjection.ql new file mode 100644 index 00000000000..624bd32e45c --- /dev/null +++ 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; + +import MyFlow::PathGraph + +from MyFlow::PathNode source, MyFlow::PathNode sink +where + MyFlow::flowPath(source, sink) and + source + .getNode() + .asExpr() + .(Statement) + .getEnclosingWorkflowStmt() + .hasTriggerEvent("pull_request_target") +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 index 99779d6cc90..c34fcb74bbc 100644 --- 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 index b40f5eb6ac0..8271f93d857 100644 --- 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: From 3814462266e9482cc9686f60257006f45f8165bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Wed, 21 Feb 2024 10:23:37 +0100 Subject: [PATCH 2/2] feat(triggers): New query for critical issues Adds a new query and the required changes to be able to account for the trigger events so that we dont report issues if they are not likely exploitable. --- ql/lib/codeql/actions/Ast.qll | 4 +- .../codeql/actions/dataflow/ExternalFlow.qll | 6 ++- .../codeql/actions/dataflow/FlowSources.qll | 38 ++++++++++++++----- .../dataflow/internal/DataFlowPrivate.qll | 4 +- ...ahmadnassri_action-changed-files.model.yml | 2 - ql/lib/ext/dorny_paths-filter.model.yml | 1 - .../ext/jitterbit_get-changed-files.model.yml | 7 ---- ql/lib/ext/tj-actions_branch-names.model.yml | 11 ++++++ ql/lib/ext/tj-actions_changed-files.model.yml | 17 --------- .../tj-actions_verify-changed-files.model.yml | 1 - .../CWE-094/CriticalExpressionInjection.ql | 2 +- 11 files changed, 50 insertions(+), 43 deletions(-) create mode 100644 ql/lib/ext/tj-actions_branch-names.model.yml diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll index 5037a55d632..2e93187b6bf 100644 --- a/ql/lib/codeql/actions/Ast.qll +++ b/ql/lib/codeql/actions/Ast.qll @@ -22,7 +22,9 @@ class AstNode instanceof YamlNode { */ class Statement extends AstNode { /** Gets the workflow that this job is a part of. */ - WorkflowStmt getEnclosingWorkflowStmt() { exists(WorkflowStmt w | w.getAChildNode*() = result) } + WorkflowStmt getEnclosingWorkflowStmt() { + exists(WorkflowStmt w | w.getAChildNode*() = this and result = w) + } } /** diff --git a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll index 6446fbb5572..594b6017729 100644 --- 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 index 09094f2c580..0e82498bfc1 100644 --- 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,20 +111,33 @@ 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 } } /** @@ -130,10 +145,13 @@ private class EventSource extends RemoteFlowSource { */ 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 index 89f31983189..ae99e7c9184 100644 --- 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 @@ -201,7 +201,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 index 8f449f6b26d..34cb56a01ad 100644 --- 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 index 6ee41e93826..6fefec9a4f8 100644 --- 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 index f19a2da37f5..d7cbde25b88 100644 --- 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 new file mode 100644 index 00000000000..20383f415c2 --- /dev/null +++ b/ql/lib/ext/tj-actions_branch-names.model.yml @@ -0,0 +1,11 @@ +extensions: + - addsTo: + pack: githubsecuritylab/actions-all + extensible: sourceModel + data: + # https://github.com/tj-actions/branch-names + - ["tj-actions/branch-names", "*", "output.base_ref_branch", "pull_request_target", "PR base branch"] + - ["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 index fc5557db6ea..21a0b479ef5 100644 --- 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 index 76d83bd249e..9b6649892af 100644 --- 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 index 624bd32e45c..a6baf060c9d 100644 --- a/ql/src/Security/CWE-094/CriticalExpressionInjection.ql +++ b/ql/src/Security/CWE-094/CriticalExpressionInjection.ql @@ -42,6 +42,6 @@ where .asExpr() .(Statement) .getEnclosingWorkflowStmt() - .hasTriggerEvent("pull_request_target") + .hasTriggerEvent(source.getNode().(RemoteFlowSource).getATriggerEvent()) select sink.getNode(), source, sink, "Potential expression injection, which may be controlled by an external user."