Pass inputs through env, so an input value cannot become shell - #9
Merged
Conversation
`${{ inputs.* }}` is substituted into the body of `run:` before bash
parses it, so an input carrying shell syntax is executed rather than
read. Verified locally: `patterns: 'x"; id > /tmp/PWNED; echo "'` ran as
root inside the job.
Seven inputs were affected — config-file, source-locale, patterns,
api-url, file-tag-name, monitor-interval, monitor-max-attempts. Each now
travels as an env var and is dereferenced as "$INPUT_*", which is the
treatment api-token already had. `github.action_path` moves with them.
The caller controls these values today, so this is not exploitable from
outside a repository. It becomes exploitable the moment a workflow wires
something untrusted into one of them, e.g.
`patterns: ${{ github.event.pull_request.title }}`.
The self-test gains a check that fails if any expression is interpolated
into a run: body again — it flags all 12 occurrences on the previous
revision.
No behaviour change: the local act suite (two happy paths, four
must-fail paths, one injection probe) is green.
An unquoted `run:` inside a step name is a mapping value to the YAML parser, so the whole workflow failed to load.
A run body is interpolated too, so spelling the delimiters literally inside it made the workflow fail to load — the check meant to ban interpolation could not itself survive being written down. The pattern is now assembled at runtime, and the explanatory comment names the delimiters instead of quoting them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every input except
api-tokenwas interpolated into the body ofrun:with${{ }}. That substitution happens before bash parses the script, so an input carrying shell syntax is executed rather than read.Verified locally under
actwithpatterns: 'x"; id > /tmp/PWNED; echo "'— the injected command ran as root inside the job.Affected
config-file,source-locale,patterns,api-url,file-tag-name,monitor-interval,monitor-max-attempts. Each now travels as an env var and is dereferenced as"$INPUT_*"— the treatmentapi-tokenalready had.github.action_pathmoves along with them.Reachability
The calling workflow controls these values, so this is not exploitable from outside a repository as written. It becomes exploitable as soon as a caller wires something untrusted into one of them:
That is a normal-looking workflow, and it would hand arbitrary shell to anyone who can open a pull request.
Regression guard
The self-test gains a check that fails if any expression is interpolated into a
run:body again. Against the previous revision it reports all 12 occurrences.Verification
Local
actsuite against this branch — two happy paths (committed config, auto-detect), four must-fail paths (bad token, terminal failure, soft-fail upload, undetectable layout), and an injection probe that asserts the payload did not run: all green. The same probe fails onmain, which is what makes it a gate rather than a comment.Not in this PR:
add-paths: '.'still sweeps the whole working directory into the translation PR. Real, but functional rather than security, and it deserves its own change so this one stays reviewable.