diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll index 5daa99d142e..1e57c8f3d29 100644 --- 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() } diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll index fca9298794f..e8a92a41142 100644 --- 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 } diff --git a/ql/lib/codeql/actions/security/CachePoisoningQuery.qll b/ql/lib/codeql/actions/security/CachePoisoningQuery.qll index 9762e9d9078..ab0f2d0809a 100644 --- 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 { diff --git a/ql/src/Security/CWE-284/CodeExecutionOnSelfHostedRunner.ql b/ql/src/Security/CWE-284/CodeExecutionOnSelfHostedRunner.ql index c7bdfbbc323..90997f63631 100644 --- 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 diff --git a/ql/src/Security/CWE-349/CachePoisoning.ql b/ql/src/Security/CWE-349/CachePoisoning.ql index 7bcbe693566..11da318f474 100644 --- 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 ( diff --git a/ql/src/Security/CWE-349/CachePoisoningByCodeInjection.ql b/ql/src/Security/CWE-349/CachePoisoningByCodeInjection.ql index e02b64e9ec5..5d739d746d5 100644 --- 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() diff --git a/ql/test/library-tests/test.expected b/ql/test/library-tests/test.expected index c735596ae05..61f7120e78e 100644 --- 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 | diff --git a/ql/test/query-tests/Security/CWE-349/.github/workflows/test13.yml b/ql/test/query-tests/Security/CWE-349/.github/workflows/test13.yml new file mode 100644 index 00000000000..72106b9d69b --- /dev/null +++ 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 diff --git a/ql/test/query-tests/Security/CWE-349/.github/workflows/test14.yml b/ql/test/query-tests/Security/CWE-349/.github/workflows/test14.yml new file mode 100644 index 00000000000..31c820904cd --- /dev/null +++ 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 diff --git a/ql/test/query-tests/Security/CWE-349/.github/workflows/test15.yml b/ql/test/query-tests/Security/CWE-349/.github/workflows/test15.yml new file mode 100644 index 00000000000..d3f51456de2 --- /dev/null +++ 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 diff --git a/ql/test/query-tests/Security/CWE-349/.github/workflows/test16.yml b/ql/test/query-tests/Security/CWE-349/.github/workflows/test16.yml new file mode 100644 index 00000000000..ec0f9b0e6c9 --- /dev/null +++ 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 diff --git a/ql/test/query-tests/Security/CWE-349/CachePoisoning.expected b/ql/test/query-tests/Security/CWE-349/CachePoisoning.expected index 75a370246cb..f0ee6d70001 100644 --- 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 |