From 1dde6116af02954d1d37a80bb7a53ee875d9ae39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Proulx?= Date: Fri, 30 May 2025 14:35:08 -0400 Subject: [PATCH 1/2] Added new Confused Deputy Auto-Merge rule with documentation and new utility functions --- .../en/rules/confused_deputy_auto_merge.md | 77 ++++++++++++ opa/rego/poutine/utils.rego | 30 +++++ .../rules/confused_deputy_auto_merge.rego | 113 ++++++++++++++++++ 3 files changed, 220 insertions(+) create mode 100644 docs/content/en/rules/confused_deputy_auto_merge.md create mode 100644 opa/rego/rules/confused_deputy_auto_merge.rego diff --git a/docs/content/en/rules/confused_deputy_auto_merge.md b/docs/content/en/rules/confused_deputy_auto_merge.md new file mode 100644 index 00000000..09f87a41 --- /dev/null +++ b/docs/content/en/rules/confused_deputy_auto_merge.md @@ -0,0 +1,77 @@ +--- +title: "Confused Deputy Auto-Merge" +slug: confused_deputy_auto_merge +url: /rules/confused_deputy_auto_merge/ +rule: confused_deputy_auto_merge +severity: error +--- + +## Description + +The workflow appears to be vulnerable to a "Confused Deputy" attack when processing Pull Requests, typically from bots like Dependabot. This vulnerability occurs when a workflow triggered by an event like `pull_request_target` automatically trusts an actor (e.g., `github.actor == 'dependabot[bot]'`) to perform privileged actions, such as merging a Pull Request. An attacker can exploit this by tricking the trusted bot (the "confused deputy") into triggering the workflow on a Pull Request from a fork containing malicious changes. The workflow then mistakenly executes the privileged action (e.g., auto-merging the malicious code) because it only checks the identity of the bot that triggered the event, not guaranteeing the origin of the code changes within the Pull Request. + +## Remediation + +The core principle for remediation is to ensure that any automated action, especially merging, is based on a reliable verification of the content and origin of the Pull Request, rather than solely on the actor triggering the workflow event. + +### GitHub Actions + +#### Recommended + +Instead of directly using `github.actor` to authorize merge operations in a `pull_request_target` workflow, use specialized GitHub Actions designed to securely handle dependency update PRs or verify the true initiator of the changes. These actions often inspect the PR metadata or commit history to confirm that the changes are genuinely from the trusted bot and not manipulated by an attacker. + +For Dependabot auto-merges, consider using actions like: +- `dependabot/fetch-metadata`: This action helps you reliably determine metadata about a Dependabot PR, including whether it has been edited by a user. You can then use this information in subsequent steps to make a safer merge decision. +- `fastify/github-action-merge-dependabot`: This action is specifically designed to securely auto-merge Dependabot pull requests. +- `actions-cool/check-user-permission`: This action can be used to verify permissions of the user who created the pull request or initiated the changes, rather than just the `github.actor` of the current workflow run. + +```yaml +name: Auto-Merge Dependabot PRs + +on: + pull_request_target: + types: + - opened + - reopened + - synchronize + +jobs: + dependabot: + runs-on: ubuntu-latest + if: ${{ !github.event.pull_request.head.repo.fork && github.event.pull_request.user.login == 'dependabot[bot]' }} + steps: + - name: Auto-merge the PR + run: gh pr merge --auto --squash ${{ github.event.pull_request.html_url }} + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} +``` + +#### Anti-Pattern + +This example demonstrates a common anti-pattern where a workflow triggered on `pull_request_target` relies solely on `github.actor` to identify a bot (like Dependabot) and then proceeds to automatically merge the Pull Request. + +```yaml +name: Auto-Merge Dependabot PRs + +on: + pull_request_target: + types: + - opened + - reopened + - synchronize + +jobs: + dependabot: + runs-on: ubuntu-latest + if: ${{ github.actor == 'dependabot[bot]' }} + steps: + - name: Auto-merge the PR + run: gh pr merge --auto --squash ${{ github.event.pull_request.html_url }} + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} +``` + +## See Also +- [Weaponizing Dependabot: Pwn Request at its finest](https://boostsecurity.io/blog/weaponizing-dependabot-pwn-request-at-its-finest) +- [GitHub Actions Exploitation: Dependabot](https://www.synacktiv.com/en/publications/github-actions-exploitation-dependabot) +- [Dependabot Confusion: Gaining Access to Private GitHub Repositories using Dependabot](https://giraffesecurity.dev/posts/dependabot-confusion/) diff --git a/opa/rego/poutine/utils.rego b/opa/rego/poutine/utils.rego index ff3e0cfc..cbfab824 100644 --- a/opa/rego/poutine/utils.rego +++ b/opa/rego/poutine/utils.rego @@ -74,3 +74,33 @@ to_set(xs) = xs if { } else := {v | v := xs[_]} if { is_array(xs) } else := {xs} + +######################################################################## +# job order utils +######################################################################## + +job_steps_after(options) := steps if { + steps := {{"step": s, "step_idx": k} | + s := options.job.steps[k] + k > options.step_idx + } +} + +job_steps_before(options) := steps if { + steps := {{"step": s, "step_idx": k} | + s := options.job.steps[k] + k < options.step_idx + } +} + + +######################################################################## +# find_first_uses_in_job +######################################################################## + +find_first_uses_in_job(job, uses) := xs if { + xs := {{"job": job, "step_idx": i} | + s := job.steps[i] + startswith(s.uses, sprintf("%v@", [uses[_]])) + } +} \ No newline at end of file diff --git a/opa/rego/rules/confused_deputy_auto_merge.rego b/opa/rego/rules/confused_deputy_auto_merge.rego new file mode 100644 index 00000000..d917ef13 --- /dev/null +++ b/opa/rego/rules/confused_deputy_auto_merge.rego @@ -0,0 +1,113 @@ +# METADATA +# title: Confused Deputy Auto-Merge +# description: |- +# Confused Deputy for GitHub Actions is a situation where a GitHub event attribute (ex. github.actor) is used to check the last interaction of a certain event. This allows an attacker abuse an event triggered by a Bot (ex. @dependabot recreate) and trigger as a side effect other privileged workflows, which may for instance automatically merge unapproved changes. +# custom: +# level: error +package rules.confused_deputy_auto_merge + +import data.poutine +import data.poutine.utils +import rego.v1 + +merge_commands[cmd] = { + "gh pr merge": `gh\s+pr\s+merge`, + "gh pr review": `gh\s+pr\s+review` +}[cmd] + +merge_github_actions = { + "ad-m/github-push-action", + "ahmadnassri/action-dependabot-auto-merge", + "ana06/automatic-pull-request-review", + "endbug/add-and-commit", + "hmarr/auto-approve-action", + "peter-evans/create-pull-request", + "stefanzweifel/git-auto-commit-action" +} + +actor_bots = { + "dependabot": `dependabot\[bot\]`, + "dependabot-preview": `dependabot-preview\[bot\]`, + "renovate": `renovate\[bot\]`, + "github-actions": `github-actions\[bot\]` +} + +uses_fix_deputy_confusion = { + "dependabot/fetch-metadata", + "fastify/github-action-merge-dependabot", + "actions-cool/check-user-permission" +} + +regex_actor_bot = `(github\.(actor|triggering_actor)\s*==\s*\'%v\'|contains\(\s*fromJSON\(.*%v.*\)\s*,\s*github\.(actor|triggering_actor)\s*\))` # Unit test: https://regex101.com/r/tjnx0o/1 + +rule := poutine.rule(rego.metadata.chain()) + +github.events contains event if some event in { + "pull_request_target", + "workflow_run" +} + +# Case with if in job +results contains poutine.finding(rule, pkg_purl, { + "path": workflow_path, + "line": line, + "details": sprintf("Detected usage of `%s` with actor `%s`", [cmd, bot]), +}) if { + [pkg_purl, workflow_path, job, step, cmd, line] := _merge_commands_run[_] + regex.match( + sprintf(regex_actor_bot, [actor_bots[bot], actor_bots[bot]]), + job["if"] + ) +} + +# Case with if in step +results contains poutine.finding(rule, pkg_purl, { + "path": workflow_path, + "line": line, + "details": sprintf("Detected usage of `%s` with actor `%s`", [cmd, bot]), +}) if { + [pkg_purl, workflow_path, _, step, cmd, line] := _merge_commands_run[_] + regex.match( + sprintf(regex_actor_bot, [actor_bots[bot], actor_bots[bot]]), + step["if"] + ) +} + +# Case with merge command +_merge_commands_run contains [pkg_purl, workflow_path, job, step, cmd, step.lines.run] if { + [pkg_purl, workflow_path, job, step] := _remove_steps_after_fetch_metadata[_] + regex.match( + merge_commands[cmd], + step.run + ) +} + +# Case with github actions +_merge_commands_run contains [pkg_purl, workflow_path, job, step, merge_github_action, step.line] if { + [pkg_purl, workflow_path, job, step] := _remove_steps_after_fetch_metadata[_] + merge_github_action := merge_github_actions[_] + regex.match( + merge_github_action, + step.action + ) +} + +# Case without metadata-fetch +_remove_steps_after_fetch_metadata contains [pkg.purl, workflow.path, job, s_step] if { + pkg := input.packages[_] + workflow := pkg.github_actions_workflows[_] + job := workflow.jobs[_] + relevant_steps := utils.find_first_uses_in_job(job, uses_fix_deputy_confusion) + count(relevant_steps) = 0 + s_step = job.steps[_] +} + +# Case with metadata-fetch which fix deputy confusion problem for future steps +_remove_steps_after_fetch_metadata contains [pkg.purl, workflow.path, job, s.step] if { + pkg := input.packages[_] + workflow := pkg.github_actions_workflows[_] + job := workflow.jobs[_] + relevant_steps := utils.find_first_uses_in_job(job, uses_fix_deputy_confusion) + count(relevant_steps) > 0 + s := utils.job_steps_before(relevant_steps[_])[_] +} \ No newline at end of file From 80ec5765eca05213f1511288dfb7fecd8c04eb76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Proulx?= Date: Fri, 30 May 2025 14:43:14 -0400 Subject: [PATCH 2/2] Fixed assertion in tests --- scanner/inventory_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/scanner/inventory_test.go b/scanner/inventory_test.go index 4ab13f76..200061ab 100644 --- a/scanner/inventory_test.go +++ b/scanner/inventory_test.go @@ -92,6 +92,7 @@ func TestFindings(t *testing.T) { "debug_enabled", "job_all_secrets", "unverified_script_exec", + "confused_deputy_auto_merge", }) findings := []results.Finding{