diff --git a/actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md b/actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md new file mode 100644 index 000000000000..4397e9487415 --- /dev/null +++ b/actions/ql/lib/change-notes/2026-08-17-actor-if-check-event-validity.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Checks on actor fields read from the event payload (e.g. `github.event.pull_request.user.login`) now only count as protection for events whose payload actually populates that field. These checks were split out of `ActorIfCheck` into a new class `EventActorIfCheck`, and `ActorIfCheck` now only covers `github.actor` and `github.triggering_actor`. Previously, a condition such as `github.event.pull_request.user.login != 'name'` on a workflow triggered by `issues` events was treated as a protective check even though `github.event.pull_request` is not populated for `issues` events, which makes the condition vacuous. This change will result in more results being found by the queries that rely on control checks, such as `actions/code-injection/critical`. diff --git a/actions/ql/lib/codeql/actions/security/ControlChecks.qll b/actions/ql/lib/codeql/actions/security/ControlChecks.qll index 2228e9f96e21..675c1d18852b 100644 --- a/actions/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/actions/ql/lib/codeql/actions/security/ControlChecks.qll @@ -314,17 +314,6 @@ class LabelIfCheck extends LabelCheck instanceof If { class ActorIfCheck extends ActorCheck instanceof If { ActorIfCheck() { - // eg: github.event.pull_request.user.login == 'admin' - exists( - normalizeExpr(this.getCondition()) - .regexpFind([ - "\\bgithub\\.event\\.pull_request\\.user\\.login\\b", - "\\bgithub\\.event\\.head_commit\\.author\\.name\\b", - "\\bgithub\\.event\\.commits.*\\.author\\.name\\b", - "\\bgithub\\.event\\.sender\\.login\\b" - ], _, _) - ) - or // eg: github.actor == 'admin' // eg: github.triggering_actor == 'admin' exists( @@ -335,6 +324,51 @@ class ActorIfCheck extends ActorCheck instanceof If { } } +/** + * Gets a regular expression matching a condition on an actor field that is + * only populated for events whose payload contains the `context_prefix` context. + */ +private string eventPayloadActorFieldRegex(string context_prefix) { + context_prefix = "github.event.pull_request" and + result = "\\bgithub\\.event\\.pull_request\\.user\\.login\\b" + or + context_prefix = "github.event.head_commit" and + result = "\\bgithub\\.event\\.head_commit\\.author\\.name\\b" + or + context_prefix = "github.event.commits" and + result = "\\bgithub\\.event\\.commits.*\\.author\\.name\\b" + or + context_prefix = "github.event.sender" and + result = "\\bgithub\\.event\\.sender\\.login\\b" +} + +/** An If node that checks an actor field from the event payload */ +class EventActorIfCheck extends ActorCheck instanceof If { + string context_prefix; + + EventActorIfCheck() { + // eg: github.event.pull_request.user.login == 'admin' + exists( + normalizeExpr(this.getCondition()) + .regexpFind(eventPayloadActorFieldRegex(context_prefix), _, _) + ) + } + + override predicate protectsCategoryAndEvent(string category, string event) { + ActorCheck.super.protectsCategoryAndEvent(category, event) and + ( + // the `sender` object is part of every webhook event payload + context_prefix = "github.event.sender" + or + // other actor fields only restrict events whose payload populates them. + // eg: `github.event.pull_request.user.login` cannot restrict the actor + // of an `issues` event since `github.event.pull_request` is not + // populated there, which makes the condition vacuous + contextTriggerDataModel(event, context_prefix) + ) + } +} + class PullRequestTargetRepositoryIfCheck extends RepositoryCheck instanceof If { PullRequestTargetRepositoryIfCheck() { // eg: github.event.pull_request.head.repo.full_name == github.repository diff --git a/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_valid_event.yml b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_valid_event.yml new file mode 100644 index 000000000000..cd139700f078 --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_valid_event.yml @@ -0,0 +1,12 @@ +on: + pull_request_target: + types: [opened] + +jobs: + # The `if:` condition checks an actor field that is populated for + # `pull_request_target` events, so the injectable step is protected. + valid-actor-check: + runs-on: ubuntu-latest + if: github.event.pull_request.user.login == 'trusted-user' + steps: + - run: echo '${{ github.event.pull_request.title }}' diff --git a/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml new file mode 100644 index 000000000000..982956b3d953 --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-094/.github/workflows/actor_check_wrong_event.yml @@ -0,0 +1,46 @@ +on: + issues: + types: [opened] + +jobs: + # The `if:` condition compares an actor field that is never populated for + # `issues` events, so it is always true and does not protect the injectable step. + vacuous-actor-check: + runs-on: ubuntu-latest + if: github.event.pull_request.user.login != 'some-bot[bot]' + steps: + - run: echo '${{ github.event.issue.title }}' + + # The `if:` condition checks an actor context that is populated for every + # event, so the injectable step is protected. + valid-actor-check: + runs-on: ubuntu-latest + if: github.actor == 'trusted-user' + steps: + - run: echo '${{ github.event.issue.title }}' + + # The `sender` object is part of every webhook event payload, so this + # check is effective and the injectable step is protected. + valid-sender-check: + runs-on: ubuntu-latest + if: github.event.sender.login == 'trusted-user' + steps: + - run: echo '${{ github.event.issue.title }}' + + # `github.event.head_commit` is only populated for `push` events, so this + # condition is always true for `issues` events and does not protect the + # injectable step. + vacuous-head-commit-check: + runs-on: ubuntu-latest + if: github.event.head_commit.author.name != 'some-bot' + steps: + - run: echo '${{ github.event.issue.title }}' + + # `github.event.commits` is only populated for `push` events, so this + # condition is always true for `issues` events and does not protect the + # injectable step. + vacuous-commits-check: + runs-on: ubuntu-latest + if: github.event.commits[0].author.name != 'some-bot' + steps: + - run: echo '${{ github.event.issue.title }}' diff --git a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected index 02f7f68c05f4..14e50942d734 100644 --- a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected +++ b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionCritical.expected @@ -270,6 +270,12 @@ nodes | .github/actions/external/ultralytics/actions/action.yaml:66:3:66:6 | input body | semmle.label | input body | | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | semmle.label | inputs.body | | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | semmle.label | github.head_ref \|\| github.ref | +| .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | +| .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] | | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE | @@ -710,6 +716,9 @@ subpaths | .github/actions/external/TestOrg/TestRepo/.github/actions/clone-repo/action.yaml:22:19:22:37 | inputs.title | .github/workflows/composite-action-caller-4.yml:14:19:14:56 | github.event.pull_request.title | .github/actions/external/TestOrg/TestRepo/.github/actions/clone-repo/action.yaml:22:19:22:37 | inputs.title | Potential code injection in $@, which may be controlled by an external user ($@). | .github/actions/external/TestOrg/TestRepo/.github/actions/clone-repo/action.yaml:22:19:22:37 | inputs.title | ${{ inputs.title }} | .github/workflows/composite-action-caller-4.yml:4:3:4:21 | pull_request_target | pull_request_target | | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | .github/workflows/test29.yml:35:18:35:54 | github.event.pull_request.body | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | Potential code injection in $@, which may be controlled by an external user ($@). | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | ${{ inputs.body }} | .github/workflows/test29.yml:12:3:12:21 | pull_request_target | pull_request_target | | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | Potential code injection in $@, which may be controlled by an external user ($@). | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | ${{ github.head_ref \|\| github.ref }} | .github/workflows/test29.yml:12:3:12:21 | pull_request_target | pull_request_target | +| .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | ${{ github.event.issue.title }} | .github/workflows/actor_check_wrong_event.yml:2:3:2:8 | issues | issues | +| .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | ${{ github.event.issue.title }} | .github/workflows/actor_check_wrong_event.yml:2:3:2:8 | issues | issues | +| .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | ${{ github.event.issue.title }} | .github/workflows/actor_check_wrong_event.yml:2:3:2:8 | issues | issues | | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | ${{steps.remove_quotations.outputs.replaced}} | .github/workflows/argus_case_study.yml:4:3:4:8 | issues | issues | | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | .github/workflows/artifactpoisoning1.yml:14:9:20:6 | Uses Step | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning1.yml:27:67:27:92 | steps.pr.outputs.id | ${{ steps.pr.outputs.id }} | .github/workflows/artifactpoisoning1.yml:4:3:4:14 | workflow_run | workflow_run | | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | .github/workflows/artifactpoisoning2.yml:13:9:19:6 | Uses Step: pr | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | Potential code injection in $@, which may be controlled by an external user ($@). | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | ${{ steps.pr.outputs.id }} | .github/workflows/artifactpoisoning2.yml:4:3:4:14 | workflow_run | workflow_run | diff --git a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected index 231d41bc2518..f10a7a55c049 100644 --- a/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected +++ b/actions/ql/test/query-tests/Security/CWE-094/CodeInjectionMedium.expected @@ -270,6 +270,12 @@ nodes | .github/actions/external/ultralytics/actions/action.yaml:66:3:66:6 | input body | semmle.label | input body | | .github/actions/external/ultralytics/actions/action.yaml:96:16:96:33 | inputs.body | semmle.label | inputs.body | | .github/actions/external/ultralytics/actions/action.yaml:223:25:223:60 | github.head_ref \|\| github.ref | semmle.label | github.head_ref \|\| github.ref | +| .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | semmle.label | github.event.pull_request.title | +| .github/workflows/actor_check_wrong_event.yml:12:21:12:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:37:21:37:51 | github.event.issue.title | semmle.label | github.event.issue.title | +| .github/workflows/actor_check_wrong_event.yml:46:21:46:51 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] | semmle.label | Uses Step: remove_quotations [replaced] | | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | semmle.label | github.event.issue.title | | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | semmle.label | env.ISSUE_TITLE | @@ -709,6 +715,9 @@ subpaths | .github/actions/action7/action.yml:214:41:214:69 | inputs.github_username | .github/actions/action7/action.yml:214:41:214:69 | inputs.github_username | .github/actions/action7/action.yml:214:41:214:69 | inputs.github_username | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action7/action.yml:214:41:214:69 | inputs.github_username | ${{ inputs.github_username }} | | .github/actions/action7/action.yml:215:41:215:66 | inputs.github_email | .github/actions/action7/action.yml:215:41:215:66 | inputs.github_email | .github/actions/action7/action.yml:215:41:215:66 | inputs.github_email | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action7/action.yml:215:41:215:66 | inputs.github_email | ${{ inputs.github_email }} | | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | Potential code injection in $@, which may be controlled by an external user. | .github/actions/action7/action.yml:217:25:217:60 | github.head_ref \|\| github.ref | ${{ github.head_ref \|\| github.ref }} | +| .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/actor_check_valid_event.yml:12:21:12:58 | github.event.pull_request.title | ${{ github.event.pull_request.title }} | +| .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/actor_check_wrong_event.yml:20:21:20:51 | github.event.issue.title | ${{ github.event.issue.title }} | +| .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/actor_check_wrong_event.yml:28:21:28:51 | github.event.issue.title | ${{ github.event.issue.title }} | | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | ${{ steps.changed-files1.outputs.all_changed_files }} | | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | ${{ steps.changed-files3.outputs.all_changed_files }} | | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | .github/workflows/changed-files.yml:53:9:56:6 | Uses Step: changed-files5 | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:58:24:58:76 | steps.changed-files5.outputs.all_changed_files | ${{ steps.changed-files5.outputs.all_changed_files }} |