✨ Dangerous-Workflow: Check for workflow_run trigger

[raghavkaul]

What kind of change does this PR introduce?

Check for privileged usage of on: workflow_run trigger in GitHub Workflows.

• Privileged: A workflow that uses this trigger contains an action that uses secrets.GITHUB_TOKEN

Which issue(s) this PR fixes

Fixes #3861.

Special notes for your reviewer

The largest function, checkUsesGithubToken checks if secrets.GITHUB_TOKEN is passed to another workflow. There are a lot of nil checks. Another approach is to look through the GitHub Workflow AST, but I wasn't sure if this was possible with actionlint.

[codecov]

Codecov Report

Attention: Patch coverage is 79.87421% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 70.76%. Comparing base (54690b4) to head (257f45d).
Report is 88 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3892      +/-   ##
==========================================
- Coverage   75.50%   70.76%   -4.75%     
==========================================
  Files         231      232       +1     
  Lines       15669    15825     +156     
==========================================
- Hits        11831    11198     -633     
- Misses       3109     3940     +831     
+ Partials      729      687      -42     

[raghavkaul]

/scdiff generate Dangerous-Workflow

[github-actions]

Here's a link to the scdiff run

[laurentsimon]

Couple of comments:

1. Do we care that a GITHUB_TOKEN is referenced? Actions called in a workflow can reference the token themselves. So I would say it does not matter if the token if referenced in the workflow or not. What matters more is whether the token has a dangerous permissions - right now we're not validating this in the Dangerous-Workflow for simplicity, but we could
2. iiuc, the solution to pull_request_target in https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ would be detected as a vuln with the changes in this PR, but should not

I think what we're missing is capturing the context #3861., which is that one of the workflow referenced in workflows: [Run Tests, Another] must be running with a lower privilege, eg not on a PR. There is also an assumption that the workflow_run workflow uses non-sanitized inputs from the less-privileged first workflow: This may be one of the github.* variables (already covered by the check), a checkout + run (already covered by the check), an artifact download (not covered by the check yet). So maybe we should start by detecting artifact download + first workflow not running on PR - and possible check if the workflow_run is privileged, ie dangerous permission is used (package, etc) or used of any secrets?

something unclear to me: does workflow_run accept a branch option? I don't see it documented in https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run

[pnacht]

something unclear to me: does workflow_run accept a branch option? I don't see it documented in https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#limiting-your-workflow-to-run-based-on-branches

[raghavkaul]

Ah, I assumed GITHUB_TOKEN had to be passed explicitly to be available. Yes, it's possible for there to be false positives. Is there another indicator of "sensitive workflow" here? And also, if the triggered workflow is restricted to branch: [<default>] as in Pedro's link, then it's not vulnerable to this attack.

one of the github.* variables (already covered by the check)

We only check for secrets.GITHUB_TOKEN being passed, why would passing github.* be unsafe?

a checkout + run (already covered by the check)

We cover this, but don't explicitly differentiate from composite actions that exist in the same repo. I can modify the code to cover this case though.

an artifact download

Isn't artifact upload (publishing) what we care about?

[spencerschrock]

We only check for secrets.GITHUB_TOKEN being passed, why would passing github.* be unsafe?

github.token is another way of accessing secrets.GITHUB_TOKEN

[spencerschrock]

Isn't artifact upload (publishing) what we care about?

We care about downloading an artifact produced by the other workflow.

And also, if the triggered workflow is restricted to branch: [] as in Pedro's link, then it's not vulnerable to this attack.

See #3861 (comment)

[laurentsimon]

We cover this, but don't explicitly differentiate from composite actions that exist in the same repo. I can modify the code to cover this case though.

My suggestion is to keep this PR simple and do this in another PR. There has not been many complains (AFAIK) about false positives introduced by the lack of permission checking. Maybe it's uncommon in practice so we need not prioritize this?

Isn't artifact upload (publishing) what we care about?

We care about downloading an artifact produced by the other workflow.

+1

And also, if the triggered workflow is restricted to branch: [] as in Pedro's link, then it's not vulnerable to this attack.

See #3861 (comment)

I thought it does work As for my previous comment, I did some digging and this does indeed seem to work, even though release.yml sets branches: [main]. This is very weird to me and honestly feels like a bug...

[github-actions]

This pull request has been marked stale because it has been open for 10 days with no activity

[raghavkaul]

Not stale, waiting for update on #3861.

[github-actions]

This pull request has been marked stale because it has been open for 10 days with no activity

[github-actions]

This pull request has been marked stale because it has been open for 10 days with no activity

[laurentsimon]

@raghavkaul still interested in getting this PR cross the finish line?

[github-actions]

This pull request has been marked stale because it has been open for 10 days with no activity