Skip to content

Actions: improve improper access control query#20904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
redsun82 wants to merge 1 commit into
base:main
Choose a base branch
from
+32 −4
Open

Actions: improve improper access control query #20904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion actions/ql/src/Security/CWE-285/ImproperAccessControl.ql
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -14,17 +14,19 @@
import codeql.actions.security.UntrustedCheckoutQuery
import codeql.actions.security.ControlChecks

from LocalJob job, LabelCheck check, MutableRefCheckoutStep checkout, Event event
from LocalJob job, LabelCheck check, PRHeadCheckoutStep checkout, Event event
where
job.isPrivileged() and
job.getAStep() = checkout and
check.dominates(checkout) and
(
job.getATriggerEvent() = event and
event.getName() = "pull_request_target" and
event.getAnActivityType() = "synchronize"
or
not exists(job.getATriggerEvent())

Check warning

Code scanning / CodeQL

Var only used in one side of disjunct Warning

The
variable event
Loading
is only used in one side of disjunct.
or
checkout instanceof MutableRefCheckoutStep
)
select checkout, "The checked-out code can be modified after the authorization check $@.", check,
check.toString()
5 changes: 5 additions & 0 deletions actions/ql/src/change-notes/2025-11-25-improve-improper-access-control.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
category: minorAnalysis
---
* The `actions/improper-access-control` query has been improved to correctly detect cases where either the check
triggers or the checkout reference are unsafe, rather than only when both applied as was done previously.
Copy link

CopilotAINov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar issue: "when both applied" should be "when both apply" or "when both were applied".

Suggested change
triggers or the checkout reference are unsafe, rather than only when both applied as was done previously.
triggers or the checkout reference are unsafe, rather than only when both apply as was done previously.

Copilot uses AI. Check for mistakes.
2 changes: 1 addition & 1 deletion ...urity/CWE-285/.github/workflows/test2.yml → ...WE-285/.github/workflows/bad_checkout.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -16,5 +16,5 @@ jobs:
uses: actions/checkout@v3
if: contains(github.event.pull_request.labels.*.name, 'safe to test')
with:
ref: ${{github.event.pull_request.head.ref }}
ref: ${{github.event.pull_request.head.ref }} # BAD (mutable ref)
Copy link
Contributor

@AdnaneKhanAdnaneKhanNov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically head.ref without a repo name isn’t exploitable (as it will just check out the branch from the base repo a fail if not present).

Copy link
Contributor

@AdnaneKhanAdnaneKhanNov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it still could be a privesc in very narrow scenarios where the repo enforces strict feature/main branch boundaries and a higher privilege group needs to approve the run (even though both have write access to the repo).

- run: ./cmd
2 changes: 1 addition & 1 deletion ...urity/CWE-285/.github/workflows/test1.yml → ...WE-285/.github/workflows/bad_triggers.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -16,5 +16,5 @@ jobs:
uses: actions/checkout@v3
if: contains(github.event.pull_request.labels.*.name, 'safe to test')
with:
ref: ${{github.event.pull_request.head.ref }}
ref: ${{github.event.pull_request.head.sha }} # BAD (bad trigger)
- run: ./cmd
20 changes: 20 additions & 0 deletions actions/ql/test/query-tests/Security/CWE-285/.github/workflows/good.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
name: Pull request feedback

on:
pull_request_target:
types: [ labeled ]

permissions:{}
jobs:
test:
permissions:
contents: write
pull-requests: write
runs-on: ubuntu-latest
steps:
- name: Checkout repo for OWNER TEST
uses: actions/checkout@v3
if: contains(github.event.pull_request.labels.*.name, 'safe to test')
with:
ref: ${{github.event.pull_request.head.sha }} # GOOD (labeled event + immutable ref)
- run: ./cmd
3 changes: 2 additions & 1 deletion actions/ql/test/query-tests/Security/CWE-285/ImproperAccessControl.expected
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
| .github/workflows/test1.yml:15:7:20:4 | Uses Step | The checked-out code can be modified after the authorization check $@. | .github/workflows/test1.yml:17:11:17:75 | contain ... test') | contain ... test') |
| .github/workflows/bad_checkout.yml:15:7:20:4 | Uses Step | The checked-out code can be modified after the authorization check $@. | .github/workflows/bad_checkout.yml:17:11:17:75 | contain ... test') | contain ... test') |
| .github/workflows/bad_triggers.yml:15:7:20:4 | Uses Step | The checked-out code can be modified after the authorization check $@. | .github/workflows/bad_triggers.yml:17:11:17:75 | contain ... test') | contain ... test') |