diff --git a/actions/ql/src/Security/CWE-285/ImproperAccessControl.ql b/actions/ql/src/Security/CWE-285/ImproperAccessControl.ql index ba002f16a874..b8261fea4b62 100644 --- a/actions/ql/src/Security/CWE-285/ImproperAccessControl.ql +++ b/actions/ql/src/Security/CWE-285/ImproperAccessControl.ql @@ -14,7 +14,7 @@ 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 @@ -25,6 +25,8 @@ where event.getAnActivityType() = "synchronize" or not exists(job.getATriggerEvent()) + or + checkout instanceof MutableRefCheckoutStep ) select checkout, "The checked-out code can be modified after the authorization check $@.", check, check.toString() diff --git a/actions/ql/src/change-notes/2025-11-25-improve-improper-access-control.md b/actions/ql/src/change-notes/2025-11-25-improve-improper-access-control.md new file mode 100644 index 000000000000..78c61fdede51 --- /dev/null +++ b/actions/ql/src/change-notes/2025-11-25-improve-improper-access-control.md @@ -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. diff --git a/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/test2.yml b/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/bad_checkout.yml similarity index 83% rename from actions/ql/test/query-tests/Security/CWE-285/.github/workflows/test2.yml rename to actions/ql/test/query-tests/Security/CWE-285/.github/workflows/bad_checkout.yml index be6a6cf39395..4be4b405d055 100644 --- a/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/test2.yml +++ b/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/bad_checkout.yml @@ -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) - run: ./cmd diff --git a/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/test1.yml b/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/bad_triggers.yml similarity index 84% rename from actions/ql/test/query-tests/Security/CWE-285/.github/workflows/test1.yml rename to actions/ql/test/query-tests/Security/CWE-285/.github/workflows/bad_triggers.yml index 48833460b44b..a59fa20c6e5b 100644 --- a/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/test1.yml +++ b/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/bad_triggers.yml @@ -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 diff --git a/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/good.yml b/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/good.yml new file mode 100644 index 000000000000..a29995d61083 --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-285/.github/workflows/good.yml @@ -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 diff --git a/actions/ql/test/query-tests/Security/CWE-285/ImproperAccessControl.expected b/actions/ql/test/query-tests/Security/CWE-285/ImproperAccessControl.expected index 92f87dc1f35b..c167b28de1e7 100644 --- a/actions/ql/test/query-tests/Security/CWE-285/ImproperAccessControl.expected +++ b/actions/ql/test/query-tests/Security/CWE-285/ImproperAccessControl.expected @@ -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') |