[Actions] CWE-285/ImproperAccessControl.ql Not Currently Working

[AdnaneKhan]

Description of the issue

The Actions ImproperAccessControl query is not working even for trivial workflows. This is an example from https://github.com/github/codeql/blob/main/actions/ql/src/Security/CWE-285/ImproperAccessControl.md and does not trigger a detection.

on:
  pull_request_target:
    types: [opened, synchronize]

jobs:
  test:
    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 }}
      - run: ./cmd

All my attempts to try variations that did trigger a finding also failed. Is this detection enabled as part of the default suite (it appears to be)?

[rvermeulen]

Hi @AdnaneKhan,

Thanks for your question. It seems that our query only checks the mutable reference case, while you are using the immutable reference, but without to the correct triggers. I will reach out to the team about your case. Stay tuned!

[AdnaneKhan]

Hi @AdnaneKhan,

Thanks for your question. It seems that our query only checks the mutable reference case, while you are using the immutable reference, but without to the correct triggers. I will reach out to the team about your case. Stay tuned!

Thanks! The expectation I have for this one is to be focused on the fact that the label persists (so this query should only fire when synchronize is present and the label check is contains vs. the label being added as part of the event). The mutable reference with single trigger case is covered by the separate TOCTOU query.

[redsun82]

👋 @AdnaneKhan #20904 should make the query better, and detects your case. Many thanks for your report! ❤️

[AdnaneKhan]

👋 @AdnaneKhan #20904 should make the query better, and detects your case. Many thanks for your report! ❤️

Thank you 🎉 A CodeQL run a day keeps the NPM worms away.