Skip to content

Commit 3120d4b

Browse files
author
Alvaro Muñoz
committed
2 parents ca59423 + 33b3fc6 commit 3120d4b

20 files changed

+336
-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/lib/ext/peter-murray_issue-body-parser-action.model.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
extensions:
2+
- addsTo:
3+
pack: githubsecuritylab/actions-all
4+
extensible: sourceModel
5+
data:
6+
- ["peter-murray/issue-body-parser-action", "*", "output.*", "text", "manual"]

‎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/library-tests/test.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,7 @@ sources
464464
| jitterbit/get-changed-files | * | output.renamed | filename | manual |
465465
| khan/pull-request-comment-trigger | * | output.comment_body | text | manual |
466466
| marocchino/on_artifact | * | output.* | artifact | manual |
467+
| peter-murray/issue-body-parser-action | * | output.* | text | manual |
467468
| puppeteer/puppeteer/.github/workflows/changed-packages.yml | * | output.changes | filename | manual |
468469
| redhat-plumbers-in-action/download-artifact | * | output.* | artifact | manual |
469470
| tj-actions/branch-names | * | output.current_branch | branch | manual |

‎ql/test/query-tests/Security/CWE-094/.github/workflows/test3.yml

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
name: Approve or Deny Marketplace Action Request
2+
3+
on:
4+
issue_comment:
5+
types: [created]
6+
7+
jobs:
8+
parse-issue:
9+
runs-on: self-hosted
10+
outputs:
11+
payload: ${{ steps.issue_body_parser_request.outputs.payload }}
12+
steps:
13+
- name: Get JSON Data out of Issue Request
14+
uses: peter-murray/issue-body-parser-action@v2
15+
id: issue_body_parser_request
16+
with:
17+
github_token: ${{ secrets.GITHUB_TOKEN }}
18+
issue_id: ${{ github.event.issue.number }}
19+
payload_marker: request
20+
fail_on_missing: false
21+
approve-or-deny-request:
22+
runs-on: self-hosted
23+
needs: parse-issue
24+
if: needs.parse-issue.outputs.payload != 'NOT_FOUND'
25+
steps:
26+
- name: Lookup the latest release of ${{ fromJson(needs.parse-issue.outputs.payload).owner }}/${{ fromJson(needs.parse-issue.outputs.payload).repo }}
27+
id: get_version
28+
env:
29+
OWNER: ${{ fromJson(needs.parse-issue.outputs.payload).owner }}
30+
REPO: ${{ fromJson(needs.parse-issue.outputs.payload).repo }}
31+
REQUEST_VERSION: ${{ fromJson(needs.parse-issue.outputs.payload).version }}
32+
run: |
33+
if [ $REQUEST_VERSION == 'latest' ]; then
34+
echo "Finding latest release of $OWNER/$REPO..."
35+
export VERSION=`curl https://api.github.com/repos/$OWNER/$REPO/releases/latest | jq -r .name`
36+
else
37+
export VERSION=$REQUEST_VERSION
38+
fi
39+
echo "VERSION: $VERSION"
40+
echo "version=$VERSION" >> $GITHUB_OUTPUT
41+
- name: Check out scripts
42+
uses: actions/checkout@v3
43+
- name: Setup Node
44+
uses: actions/setup-node@v3
45+
with:
46+
node-version: '14'
47+
check-latest: true
48+
- name: Install dependencies
49+
run: |
50+
cd .github/scripts
51+
npm install
52+
- name: Approve or deny request
53+
uses: actions/github-script@main
54+
env:
55+
VERSION: ${{ steps.get_version.outputs.version }}
56+
with:
57+
debug: true
58+
script: |
59+
const options = { token: '${{ secrets.TOKEN }}', adminOpsOrg: '${{ vars.ADMIN_OPS_ORG }}', actionsApprovedOrg: '${{ vars.ACTIONS_APPROVED_ORG }}', actionsApproverTeam: '${{ vars.ACTIONS_APPROVERS_TEAM }}', baseUrl: '${{ github.api_url }}', version: process.env.VERSION };
60+
const payload = ${{ needs.parse-issue.outputs.payload }}
61+
await require('./.github/scripts/approve-or-deny-request.js')({github, context, payload, options});

‎ql/test/query-tests/Security/CWE-094/CodeInjection.expected

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ edges
5959
| .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY |
6060
| .github/workflows/test2.yml:17:9:25:6 | Uses Step: changed | .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files |
6161
| .github/workflows/test2.yml:29:9:37:6 | Uses Step: changed2 | .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files |
62+
| .github/workflows/test3.yml:11:7:12:4 | Job outputs node [payload] | .github/workflows/test3.yml:60:27:60:66 | needs.parse-issue.outputs.payload |
63+
| .github/workflows/test3.yml:11:17:11:70 | steps.issue_body_parser_request.outputs.payload | .github/workflows/test3.yml:11:7:12:4 | Job outputs node [payload] |
64+
| .github/workflows/test3.yml:13:9:21:2 | Uses Step: issue_body_parser_request | .github/workflows/test3.yml:11:17:11:70 | steps.issue_body_parser_request.outputs.payload |
6265
| .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] |
6366
| .github/workflows/test.yml:8:20:8:50 | steps.step5.outputs.MSG5 | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] |
6467
| .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | .github/workflows/test.yml:20:18:20:48 | steps.step0.outputs.value |
@@ -222,6 +225,10 @@ nodes
222225
| .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | semmle.label | steps.changed.outputs.locale_files |
223226
| .github/workflows/test2.yml:29:9:37:6 | Uses Step: changed2 | semmle.label | Uses Step: changed2 |
224227
| .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | semmle.label | steps.changed2.outputs.locale_files |
228+
| .github/workflows/test3.yml:11:7:12:4 | Job outputs node [payload] | semmle.label | Job outputs node [payload] |
229+
| .github/workflows/test3.yml:11:17:11:70 | steps.issue_body_parser_request.outputs.payload | semmle.label | steps.issue_body_parser_request.outputs.payload |
230+
| .github/workflows/test3.yml:13:9:21:2 | Uses Step: issue_body_parser_request | semmle.label | Uses Step: issue_body_parser_request |
231+
| .github/workflows/test3.yml:60:27:60:66 | needs.parse-issue.outputs.payload | semmle.label | needs.parse-issue.outputs.payload |
225232
| .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] |
226233
| .github/workflows/test.yml:8:20:8:50 | steps.step5.outputs.MSG5 | semmle.label | steps.step5.outputs.MSG5 |
227234
| .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | semmle.label | Uses Step: step0 [value] |

‎ql/test/query-tests/Security/CWE-094/PrivilegedCodeInjection.expected

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ edges
5959
| .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY |
6060
| .github/workflows/test2.yml:17:9:25:6 | Uses Step: changed | .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files |
6161
| .github/workflows/test2.yml:29:9:37:6 | Uses Step: changed2 | .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files |
62+
| .github/workflows/test3.yml:11:7:12:4 | Job outputs node [payload] | .github/workflows/test3.yml:60:27:60:66 | needs.parse-issue.outputs.payload |
63+
| .github/workflows/test3.yml:11:17:11:70 | steps.issue_body_parser_request.outputs.payload | .github/workflows/test3.yml:11:7:12:4 | Job outputs node [payload] |
64+
| .github/workflows/test3.yml:13:9:21:2 | Uses Step: issue_body_parser_request | .github/workflows/test3.yml:11:17:11:70 | steps.issue_body_parser_request.outputs.payload |
6265
| .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] |
6366
| .github/workflows/test.yml:8:20:8:50 | steps.step5.outputs.MSG5 | .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] |
6467
| .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | .github/workflows/test.yml:20:18:20:48 | steps.step0.outputs.value |
@@ -222,6 +225,10 @@ nodes
222225
| .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | semmle.label | steps.changed.outputs.locale_files |
223226
| .github/workflows/test2.yml:29:9:37:6 | Uses Step: changed2 | semmle.label | Uses Step: changed2 |
224227
| .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | semmle.label | steps.changed2.outputs.locale_files |
228+
| .github/workflows/test3.yml:11:7:12:4 | Job outputs node [payload] | semmle.label | Job outputs node [payload] |
229+
| .github/workflows/test3.yml:11:17:11:70 | steps.issue_body_parser_request.outputs.payload | semmle.label | steps.issue_body_parser_request.outputs.payload |
230+
| .github/workflows/test3.yml:13:9:21:2 | Uses Step: issue_body_parser_request | semmle.label | Uses Step: issue_body_parser_request |
231+
| .github/workflows/test3.yml:60:27:60:66 | needs.parse-issue.outputs.payload | semmle.label | needs.parse-issue.outputs.payload |
225232
| .github/workflows/test.yml:8:7:10:4 | Job outputs node [job_output] | semmle.label | Job outputs node [job_output] |
226233
| .github/workflows/test.yml:8:20:8:50 | steps.step5.outputs.MSG5 | semmle.label | steps.step5.outputs.MSG5 |
227234
| .github/workflows/test.yml:12:9:18:6 | Uses Step: step0 [value] | semmle.label | Uses Step: step0 [value] |
@@ -333,6 +340,7 @@ subpaths
333340
| .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY | .github/workflows/test1.yml:22:38:22:75 | github.event.pull_request.title | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/test1.yml:25:20:25:39 | env.ISSUE_KEY | ${{ env.ISSUE_KEY }} |
334341
| .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | .github/workflows/test2.yml:17:9:25:6 | Uses Step: changed | .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/test2.yml:27:26:27:66 | steps.changed.outputs.locale_files | ${{ steps.changed.outputs.locale_files }} |
335342
| .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | .github/workflows/test2.yml:29:9:37:6 | Uses Step: changed2 | .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/test2.yml:39:25:39:66 | steps.changed2.outputs.locale_files | ${{ steps.changed2.outputs.locale_files }} |
343+
| .github/workflows/test3.yml:60:27:60:66 | needs.parse-issue.outputs.payload | .github/workflows/test3.yml:13:9:21:2 | Uses Step: issue_body_parser_request | .github/workflows/test3.yml:60:27:60:66 | needs.parse-issue.outputs.payload | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/test3.yml:60:27:60:66 | needs.parse-issue.outputs.payload | ${{ needs.parse-issue.outputs.payload }} |
336344
| .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | .github/workflows/test.yml:15:20:15:64 | github.event['head_commit']['message'] | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/test.yml:49:20:49:56 | needs.job1.outputs['job_output'] | ${{needs.job1.outputs['job_output']}} |
337345
| .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:9:19:9:64 | github.event.workflow_run.display_title | ${{ github.event.workflow_run.display_title }} |
338346
| .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | Potential privileged code injection in $@, which may be controlled by an external user. | .github/workflows/workflow_run.yml:10:19:10:70 | github.event.workflow_run.head_commit.message | ${{ github.event.workflow_run.head_commit.message }} |

‎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

0 commit comments

Comments
 (0)