New check: Check for dangerous code practices in github workflows

[laurentsimon]

There are several examples of github token leaks via pull_request_target event. It'd be nice to check for it - possibly filtering out known acceptable github actions that use it after we have reviewed their code (and it's pinned by hash). This check goes in the same direction as #414, ie harden github worflows.

[inferno-chromium]

Dangerous permissions
or something :)

[laurentsimon]

Thinking of broadening this check to "Dangerous workflow coding patterns":

1. dangerous events: pull_request_target. In particular, it should not checkout untrusted code, e.g. using with ref. Examples of uses here.
2. auto-merge
3. use of variable in run scripts: this lead to injection, see here. The use is not always vulnerable, e.g. if the value is not attacker's controlled (hash commit) or if there's proper check in the workflow that it's only run on maintainer's code. We can use this list of attacker's controlled values and GITHUB_* env variables to start with. See Feature - Scorecard identify Expression injection in Actions #990 for a library we can use. See CodeQl's experimental detection code [here](https://github.com/github/codeql/blob/main/javascript/ql/src/experimental/Security/CWE-094/ExpressionInjection.ql).
4. logging of github context and secrets.
5. long-term secrets stored in GH encrypted secrets, in particular for pull_request triggers (not rotation possible, a limitation of current APIs)
6. Use of 3P GitHub action in publishing workflows. We should check only the minimum actions are used and that no script are used with run. The latter is reasonable since releasing actions exist for all package managers.
7. Use of secrets (e.g. encrypted secrets) as CLI argument, see here
8. no use of persist-credentials to remove the secrets across steps.
9. Editing of $GITHUB_ENV in pull_request_target and workflow_run: if it's based on attacker-controlled data, it can lead to RCE. Note that in general, editing of GITHUB_ENV should be avoided, as there are other ways to pass data between steps
10. I'm scouring for more :-)

I've left out curl | bash because it's part of another check around dependency pinning #403

We could also make this check the "workflow hardening check": use the above plus token permission check.

[laurentsimon]

related link https://securitylab.github.com/advisories/GHSL-2020-328-GoogleCloudPlatform-microservices-demo-workflow/

[JarLob]

Hi, the important prerequisite for the dangerous pattern of pull_request_target is the usage of untrusted data, most commonly an explicit checkout of github.event.pull_request.head as in the example above. There are many ways to checkout the pull request code, but I think you can reduce false positives by checking for the most common - checkout action ref: parameter. Yet this still doesn't mean it is vulnerable if the code from pull request is treated as data, for example built scripts or tests are not run. But it may be hard to detect it programmatically to completely eliminate FPs. From my experience if pull_request_target is used with checkout ref: 95% chance the code is vulnerable.

[laurentsimon]

Thanks for the information, that's super useful. If someone in your team is interested in taking a stab at this, please let me know.

[laurentsimon]

Added a few more bad things to the list after reading https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

[laurentsimon]

part 3 https://securitylab.github.com/research/github-actions-building-blocks/

[asraa]

Can I try this one? One thing I can't figure out is if it should be merged into Token Permissions and that be renamed, or totally separate like "Workflow Patterns"

[laurentsimon]

You sure can! I think we can start it as a different check first. We can merge them later. I've assigned to you. Thanks you!
Since it's a pretty comprehensive check, we can start with pull_request_target for v4?

[laurentsimon]

Let's try to do 5 and 8. They are fairly simple. Adding to v5 milestone. wdut? We can try to share the workload if needed.

[asraa]

On it for 8!

[calebbrown]

Second order command injection attacks are also possible if attacker controlled input is passed from a workflow to a vulnerable action.

For example:

• this workflow looks safe: github.com/wayou/wayou.github.io/.github/workflows/main.yml
• however, the action it uses is vulnerable to command injection: github.com/wayou/turn-issues-to-posts-action/action.yml
• using the following body in a new issue on the wayou/wayou.github.io repo can run arbitrary code:

EOF
# The EOF above escapes from the heredoc

# arbitrary commands go here

# make sure the EOF doesn't cause the action to fail
cat <<EOF

[laurentsimon]

That's an amazing find. Thanks @calebbrown for letting us know!

[laurentsimon]

I'll take a stab at 5.

[spencerschrock]

Closing as Dangerous-Workflows was impelemented. Any remaining enhancements that people care about should be filed as their own issue.

[laurentsimon]

Re-opening because there's a list of things we have not implemented. I don't mind closing this issue of we created dedicated issues for the remaining ones

[laurentsimon]

Maybe 9 is the only one to create an issue for?

[spencerschrock]

#3830