From 851ebece3ccfde7d367448d997ab3730b58a3904 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20Sz=C3=A9pe?= Date: Tue, 10 Dec 2024 13:50:26 +0000 Subject: [PATCH 1/2] Rename "unsecure" to insecure --- docs/audits.md | 4 ++-- src/audit/insecure_commands.rs | 4 ++-- tests/acceptance.rs | 4 ++-- tests/snapshots/snapshot__insecure_commands-2.snap | 2 +- tests/snapshots/snapshot__insecure_commands.snap | 4 ++-- tests/test-data/inlined-ignores.yml | 2 +- tests/test-data/insecure-commands.yml | 4 ++-- 7 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/audits.md b/docs/audits.md index fa8c0f6..fe181dc 100644 --- a/docs/audits.md +++ b/docs/audits.md @@ -634,7 +634,7 @@ Workflow commands (like `::set-env` and `::add-path`) to inject environment variables and therefore obtain code execution). However, users can explicitly re-enable them by setting the -`ACTIONS_ALLOW_UNSECURE_COMMANDS` environment variable at the workflow, +`ACTIONS_ALLOW_INSECURE_COMMANDS` environment variable at the workflow, job, or step level. Other resources: @@ -653,7 +653,7 @@ In general, users should use for [Github Actions environment files] run: | echo "::add-path::$HOME/.local/my-bin" env: - ACTIONS_ALLOW_UNSECURE_COMMANDS: true + ACTIONS_ALLOW_INSECURE_COMMANDS: true ``` === "After" diff --git a/src/audit/insecure_commands.rs b/src/audit/insecure_commands.rs index 57b198a..cbfa0da 100644 --- a/src/audit/insecure_commands.rs +++ b/src/audit/insecure_commands.rs @@ -31,7 +31,7 @@ impl InsecureCommands { .persona(Persona::Auditor) .add_location( location.with_keys(&["env".into()]).annotated( - "non-static environment may contain ACTIONS_ALLOW_UNSECURE_COMMANDS", + "non-static environment may contain ACTIONS_ALLOW_INSECURE_COMMANDS", ), ) .build(workflow) @@ -54,7 +54,7 @@ impl InsecureCommands { } fn has_insecure_commands_enabled(&self, env: &Env) -> bool { - if let Some(EnvValue::String(value)) = env.get("ACTIONS_ALLOW_UNSECURE_COMMANDS") { + if let Some(EnvValue::String(value)) = env.get("ACTIONS_ALLOW_INSECURE_COMMANDS") { !value.is_empty() } else { false diff --git a/tests/acceptance.rs b/tests/acceptance.rs index 70386c7..9431068 100644 --- a/tests/acceptance.rs +++ b/tests/acceptance.rs @@ -210,7 +210,7 @@ fn audit_unpinned_uses() -> anyhow::Result<()> { } #[test] -fn audit_unsecure_commands_allowed() -> anyhow::Result<()> { +fn audit_insecure_commands_allowed() -> anyhow::Result<()> { let auditable = workflow_under_test("insecure-commands.yml"); let cli_args = [&auditable]; @@ -225,7 +225,7 @@ fn audit_unsecure_commands_allowed() -> anyhow::Result<()> { assert_value_match( &findings, "$[0].locations[0].concrete.feature", - "ACTIONS_ALLOW_UNSECURE_COMMANDS", + "ACTIONS_ALLOW_INSECURE_COMMANDS", ); Ok(()) diff --git a/tests/snapshots/snapshot__insecure_commands-2.snap b/tests/snapshots/snapshot__insecure_commands-2.snap index fc7ddc3..07ae5c2 100644 --- a/tests/snapshots/snapshot__insecure_commands-2.snap +++ b/tests/snapshots/snapshot__insecure_commands-2.snap @@ -8,7 +8,7 @@ error[insecure-commands]: execution of insecure workflow commands is enabled | 8 | env: | _____^ -9 | | ACTIONS_ALLOW_UNSECURE_COMMANDS: yes +9 | | ACTIONS_ALLOW_INSECURE_COMMANDS: yes | |__________________________________________^ insecure commands enabled here | = note: audit confidence → High diff --git a/tests/snapshots/snapshot__insecure_commands.snap b/tests/snapshots/snapshot__insecure_commands.snap index bc5d4f9..c6214e9 100644 --- a/tests/snapshots/snapshot__insecure_commands.snap +++ b/tests/snapshots/snapshot__insecure_commands.snap @@ -8,7 +8,7 @@ error[insecure-commands]: execution of insecure workflow commands is enabled | 8 | env: | _____^ -9 | | ACTIONS_ALLOW_UNSECURE_COMMANDS: yes +9 | | ACTIONS_ALLOW_INSECURE_COMMANDS: yes | |__________________________________________^ insecure commands enabled here | = note: audit confidence → High @@ -17,7 +17,7 @@ error[insecure-commands]: execution of insecure workflow commands is enabled --> @@INPUT@@:22:9 | 22 | env: ${{ matrix.env }} - | ^^^^^^^^^^^^^^^^^^^^^^ non-static environment may contain ACTIONS_ALLOW_UNSECURE_COMMANDS + | ^^^^^^^^^^^^^^^^^^^^^^ non-static environment may contain ACTIONS_ALLOW_INSECURE_COMMANDS | = note: audit confidence → Low diff --git a/tests/test-data/inlined-ignores.yml b/tests/test-data/inlined-ignores.yml index 55845a0..075baa4 100644 --- a/tests/test-data/inlined-ignores.yml +++ b/tests/test-data/inlined-ignores.yml @@ -13,7 +13,7 @@ jobs: insecure-commands-ignored: runs-on: ubuntu-latest env: - ACTIONS_ALLOW_UNSECURE_COMMANDS: yes # zizmor: ignore[insecure-commands] + ACTIONS_ALLOW_INSECURE_COMMANDS: yes # zizmor: ignore[insecure-commands] steps: - run: echo "I shall pass!" diff --git a/tests/test-data/insecure-commands.yml b/tests/test-data/insecure-commands.yml index caeb10e..7be0cbd 100644 --- a/tests/test-data/insecure-commands.yml +++ b/tests/test-data/insecure-commands.yml @@ -6,7 +6,7 @@ jobs: some-dangerous-job: runs-on: ubuntu-latest env: - ACTIONS_ALLOW_UNSECURE_COMMANDS: yes + ACTIONS_ALLOW_INSECURE_COMMANDS: yes steps: - run: echo "don't do this" @@ -15,7 +15,7 @@ jobs: strategy: matrix: env: - - ACTIONS_ALLOW_UNSECURE_COMMANDS: yes + - ACTIONS_ALLOW_INSECURE_COMMANDS: yes steps: - run: echo "don't do this" From 8ffcad2d3a20efeed58c729821e6e7b8f509d0d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20Sz=C3=A9pe?= Date: Tue, 10 Dec 2024 19:39:12 +0000 Subject: [PATCH 2/2] Revert GitHub's own misspelling --- docs/audits.md | 12 ++++++------ docs/development.md | 2 +- src/audit/insecure_commands.rs | 4 ++-- tests/acceptance.rs | 4 ++-- tests/snapshots/snapshot__insecure_commands-2.snap | 2 +- tests/snapshots/snapshot__insecure_commands.snap | 4 ++-- tests/test-data/inlined-ignores.yml | 2 +- tests/test-data/insecure-commands.yml | 4 ++-- 8 files changed, 17 insertions(+), 17 deletions(-) diff --git a/docs/audits.md b/docs/audits.md index fe181dc..93dfd15 100644 --- a/docs/audits.md +++ b/docs/audits.md @@ -629,12 +629,12 @@ A before/after example is shown below. Detects opt-in for executing insecure workflow commands. Workflow commands (like `::set-env` and `::add-path`) -[were deprecated by Github] in 2020 due to their inherent weaknesses +[were deprecated by GitHub] in 2020 due to their inherent weaknesses (e.g., allowing any command with the ability to emit to `stdout` to inject environment variables and therefore obtain code execution). However, users can explicitly re-enable them by setting the -`ACTIONS_ALLOW_INSECURE_COMMANDS` environment variable at the workflow, +`ACTIONS_ALLOW_UNSECURE_COMMANDS` environment variable at the workflow, job, or step level. Other resources: @@ -643,7 +643,7 @@ Other resources: ### Remediation -In general, users should use for [Github Actions environment files] +In general, users should use for [GitHub Actions environment files] (like `GITHUB_PATH` and `GITHUB_OUTPUT`) instead of using workflow commands. === "Before" @@ -653,7 +653,7 @@ In general, users should use for [Github Actions environment files] run: | echo "::add-path::$HOME/.local/my-bin" env: - ACTIONS_ALLOW_INSECURE_COMMANDS: true + ACTIONS_ALLOW_UNSECURE_COMMANDS: true ``` === "After" @@ -703,8 +703,8 @@ If you need to pass state between steps, consider using `GITHUB_OUTPUT` instead. [Trusted Publishing - RubyGems Guides]: https://guides.rubygems.org/trusted-publishing/ [Trusted publishing: a new benchmark for packaging security]: https://blog.trailofbits.com/2023/05/23/trusted-publishing-a-new-benchmark-for-packaging-security/ [Trusted Publishers for All Package Repositories]: https://repos.openssf.org/trusted-publishers-for-all-package-repositories.html -[were deprecated by Github]: https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ -[Github Actions environment files]: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#environment-files +[were deprecated by GitHub]: https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ +[GitHub Actions environment files]: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#environment-files [Semgrep audit]: https://semgrep.dev/r?q=yaml.github-actions.security.allowed-unsecure-commands.allowed-unsecure-commands [GitHub Actions exploitation: environment manipulation]: https://www.synacktiv.com/en/publications/github-actions-exploitation-repo-jacking-and-environment-manipulation [GHSL-2024-177: Environment Variable injection in an Actions workflow of Litestar]: https://securitylab.github.com/advisories/GHSL-2024-177_Litestar/ diff --git a/docs/development.md b/docs/development.md index 91349dd..ca76708 100644 --- a/docs/development.md +++ b/docs/development.md @@ -180,7 +180,7 @@ Some things that can be useful to discuss beforehand: - Which criticality should we assign for this new finding? - Which confidence should we assign for this new finding? - Should this new audit be pedantic at all? -- Does this new audit require using the Github API, or is it entirely offline? +- Does this new audit require using the GitHub API, or is it entirely offline? When developing a new `zizmor` audit, there are a couple of implementation details to be aware of: diff --git a/src/audit/insecure_commands.rs b/src/audit/insecure_commands.rs index cbfa0da..57b198a 100644 --- a/src/audit/insecure_commands.rs +++ b/src/audit/insecure_commands.rs @@ -31,7 +31,7 @@ impl InsecureCommands { .persona(Persona::Auditor) .add_location( location.with_keys(&["env".into()]).annotated( - "non-static environment may contain ACTIONS_ALLOW_INSECURE_COMMANDS", + "non-static environment may contain ACTIONS_ALLOW_UNSECURE_COMMANDS", ), ) .build(workflow) @@ -54,7 +54,7 @@ impl InsecureCommands { } fn has_insecure_commands_enabled(&self, env: &Env) -> bool { - if let Some(EnvValue::String(value)) = env.get("ACTIONS_ALLOW_INSECURE_COMMANDS") { + if let Some(EnvValue::String(value)) = env.get("ACTIONS_ALLOW_UNSECURE_COMMANDS") { !value.is_empty() } else { false diff --git a/tests/acceptance.rs b/tests/acceptance.rs index 9431068..11ad78c 100644 --- a/tests/acceptance.rs +++ b/tests/acceptance.rs @@ -6,7 +6,7 @@ use serde_json_path::JsonPath; mod common; // Acceptance tests for zizmor, on top of Json output -// For now we don't cover tests that depends on Github API under the hood +// For now we don't cover tests that depends on GitHub API under the hood fn zizmor() -> Command { let mut cmd = Command::cargo_bin("zizmor").expect("Cannot create executable command"); @@ -225,7 +225,7 @@ fn audit_insecure_commands_allowed() -> anyhow::Result<()> { assert_value_match( &findings, "$[0].locations[0].concrete.feature", - "ACTIONS_ALLOW_INSECURE_COMMANDS", + "ACTIONS_ALLOW_UNSECURE_COMMANDS", ); Ok(()) diff --git a/tests/snapshots/snapshot__insecure_commands-2.snap b/tests/snapshots/snapshot__insecure_commands-2.snap index 07ae5c2..fc7ddc3 100644 --- a/tests/snapshots/snapshot__insecure_commands-2.snap +++ b/tests/snapshots/snapshot__insecure_commands-2.snap @@ -8,7 +8,7 @@ error[insecure-commands]: execution of insecure workflow commands is enabled | 8 | env: | _____^ -9 | | ACTIONS_ALLOW_INSECURE_COMMANDS: yes +9 | | ACTIONS_ALLOW_UNSECURE_COMMANDS: yes | |__________________________________________^ insecure commands enabled here | = note: audit confidence → High diff --git a/tests/snapshots/snapshot__insecure_commands.snap b/tests/snapshots/snapshot__insecure_commands.snap index c6214e9..bc5d4f9 100644 --- a/tests/snapshots/snapshot__insecure_commands.snap +++ b/tests/snapshots/snapshot__insecure_commands.snap @@ -8,7 +8,7 @@ error[insecure-commands]: execution of insecure workflow commands is enabled | 8 | env: | _____^ -9 | | ACTIONS_ALLOW_INSECURE_COMMANDS: yes +9 | | ACTIONS_ALLOW_UNSECURE_COMMANDS: yes | |__________________________________________^ insecure commands enabled here | = note: audit confidence → High @@ -17,7 +17,7 @@ error[insecure-commands]: execution of insecure workflow commands is enabled --> @@INPUT@@:22:9 | 22 | env: ${{ matrix.env }} - | ^^^^^^^^^^^^^^^^^^^^^^ non-static environment may contain ACTIONS_ALLOW_INSECURE_COMMANDS + | ^^^^^^^^^^^^^^^^^^^^^^ non-static environment may contain ACTIONS_ALLOW_UNSECURE_COMMANDS | = note: audit confidence → Low diff --git a/tests/test-data/inlined-ignores.yml b/tests/test-data/inlined-ignores.yml index 075baa4..55845a0 100644 --- a/tests/test-data/inlined-ignores.yml +++ b/tests/test-data/inlined-ignores.yml @@ -13,7 +13,7 @@ jobs: insecure-commands-ignored: runs-on: ubuntu-latest env: - ACTIONS_ALLOW_INSECURE_COMMANDS: yes # zizmor: ignore[insecure-commands] + ACTIONS_ALLOW_UNSECURE_COMMANDS: yes # zizmor: ignore[insecure-commands] steps: - run: echo "I shall pass!" diff --git a/tests/test-data/insecure-commands.yml b/tests/test-data/insecure-commands.yml index 7be0cbd..caeb10e 100644 --- a/tests/test-data/insecure-commands.yml +++ b/tests/test-data/insecure-commands.yml @@ -6,7 +6,7 @@ jobs: some-dangerous-job: runs-on: ubuntu-latest env: - ACTIONS_ALLOW_INSECURE_COMMANDS: yes + ACTIONS_ALLOW_UNSECURE_COMMANDS: yes steps: - run: echo "don't do this" @@ -15,7 +15,7 @@ jobs: strategy: matrix: env: - - ACTIONS_ALLOW_INSECURE_COMMANDS: yes + - ACTIONS_ALLOW_UNSECURE_COMMANDS: yes steps: - run: echo "don't do this"