Skip to content

Commit e631196

Browse files
author
Alvaro Muñoz
committed
Take explicit permission into account for privilege calculation
1 parent 1fd7c14 commit e631196

File tree

5 files changed

+118
-5
lines changed

5 files changed

+118
-5
lines changed

‎ql/lib/codeql/actions/ast/internal/Ast.qll

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -848,11 +848,23 @@ class JobImpl extends AstNodeImpl, TJobNode {
848848
this.getPermissions().getAPermission().matches("%write")
849849
}
850850

851+
private predicate hasExplicitReadPermission() {
852+
// the job has not an explicit write permission
853+
exists(this.getPermissions().getAPermission()) and
854+
not this.getPermissions().getAPermission().matches("%write")
855+
}
856+
851857
private predicate hasImplicitWritePermission() {
852858
// the job has an explicit write permission
853859
this.getEnclosingWorkflow().getPermissions().getAPermission().matches("%write")
854860
}
855861

862+
private predicate hasImplicitReadPermission() {
863+
// the job has not an explicit write permission
864+
exists(this.getEnclosingWorkflow().getPermissions().getAPermission()) and
865+
not this.getEnclosingWorkflow().getPermissions().getAPermission().matches("%write")
866+
}
867+
856868
private predicate hasRuntimeData() {
857869
exists(string path, string trigger, string name, string secrets_source, string perms |
858870
workflowDataModel(path, trigger, name, secrets_source, perms, _) and
@@ -892,8 +904,7 @@ class JobImpl extends AstNodeImpl, TJobNode {
892904

893905
/** Holds if the action is privileged and externally triggerable. */
894906
predicate isPrivilegedExternallyTriggerable() {
895-
exists(EventImpl e |
896-
this.getATriggerEvent() = e and
907+
exists(EventImpl e | this.getATriggerEvent() = e |
897908
// job is triggereable by an external user
898909
e.isExternallyTriggerable() and
899910
// no matter if `pull_request` is granted write permissions or access to secrets
@@ -903,9 +914,18 @@ class JobImpl extends AstNodeImpl, TJobNode {
903914
// job is privileged (write access or access to secrets)
904915
this.isPrivileged()
905916
or
906-
// the trigger event is __normally__ privileged and we have no runtime data to prove otherwise
917+
// the trigger event is __normally__ privileged
918+
e.isPrivileged() and
919+
// and we have no runtime data to prove otherwise
907920
not this.hasRuntimeData() and
908-
e.isPrivileged()
921+
// and the job is not explicitly non-privileged
922+
not (
923+
(
924+
this.hasExplicitReadPermission() or
925+
this.hasImplicitReadPermission()
926+
) and
927+
not this.hasExplicitSecretAccess()
928+
)
909929
)
910930
)
911931
}

‎ql/test/query-tests/Security/CWE-349/.github/workflows/test20.yml

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
name: Publish
2+
3+
on:
4+
push:
5+
branches:
6+
- main
7+
pull_request_target:
8+
workflow_dispatch:
9+
workflow_call:
10+
11+
jobs:
12+
build-and-upload:
13+
runs-on: ubuntu-latest
14+
permissions:
15+
contents: read
16+
steps:
17+
18+
- name: Checkout PR
19+
if: ${{ github.event_name == 'pull_request_target' }}
20+
uses: actions/checkout@v3
21+
with:
22+
ref: ${{ github.event.pull_request.head.ref }}
23+
repository: ${{ github.event.pull_request.head.repo.full_name }}
24+
25+
- name: Checkout
26+
if: ${{ github.event_name != 'pull_request_target' }}
27+
uses: actions/checkout@v3
28+
with:
29+
ref: main
30+
31+
- name: Setup Pages
32+
uses: actions/configure-pages@v1
33+
- name: Use Node.js
34+
uses: actions/setup-node@v3
35+
with:
36+
node-version: 18
37+
cache: npm
38+
- name: Update npm to latest
39+
run: npm i --prefer-online --no-fund --no-audit -g npm@latest
40+
- run: npm -v
41+
- run: npm i --ignore-scripts --no-audit --no-fund --package-lock
42+
- run: npm run build -w www
43+
- name: Upload artifact
44+
uses: actions/upload-pages-artifact@v1
45+
with:
46+
path: './workspaces/www/build'

‎ql/test/query-tests/Security/CWE-829/.github/workflows/test4.yml

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
name: Publish
2+
3+
on:
4+
push:
5+
branches:
6+
- main
7+
pull_request_target:
8+
workflow_dispatch:
9+
workflow_call:
10+
11+
jobs:
12+
build-and-upload:
13+
runs-on: ubuntu-latest
14+
permissions:
15+
contents: read
16+
steps:
17+
18+
- name: Checkout PR
19+
if: ${{ github.event_name == 'pull_request_target' }}
20+
uses: actions/checkout@v3
21+
with:
22+
ref: ${{ github.event.pull_request.head.ref }}
23+
repository: ${{ github.event.pull_request.head.repo.full_name }}
24+
25+
- name: Checkout
26+
if: ${{ github.event_name != 'pull_request_target' }}
27+
uses: actions/checkout@v3
28+
with:
29+
ref: main
30+
31+
- name: Setup Pages
32+
uses: actions/configure-pages@v1
33+
- name: Use Node.js
34+
uses: actions/setup-node@v3
35+
with:
36+
node-version: 18
37+
cache: npm
38+
- name: Update npm to latest
39+
run: npm i --prefer-online --no-fund --no-audit -g npm@latest
40+
- run: npm -v
41+
- run: npm i --ignore-scripts --no-audit --no-fund --package-lock
42+
- run: npm run build -w www
43+
- name: Upload artifact
44+
uses: actions/upload-pages-artifact@v1
45+
with:
46+
path: './workspaces/www/build'

‎ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,5 @@
44
| .github/workflows/level0.yml:99:9:103:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
55
| .github/workflows/level0.yml:125:9:129:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
66
| .github/workflows/mend.yml:22:9:29:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
7-
| .github/workflows/test3.yml:28:9:33:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
87
| .github/workflows/untrusted_checkout.yml:10:9:13:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
98
| .github/workflows/untrusted_checkout.yml:13:9:16:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |

‎ql/test/query-tests/Security/CWE-829/UntrustedCheckoutMedium.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
11
| .github/workflows/dependabot1.yml:15:9:19:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
22
| .github/workflows/dependabot1.yml:39:9:43:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
33
| .github/workflows/priv_pull_request_checkout.yml:14:9:20:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
4+
| .github/workflows/test3.yml:28:9:33:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
5+
| .github/workflows/test4.yml:18:7:25:4 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |

0 commit comments

Comments
 (0)