Skip to content

ci: validate release tags before writing them to $GITHUB_ENV - #1443

Open
dwin-gharibi wants to merge 1 commit into
TencentCloud:masterfrom
dwin-gharibi:ci-github-env-value-injection
Open

ci: validate release tags before writing them to $GITHUB_ENV#1443
dwin-gharibi wants to merge 1 commit into
TencentCloud:masterfrom
dwin-gharibi:ci-github-env-value-injection

Conversation

@dwin-gharibi

Copy link
Copy Markdown
Contributor

Closes #1442.

Motivation

Both release scripts echo parsed tags verbatim into $GITHUB_ENV and $GITHUB_OUTPUT. A value containing a
newline writes extra lines, which the runner parses as further KEY=VALUE pairs — so whoever controls the
pin file can set arbitrary environment variables (LD_PRELOAD, NODE_OPTIONS, PATH, …) for every
subsequent step in the job.

read-kernel-metadata.sh applied no format validation at all; read-release-assets.sh checked only a prefix,
leaving the rest of the value unconstrained.

What this changes

Validation is added in the Python half of each script, before anything is printed — so the check covers both
the eval path and the --export path, and a bad value fails the script instead of being sanitised into
something surprising.

scripts/read-kernel-metadata.shbm.source_tag and pvm.source_tag must match
[A-Za-z0-9._+-]+.

scripts/read-release-assets.sh — the same charset check is applied to all four pins, in addition to
the existing kernel-release- / guest-image- prefix checks (which are kept).

The error names the offending key and quotes the value, and says why the constraint exists.

[A-Za-z0-9._+-] is the character set git tags and release names actually use; it excludes newline, CR, =
and shell metacharacters by construction, so there is no separate newline check to get wrong.

No comment changes.

Testing

Crafted inputs are rejected and nothing is written:

kernel-metadata, bm.source_tag = "kernel-release-1.0\nLD_PRELOAD=/tmp/pwn.so"
  exit=1
  .../evil.json: bm.source_tag must match [A-Za-z0-9._+-]+ (got 'kernel-release-1.0\nLD_PRELOAD=/tmp/pwn.so');
  release tags are written verbatim into $GITHUB_ENV and $GITHUB_OUTPUT
  GITHUB_ENV lines: 0

release-assets, kernel_bm_amd64 = "kernel-release-1.0\nEVIL=1"
  exit=1
  kernel_bm_amd64 in .../evil.yaml must match [A-Za-z0-9._+-]+ (got 'kernel-release-1.0\nEVIL=1'); ...
  GITHUB_ENV lines: 0

Valid inputs still work:

kernel-metadata:                      release-assets:
  1 KERNEL_BM_SOURCE_TAG=...            1 KERNEL_BM_AMD64_RELEASE_TAG=...
  2 KERNEL_PVM_SOURCE_TAG=...           2 KERNEL_BM_ARM64_RELEASE_TAG=...
  exit=0                                3 KERNEL_PVM_RELEASE_TAG=...
                                        4 GUEST_IMAGE_RELEASE_TAG=...
                                        exit=0

And the real committed pin file passes, which is the check that matters for not breaking the release
workflows:

$ GITHUB_ENV=/tmp/env bash scripts/read-release-assets.sh --export
bm_amd64=kernel-release-v1.0.0 bm_arm64=kernel-release-v1.0.0 pvm=kernel-release-v1.0.0 guest_image=guest-image-v1.0.0
exit=0

There is no committed kernel-metadata.json in the tree (it is produced by the kernel release workflow), so
that script was exercised with a synthesised valid file.

CI gates: these scripts are not covered by fmt-check or unit-test-check. They are consumed by
release-one-click.yml and release-docker-images.yml, whose inputs are the files verified above.

Risk / rollout

A release would now fail rather than silently proceed if a pin file contained an unexpected character.
That is the intent, and the current pins pass. If any real tag ever needs a character outside
[A-Za-z0-9._+-], the regex is the single place to widen it.

Signed-off-by: Dwin Gharibi <dwin.gharibi@email.kntu.ac.ir>
Copilot AI lite review requested due to automatic review settings August 21, 2026 11:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@cubesandboxbot

Copy link
Copy Markdown

Review of #1443ci: validate release tags before writing them to $GITHUB_ENV

AI-generated review; not reviewed or approved by a human.

Verdict: Approve — the change is correct and closes the described injection. No blocking issues found.

This is a well-scoped security hardening of the two release scripts that copy parsed tag values verbatim into $GITHUB_ENV / $GITHUB_OUTPUT. I read both scripts in full, traced every consumer in release-one-click.yml and release-docker-images.yml, and checked the producer side in release-kernel.yml and the committed deploy/release-assets.yaml.

Verified correct

  • Check placement — validation runs in the Python half before any print, so it covers both the eval path and the --export path, and a bad value aborts the script (nonzero exit → set -e kills the job) before anything reaches $GITHUB_ENV/$GITHUB_OUTPUT.
  • Anchoring is right\A…\Z requires the entire string to match. Using $ instead would have silently permitted a trailing newline (the exact injection primitive), so this choice matters and is correct. .match() + \A makes \A redundant but harmless.
  • Charset is the right allowlist[A-Za-z0-9._+-] excludes newline/CR, =, and every shell metacharacter, i.e. precisely what makes a KEY=VALUE-line injection possible. A \r\n mid-string, #-comment injection, or << heredoc injection are all blocked by construction.
  • Strip/check consistency — values are .strip()ed before both the check and the print, so the validated string is exactly the string that gets written. There is no "checked the raw value but wrote something else" mismatch.
  • No regression for current values — committed pins (kernel-release-v1.0.0, guest-image-v1.0.0) and the producer-side source tags hardcoded in release-kernel.yml (6.6.119-49.6, 6.6.69-1.2.cubesandbox) all match the charset.
  • Transitive coverage — downstream direct writes in the workflows (release-one-click.yml:319-321/452-454, release-docker-images.yml:162-167/522-523) all derive from these validated values, so they are protected too.

Minor observations (non-blocking)

  1. An input-override path bypasses the new validation (release-docker-images.yml, resolve_asset_tags): when inputs.kernel_bm_amd64_release_tag / …arm64… / …pvm… / guest_image_release_tag are supplied, they are written straight to $GITHUB_OUTPUT (lines 162-167) without passing through read-release-assets.sh. These inputs come from trusted in-repo callers rather than the pin/metadata files, so it is not the same threat model, but if the goal is "no unvalidated tag reaches $GITHUB_OUTPUT", this path is currently uncovered. Worth a follow-up or a note in the PR.

  2. Charset is stricter than the git-tag charset — git refs legally allow characters outside [A-Za-z0-9._+-] (e.g. ~ in Debian-style 1.0.0~rc1, or #, %, &). Current pins and source tags all conform, and the PR explicitly acknowledges the trade-off, so this is fine — just flagging that the regex is the single point to widen if a future release ever needs such a tag.

  3. No automated test — the manual testing in the PR is thorough (crafted rejection + real-pin pass), but the validation logic and fail-closed behavior have no automated guard in CI. There is no existing harness for these scripts, so this is a nit rather than a required change.

  4. Duplicated validation logic between the two scripts (same _SAFE_TAG regex + loop). Acceptable given each script embeds its own Python payload, but a shared helper would keep the two in lockstep.

No comments were posted inline because the change contains no defects that warrant line-level flags; the observations above are follow-up material rather than corrections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug Report] Release scripts write parsed values into $GITHUB_ENV / $GITHUB_OUTPUT unescaped

2 participants