Skip to content

Commit 67c9648

Browse files
author
Alvaro Muñoz
authored
Merge pull request #29 from github/toctou_queries
TOCTOU queries
2 parents ff2cfa5 + 73fbd23 commit 67c9648

15 files changed

+253
-8
lines changed

‎ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -227,17 +227,46 @@ class GhSHACheckout extends SHACheckoutStep instanceof Run {
227227
}
228228

229229
/** An If node that contains an actor, user or label check */
230-
class ControlCheck extends If {
231-
ControlCheck() {
230+
abstract class ControlCheck extends If { }
231+
232+
class LabelControlCheck extends ControlCheck {
233+
LabelControlCheck() {
234+
// eg: contains(github.event.pull_request.labels.*.name, 'safe to test')
235+
// eg: github.event.label.name == 'safe to test'
236+
exists(
237+
Utils::normalizeExpr(this.getCondition())
238+
.regexpFind([
239+
"\\bgithub\\.event\\.pull_request\\.labels\\b", "\\bgithub\\.event\\.label\\.name\\b"
240+
], _, _)
241+
)
242+
}
243+
}
244+
245+
class ActorControlCheck extends ControlCheck {
246+
ActorControlCheck() {
247+
// eg: contains(github.actor, 'dependabot')
248+
// eg: github.triggering_actor != 'CI Agent'
249+
// eg: github.event.pull_request.user.login == 'mybot'
250+
exists(
251+
Utils::normalizeExpr(this.getCondition())
252+
.regexpFind([
253+
"\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b",
254+
"\\bgithub\\.event\\.comment\\.user\\.login\\b",
255+
"\\bgithub\\.event\\.pull_request\\.user\\.login\\b",
256+
], _, _)
257+
)
258+
}
259+
}
260+
261+
class AssociationControlCheck extends ControlCheck {
262+
AssociationControlCheck() {
263+
// eg: contains(fromJson('["MEMBER", "OWNER"]'), github.event.comment.author_association)
232264
exists(
233265
Utils::normalizeExpr(this.getCondition())
234266
.regexpFind([
235-
"\\bgithub\\.actor\\b", // actor
236-
"\\bgithub\\.triggering_actor\\b", // actor
237-
"\\bgithub\\.event\\.comment\\.user\\.login\\b", //user
238-
"\\bgithub\\.event\\.pull_request\\.user\\.login\\b", //user
239-
"\\bgithub\\.event\\.pull_request\\.labels\\b", // label
240-
"\\bgithub\\.event\\.label\\.name\\b" // label
267+
"\\bgithub\\.event\\.comment\\.author_association\\b",
268+
"\\bgithub\\.event\\.issue\\.author_association\\b",
269+
"\\bgithub\\.event\\.pull_request\\.author_association\\b",
241270
], _, _)
242271
)
243272
}

‎ql/src/Security/CWE-285/ImproperAccessControl.ql

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
/**
2+
* @name Improper Access Control
3+
* @description The access control mechanism is not properly implemented, allowing untrusted code to be executed in a privileged context.
4+
* @kind problem
5+
* @problem.severity error
6+
* @precision high
7+
* @security-severity 9.3
8+
* @id actions/improper-access-control
9+
* @tags actions
10+
* security
11+
* external/cwe/cwe-285
12+
*/
13+
14+
import codeql.actions.security.UntrustedCheckoutQuery
15+
16+
from LocalJob job, LabelControlCheck check, MutableRefCheckoutStep checkout, Event event
17+
where
18+
job = checkout.getEnclosingJob() and
19+
job.isPrivileged() and
20+
job.getATriggerEvent() = event and
21+
event.getName() = "pull_request_target" and
22+
event.getAnActivityType() = "synchronize" and
23+
job.getAStep() = checkout and
24+
(
25+
checkout.getIf() = check
26+
or
27+
checkout.getEnclosingJob().getIf() = check
28+
)
29+
select checkout, "The checked-out code can be changed after the authorization check o step $@.",
30+
check, check.toString()

‎ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/**
2+
* @name Untrusted Checkout TOCTOU
3+
* @description Untrusted Checkout is protected by a security check but the checked-out branch can be changed after the check.
4+
* @kind problem
5+
* @problem.severity error
6+
* @precision high
7+
* @security-severity 9.3
8+
* @id actions/untrusted-checkout-toctou/critical
9+
* @tags actions
10+
* security
11+
* external/cwe/cwe-367
12+
*/
13+
14+
import actions
15+
import codeql.actions.security.UntrustedCheckoutQuery
16+
import codeql.actions.security.PoisonableSteps
17+
18+
from ControlCheck check, MutableRefCheckoutStep checkout
19+
where
20+
// the mutable checkout step is protected by an access check
21+
check = [checkout.getIf(), checkout.getEnclosingJob().getIf()] and
22+
// the checked-out code may lead to arbitrary code execution
23+
checkout.getAFollowingStep() instanceof PoisonableStep
24+
select checkout, "The checked-out code can be changed after the authorization check o step $@.",
25+
check, check.toString()

‎ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/**
2+
* @name Untrusted Checkout TOCTOU
3+
* @description Untrusted Checkout is protected by a security check but the checked-out branch can be changed after the check.
4+
* @kind problem
5+
* @problem.severity warning
6+
* @precision medium
7+
* @security-severity 5.3
8+
* @id actions/untrusted-checkout-toctou/high
9+
* @tags actions
10+
* security
11+
* external/cwe/cwe-367
12+
*/
13+
14+
import actions
15+
import codeql.actions.security.UntrustedCheckoutQuery
16+
import codeql.actions.security.PoisonableSteps
17+
18+
from ControlCheck check, MutableRefCheckoutStep checkout
19+
where
20+
// the mutable checkout step is protected by an access check
21+
check = [checkout.getIf(), checkout.getEnclosingJob().getIf()] and
22+
// there are no evidences that the checked-out code can lead to arbitrary code execution
23+
not checkout.getAFollowingStep() instanceof PoisonableStep
24+
select checkout, "The checked-out code can be changed after the authorization check o step $@.",
25+
check, check.toString()

‎ql/test/query-tests/Security/CWE-285/.github/workflows/test1.yml

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
name: Pull request feedback
2+
3+
on:
4+
pull_request_target:
5+
types: [ opened, synchronize ]
6+
7+
permissions: {}
8+
jobs:
9+
test:
10+
permissions:
11+
contents: write
12+
pull-requests: write
13+
runs-on: ubuntu-latest
14+
steps:
15+
- name: Checkout repo for OWNER TEST
16+
uses: actions/checkout@v3
17+
if: contains(github.event.pull_request.labels.*.name, 'safe to test')
18+
with:
19+
ref: ${{ github.event.pull_request.head.ref }}
20+
- run: ./cmd

‎ql/test/query-tests/Security/CWE-285/.github/workflows/test2.yml

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
name: Pull request feedback
2+
3+
on:
4+
pull_request_target:
5+
types: [ labeled ]
6+
7+
permissions: {}
8+
jobs:
9+
test:
10+
permissions:
11+
contents: write
12+
pull-requests: write
13+
runs-on: ubuntu-latest
14+
steps:
15+
- name: Checkout repo for OWNER TEST
16+
uses: actions/checkout@v3
17+
if: contains(github.event.pull_request.labels.*.name, 'safe to test')
18+
with:
19+
ref: ${{ github.event.pull_request.head.ref }}
20+
- run: ./cmd

‎ql/test/query-tests/Security/CWE-285/ImproperAccessControl.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
| .github/workflows/test1.yml:15:7:20:4 | Uses Step | The checked-out code can be changed after the authorization check o step $@. | .github/workflows/test1.yml:17:11:17:75 | contain ... test') | contain ... test') |

‎ql/test/query-tests/Security/CWE-285/ImproperAccessControl.qlref

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Security/CWE-285/ImproperAccessControl.ql
2+

‎ql/test/query-tests/Security/CWE-367/.github/workflows/comment.yml

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
# https://github.com/AdnaneKhan/ActionsTOCTOU/blob/main/.github/workflows/comment_victim.yml
2+
name: Comment Triggered Test
3+
on:
4+
issue_comment:
5+
types: [created]
6+
permissions: 'write-all'
7+
jobs:
8+
benchmark:
9+
name: Integration Tests
10+
if: ${{ github.event.issue.pull_request && contains(fromJson('["MEMBER", "OWNER"]'), github.event.comment.author_association) && startsWith(github.event.comment.body, '/run-tests ') }}
11+
runs-on: [ubuntu-latest]
12+
steps:
13+
14+
# test1
15+
- uses: actions/github-script@v6
16+
name: Get PR branch
17+
id: issue
18+
with:
19+
script: |
20+
const pr = context.payload.issue.number
21+
const data = await github.rest.pulls.get({
22+
owner: context.repo.owner,
23+
repo: context.repo.repo,
24+
pull_number: pr
25+
})
26+
return {
27+
ref: data.data.head.ref,
28+
sha: data.data.head.sha,
29+
}
30+
- uses: actions/checkout@v4
31+
with:
32+
submodules: recursive
33+
ref: ${{ fromJson(steps.issue.outputs.result).sha }}
34+
- run: bash comment_example/tests.sh
35+
36+
# test2
37+
- uses: actions/checkout@v4
38+
with:
39+
submodules: recursive
40+
ref: "refs/pull/${{ github.event.number }}/merge"
41+
- run: bash comment_example/tests.sh

‎ql/test/query-tests/Security/CWE-367/.github/workflows/deployment.yml

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
# https://github.com/AdnaneKhan/ActionsTOCTOU/blob/main/.github/workflows/deployment_victim.yml
2+
name: Environment PR Check
3+
4+
on:
5+
pull_request_target:
6+
branches:
7+
- main
8+
paths:
9+
- 'README.md'
10+
workflow_dispatch:
11+
jobs:
12+
test:
13+
environment: Public CI
14+
runs-on: ubuntu-latest
15+
steps:
16+
- name: Checkout from PR branch
17+
uses: actions/checkout@v4
18+
with:
19+
repository: ${{ github.event.pull_request.head.repo.full_name }}
20+
ref: ${{ github.event.pull_request.head.ref }}
21+
22+
- name: Set Node.js 20.x for GitHub Action
23+
uses: actions/setup-node@v4
24+
with:
25+
node-version: 20.x
26+
27+
- name: installing node_modules
28+
run: cd deployment_example && npm install
29+
30+
- name: Build GitHub Action
31+
run: cd deployment_example && npm run build

‎ql/test/query-tests/Security/CWE-367/.github/workflows/label.yml

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
# https://github.com/AdnaneKhan/ActionsTOCTOU/blob/main/.github/workflows/label_victim.yml
2+
name: Label Trigger Test
3+
on:
4+
pull_request_target:
5+
types: [labeled]
6+
branches: [main]
7+
8+
jobs:
9+
integration-tests:
10+
runs-on: ubuntu-latest
11+
if: contains(github.event.pull_request.labels.*.name, 'safe-to-test')
12+
steps:
13+
- uses: actions/checkout@v4
14+
with:
15+
ref: ${{ github.event.pull_request.head.ref }}
16+
repository: ${{ github.event.pull_request.head.repo.full_name }}
17+
- run: bash label_example/tests.sh

‎ql/test/query-tests/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
| .github/workflows/comment.yml:37:9:41:6 | Uses Step | The checked-out code can be changed after the authorization check o step $@. | .github/workflows/comment.yml:10:9:10:188 | ${{ git ... s ') }} | ${{ git ... s ') }} |
2+
| .github/workflows/label.yml:13:9:17:6 | Uses Step | The checked-out code can be changed after the authorization check o step $@. | .github/workflows/label.yml:11:9:11:73 | contain ... -test') | contain ... -test') |

‎ql/test/query-tests/Security/CWE-367/UntrustedCheckoutTOCTOUCritical.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-367/UntrustedCheckoutTOCTOUCritical.ql

‎ql/test/query-tests/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.expected

Whitespace-only changes.

‎ql/test/query-tests/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql

0 commit comments

Comments
 (0)