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: