sec(ci): rollback.yml interpolates the free-text reason input into run blocks in a job holding id-token write #1542

Activity

1. cristim

   added

   triagedItem has been triagedItem has been triaged

   priority/p0Drop everything; same-day fixDrop everything; same-day fix

   severity/criticalMajor harm when it happensMajor harm when it happens

   urgency/nowDrop other thingsDrop other things

   impact/all-usersAffects every userAffects every user

   effort/mDaysDays

   type/securitySecurity findingSecurity finding

   on Jul 28, 2026
2. cristim commented on Jul 28, 2026

   

   cristim

   on Jul 28, 2026

   MemberAuthor

   More actions

   Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

   Second interpolated input in the same job, with the same escalation path. Filing here rather than as a separate issue because the fix is the same one-file change to the same job, and splitting it would produce two PRs touching the same lines.

   Where

   • .github/workflows/rollback.yml:68 - if [[ ! "${{ inputs.image_tag }}" =~ ^[a-zA-Z0-9._-]+$ ]]
   • .github/workflows/rollback.yml:95 - IS_VALID=false sets an output only
   • .github/workflows/rollback.yml:80, :84, :87 - image_tag flows into needs.validate.outputs.image_uri
   • .github/workflows/rollback.yml:236, :291, :333, :378 - that value flows back into run: blocks

   What

   The image_tag guard is supposed to constrain the input, but the value is substituted into the script text before bash evaluates the [[ ]]. An image_tag containing "; <payload>; : closes the test and runs the payload, so the validation it exists to enforce never executes. The guard is self-defeating for exactly the input it guards.

   Second, independent defect on the same lines: IS_VALID=false only sets a step output (:95); the validate job does not exit 1. So validate reports success either way and the entire gate lives in downstream if: conditions on the rollback-* jobs.

   Failure scenario

   Identical escalation surface to the reason finding this issue already covers: code execution in the validate job, which has no environment: binding, running on refs/heads/main, which the AWS deploy role's trust policy accepts (terraform/environments/aws/ci-cd-permissions/role.tf:28-33) independently of the environment:* subjects. A user with plain write access dispatching rollback.yml from main gets RCE in a job that can mint an OIDC token and assume cudly-terraform-deploy, without passing the GitHub Environment reviewer gate that protects the rollback-* jobs.

   Correction to an assumption recorded elsewhere

   #1330 (ci: harden remaining staging cleanup + rollback hardening) says of the verify-image step's unquoted $REPO / $IMAGE_TAG:

   both are shell-safe today thanks to the ^[a-zA-Z0-9._-]+$ regex on inputs.image_tag

   That premise does not hold, for the reason above: the regex cannot constrain a value that has already been substituted into the script source. The verify-image quoting in #1330 should be treated as a real fix, not defence-in-depth. A note to that effect is being added to #1330.

   Fix direction

   Fold into the same change as the reason fix:

   env:
     IMAGE_TAG: ${{ inputs.image_tag }}
   run: |
     if [[ ! "$IMAGE_TAG" =~ ^[a-zA-Z0-9._-]+$ ]]; then
       echo "::error::invalid image_tag"
       exit 1
     fi

   Note the exit 1: the job must fail on invalid input rather than emitting IS_VALID=false and relying on downstream if: conditions.
3. cristim commented on Jul 28, 2026

   

   cristim

   on Jul 28, 2026

   MemberAuthor

   More actions

   Correction to what PR #1641 claims, before this closes.

   This issue described two controls. PR #1641 restores one of them. Stating that precisely because an earlier version of the PR body over-claimed:

   Control 1 — eliminate the injection: DONE. Zero ${{ }} expressions remain in any run: block (11 before). Verified two ways: by parsing the YAML, and by executing the real run-blocks against hostile payloads (image_tag='$(touch PWNED)' rejected with no marker file; reason containing $(...), backticks and newlines rendered as literal text; a forged-JSON attempt against the jq -n --arg audit record produced 9 keys with .result still failure). actionlint with shellcheck: pre-change exit 1, post-change exit 0.

   Control 2 — restore the approval gate: NOT restored, because it never existed.

   This issue says "The approval control is architecturally bypassed rather than merely weak." That is right about the architecture and, it turns out, understated about the control. Querying the live API:

   $ gh api repos/LeanerCloud/CUDly/environments
   total_count: 5
   aws-fargate-dev: protection_rules=[] | aws-fargate-staging: [] | azure-: [] | dev: [] | gcp-: []
   

   No environment in this repo has any protection rules, and none of the four <cloud>-<env>-rollback environments exists. GitHub auto-creates a referenced environment bare on first use, so those jobs would have run with no approval regardless. The environments literally named azure- and gcp- are the fossil record of that auto-create path already firing, when an ${{ inputs.environment }} rendered empty.

   So the gate was not bypassed by the ungated job — there was no gate to bypass. Filed as #1660 (provision the environments and their required reviewers in IaC, so they cannot be auto-created bare).

   On closing this issue. PR #1641 says Closes LeanerCloud/cloud-commitments-cli#1542, and I think that is right, but flagging the judgement so it can be overruled:

   • The defect this issue identifies — arbitrary code execution reaching production cloud credentials — is dead. Both halves of the described failure scenario are gone: no RCE, and no unbound job holding id-token: write.
   • Fix-direction bullet 4 ("Remove repo:...:ref:refs/heads/main from the role trust policy") is not done, and deliberately so: deploy-gcp, cleanup-staging and destroy-fargate-dev currently hold id-token: write with no environment binding and would break. Tracked in sec(ci): destroy and rollback workflows have no environment binding, so no reviewer gate applies #1591 and sec(ci): sibling deploy and sanity workflows grant id-token write to ungated jobs cloud-commitments-platform#140.
   • The gate provisioning is sec(ci): deployment environments have no protection rules, so environment-bound credentialed jobs run unapproved cloud-commitments-platform#141.

   If the preference is that this issue stays open until the trust-policy subject is removed and the environments carry reviewers, say so and I will switch the PR to Refs LeanerCloud/cloud-commitments-cli#1542.

   Other findings from the review of #1641, all filed rather than folded in: LeanerCloud/cloud-commitments-platform#139 (the rollback environments are absent from the AWS and Azure trust-policy sub allowlists, so that path cannot authenticate at all — pre-existing, the original had the same bindings), LeanerCloud/cloud-commitments-platform#142 (image verified against the assumed role's registry rather than AWS_ACCOUNT_ID's), LeanerCloud/cloud-commitments-platform#140 (sibling workflows share the ungated-credentialed shape), #1646 (no workflow linter — the systemic reason this class was invisible), #1649 (the same defect in deploy-aws-lambda.yml, fixed in PR #1657).

   One accepted tradeoff: image existence is now checked inside each gated rollback job rather than in a standalone job, so a rollback to a nonexistent tag fails after the approval prompt rather than before it. Deliberate — the standalone job was the ungated-but-credentialed one.
4. cristim

   closed this as completedin #1641on Jul 28, 2026
5. cristim

   added a commit that references this issue on Aug 19, 2026

   chore(ci): gate workflows on actionlint and zizmor (#1870)

   ...

   c8bc76f
6. cristim

   added 2 commits that reference this issue on Sep 27, 2026

   fix(ci): stop interpolating dispatch inputs into rollback run blocks (#…

   ...

   This issue will close once commit 1400cb7 is merged into the 'main' branch.

   1400cb7

   chore(ci): gate workflows on actionlint and zizmor (#1870)

   ...

   This issue will close once commit 28cb9c0 is merged into the 'main' branch.

   28cb9c0