Skip to content

Commit

Permalink
feat: bot-conditions (#460)
Browse files Browse the repository at this point in the history
  • Loading branch information
@woodruffw
woodruffw authored Jan 18, 2025
1 parent 8a746a3 commit 9b2ecfa
Show file tree
Hide file tree
Showing 9 changed files with 391 additions and 28 deletions.
105 changes: 89 additions & 16 deletions docs/audits.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ Unless needed for `git` operations, @actions/checkout should be used with
If the persisted credential is needed, it should be made explicit
with `#!yaml persist-credentials: true`.

=== "Before"
=== "Before :warning:"

```yaml title="artipacked.yml" hl_lines="7"
on: push
Expand All @@ -59,7 +59,7 @@ with `#!yaml persist-credentials: true`.
- uses: actions/checkout@v4
```

=== "After"
=== "After :white_check_mark:"

```yaml title="artipacked.yml" hl_lines="7-9"
on: push
Expand Down Expand Up @@ -155,7 +155,7 @@ by default, and then set specific job-level permissions as needed.

For example:

=== "Before"
=== "Before :warning:"

```yaml title="excessive-permissions.yml" hl_lines="8-9"
on:
Expand Down Expand Up @@ -191,7 +191,7 @@ For example:
uses: pypa/gh-action-pypi-publish@release/v1
```

=== "After"
=== "After :white_check_mark:"

```yaml title="excessive-permissions.yml" hl_lines="8 21-22"
on:
Expand Down Expand Up @@ -245,7 +245,7 @@ Use [encrypted secrets] instead of hardcoded credentials.

[encrypted secrets]: https://docs.github.com/en/actions/security-for-github-actions/security-guides/using-secrets-in-github-actions

=== "Before"
=== "Before :warning:"

```yaml title="hardcoded-container-credentials.yml" hl_lines="11 17"
on:
Expand All @@ -269,7 +269,7 @@ Use [encrypted secrets] instead of hardcoded credentials.
- run: echo 'hello!'
```

=== "After"
=== "After :white_check_mark:"

```yaml title="hardcoded-container-credentials.yml" hl_lines="11 17"
on:
Expand Down Expand Up @@ -485,7 +485,7 @@ shell quoting/expansion rules.
To avoid having to specialize your handling for different runners,
you can set `shell: sh` or `shell: bash`.

=== "Before"
=== "Before :warning:"

```yaml title="template-injection.yml" hl_lines="3"
- name: Check title
Expand All @@ -497,7 +497,7 @@ shell quoting/expansion rules.
fi
```

=== "After"
=== "After :white_check_mark:"

```yaml title="template-injection.yml" hl_lines="3 8-9"
- name: Check title
Expand Down Expand Up @@ -581,7 +581,7 @@ For Docker actions (like `docker://ubuntu`): add an appropriate

A before/after example is shown below.

=== "Before"
=== "Before :warning:"

```yaml title="unpinned-uses.yml" hl_lines="8 12"
name: unpinned-uses
Expand All @@ -601,7 +601,7 @@ A before/after example is shown below.
args: hello!
```

=== "After"
=== "After :white_check_mark:"

```yaml title="unpinned-uses.yml" hl_lines="8 12"
name: unpinned-uses
Expand Down Expand Up @@ -652,7 +652,7 @@ Other resources:
In general, users should use for [GitHub Actions environment files]
(like `GITHUB_PATH` and `GITHUB_OUTPUT`) instead of using workflow commands.

=== "Before"
=== "Before :warning:"

```yaml title="insecure-commands" hl_lines="3"
- name: Setup my-bin
Expand All @@ -662,7 +662,7 @@ In general, users should use for [GitHub Actions environment files]
ACTIONS_ALLOW_UNSECURE_COMMANDS: true
```

=== "After"
=== "After :white_check_mark:"

```yaml title="insecure-commands" hl_lines="3"
- name: Setup my-bin
Expand Down Expand Up @@ -763,9 +763,9 @@ intended to publish build artifacts:

| Type | Examples | Introduced in | Works offline | Enabled by default |
|----------|-------------------------|---------------|----------------|--------------------|
| Workflow | [secrets-inherit] | v1.1.0 | ✅ | ✅ |
| Workflow | [secrets-inherit.yml] | v1.1.0 | ✅ | ✅ |

[secrets-inherit]: https://github.com/woodruffw/gha-hazmat/blob/main/.github/workflows/secrets-inherit
[secrets-inherit.yml]: https://github.com/woodruffw/gha-hazmat/blob/main/.github/workflows/secrets-inherit.yml

Detects excessive secret inheritance between calling workflows and reusable
(called) workflows.
Expand All @@ -781,7 +781,7 @@ secrets a reusable workflow was executed with.
In general, `secrets: inherit` should be replaced with a `secrets:` block
that explicitly forwards each secret actually needed by the reusable workflow.

=== "Before"
=== "Before :warning:"

```yaml title="reusable.yml" hl_lines="4"
jobs:
Expand All @@ -790,7 +790,7 @@ that explicitly forwards each secret actually needed by the reusable workflow.
secrets: inherit
```

=== "After"
=== "After :white_check_mark:"

```yaml title="reusable.yml" hl_lines="4-6"
jobs:
Expand All @@ -801,6 +801,78 @@ that explicitly forwards each secret actually needed by the reusable workflow.
me-too: ${{ secrets.me-too }}
```

## `bot-conditions`

| Type | Examples | Introduced in | Works offline | Enabled by default |
|----------|-------------------------|---------------|----------------|--------------------|
| Workflow | [bot-conditions.yml] | v1.2.0 | ✅ | ✅ |

[bot-conditions.yml]: https://github.com/woodruffw/gha-hazmat/blob/main/.github/workflows/bot-conditions.yml

Detects potentially spoofable bot conditions.

Many workflows allow trustworthy bots (such as [Dependabot](https://github.com/dependabot))
to bypass checks or otherwise perform privileged actions. This is often done
with a `github.actor` check, e.g.:

```yaml
if: github.actor == 'dependabot[bot]'
```
However, this condition is spoofable: `github.actor` refers to the *last* actor
to perform an "action" on the triggering context, and not necessarily
the actor actually causing the trigger. An attacker can take
advantage of this discrepancy to create a PR where the `HEAD` commit
has `github.actor == 'dependabot[bot]'` but the rest of the branch history
contains attacker-controlled code, bypassing the actor check.

Other resources:

* [GitHub Actions exploitations: Dependabot]

### Remediation

In general, checking a trigger's authenticity via `github.actor` is
insufficient. Instead, most users should use `github.event.pull_request.user.login`
or similar, since that context refers to the actor that *created* the Pull Request
rather than the last one to modify it.

More generally,
[GitHub's documentation recommends](https://docs.github.com/en/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions)
not using `pull_request_target` for auto-merge workflows.

=== "Before :warning:"

```yaml title="bot-conditions.yml" hl_lines="1 6"
on: pull_request_target
jobs:
automerge:
runs-on: ubuntu-latest
if: github.actor == 'dependabot[bot] && github.repository == 'me/my-repo'
steps:
- run: gh pr merge --auto --merge "$PR_URL"
env:
PR_URL: ${{ github.event.pull_request.html_url }}
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
```

=== "After :white_check_mark:"

```yaml title="bot-conditions.yml" hl_lines="1 6"
on: pull_request
jobs:
automerge:
runs-on: ubuntu-latest
if: github.event.pull_request.user.login == 'dependabot[bot] && github.repository == 'me/my-repo'
steps:
- run: gh pr merge --auto --merge "$PR_URL"
env:
PR_URL: ${{ github.event.pull_request.html_url }}
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
```

[ArtiPACKED: Hacking Giants Through a Race Condition in GitHub Actions Artifacts]: https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/
[Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests]: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/
[What the fork? Imposter commits in GitHub Actions and CI/CD]: https://www.chainguard.dev/unchained/what-the-fork-imposter-commits-in-github-actions-and-ci-cd
Expand All @@ -822,3 +894,4 @@ that explicitly forwards each secret actually needed by the reusable workflow.
[reusable workflows]: https://docs.github.com/en/actions/sharing-automations/reusing-workflows
[Principle of Least Authority]: https://en.wikipedia.org/wiki/Principle_of_least_privilege
[Cacheract: The Monster in your Build Cache]: https://adnanthekhan.com/2024/12/21/cacheract-the-monster-in-your-build-cache/
[GitHub Actions exploitations: Dependabot]: https://www.synacktiv.com/publications/github-actions-exploitation-dependabot
28 changes: 17 additions & 11 deletions docs/release-notes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ of `zizmor`.

## Next (UNRELEASED)

### Improved
### New Features 🌈

* **New audit**: [bot-conditions] detects spoofable uses of `github.actor`
within dangerous triggers (#460)

### Improvements 🌱

* The [unpinned-uses] audit no longer flags local reusable workflows or actions
as unpinned/unhashed (#439)
Expand All @@ -20,7 +25,7 @@ of `zizmor`.
incorrect) repository-relative path (#453)
* `zizmor` now provides `manylinux` wheel builds for `aarch64` (#457)

### Fixed
### Bug Fixes 🐛

* The [template-injection] audit no longer considers `github.event.pull_request.base.sha`
dangerous (#445)
Expand All @@ -29,7 +34,7 @@ of `zizmor`.

## v1.1.1

### Fixed
### Bug Fixes 🐛

* Fixed a regression where workflows with calls to unpinned reusable workflows
would fail to parse (#437)
Expand All @@ -39,17 +44,17 @@ of `zizmor`.
This release comes with one new audit ([secrets-inherit]), plus a slew
of bugfixes and internal refactors that unblock future improvements!

### Added
### New Features 🌈

* **New audit**: [secrets-inherit] detects use of `secrets: inherit` with
reusable workflow calls (#408)

### Improved
### Improvements 🌱

* The [template-injection] audit now detects injections in calls
to @azure/cli and @azure/powershell (#421)

### Fixed
### Bug Fixes 🐛

* The [template-injection] audit no longer consider `github.server_url`
dangerous (#412)
Expand All @@ -61,12 +66,12 @@ of bugfixes and internal refactors that unblock future improvements!
This is a small quality and bugfix release. Thank you to everybody
who helped by reporting and shaking out bugs from our first stable release!

### Improved
### Improvements 🌱

* The [github-env] audit now detects dangerous writes to `GITHUB_PATH`,
is more precise, and can produce multiple findings per run block (#391)

### Fixed
### Bug Fixes 🐛

* `workflow_call.secrets` keys with missing values are now parsed correctly (#388)
* The [cache-poisoning] audit no longer incorrectly treats `docker/build-push-action` as
Expand All @@ -89,7 +94,7 @@ happen with a new major version.
This stable release comes with a large number of new features as well
as stability commitments for existing features; read more below!

### Added
### New Features 🌈

* Composite actions (i.e. `action.yml` where the action is *not* a Docker
or JavaScript action) are now supported, and are audited by default
Expand All @@ -110,7 +115,7 @@ as stability commitments for existing features; read more below!
This can be used to connect to a GitHub Enterprise (GHE) instance
instead of the default `github.com` instance.

### Improved
### Improvements 🌱

* The [cache-poisoning] audit is now aware of common publishing actions
and uses then to determine whether to produce a finding (#338, #341)
Expand All @@ -124,7 +129,7 @@ as stability commitments for existing features; read more below!
* The [github-env] audit is now significantly more precise on `bash` and `pwsh`
inputs (#354)

### Fixed
### Bug Fixes 🐛

* The [excessive-permissions] audit is now less noisy on single-job workflows (#337)
* Expressions like `function().foo.bar` are now parsed correctly (#340)
Expand Down Expand Up @@ -447,3 +452,4 @@ This is one of `zizmor`'s bigger recent releases! Key enhancements include:
[template-injection]: ./audits.md#template-injection
[secrets-inherit]: ./audits.md#secrets-inherit
[unpinned-uses]: ./audits.md#unpinned-uses
[bot-conditions]: ./audits.md#bot-conditions
Loading

0 comments on commit 9b2ecfa

Please sign in to comment.