ci: stop interpolating untrusted event data into shell, and seal evidential binaries - #2172
Open
gHashTag wants to merge 2 commits into
Open
ci: stop interpolating untrusted event data into shell, and seal evidential binaries#2172gHashTag wants to merge 2 commits into
gHashTag wants to merge 2 commits into
Conversation
…ential binaries A pull request title was pasted into a run: block, so its author chose what the runner executed. Measured over ten payloads: five execute a command under the interpolated form, including one that writes GITHUB_TOKEN to a file; all ten pass through an env: variable byte for byte with no side effect. Corrects the record: the failure that exposed this was caused by the backticks in the title of #2168, not by the parenthesis. Reproduced byte for byte. The parenthesis is what prevented execution, which makes the finding worse. The unused issue_title output is deleted rather than sanitised. Remaining event fields move to env:, the issue number is validated before it is written to GITHUB_OUTPUT, and SYNC_ARGS becomes an array. Second vector in l1-traceability.yml: a branch name may contain shell metacharacters, so the ref is validated before it reaches git. check_untrusted_shell_interp.py: 5 untrusted interpolations before, 0 after, over 35 workflows. It carries no branches: filter, so it also runs on stacked pull requests. artifact_seal.py records commit, build commands, toolchain, profile, digests and test results, and can rebuild from the commit and compare. Reproduced bit-exactly once the build path was made constant: the same commit built from differently named worktrees differed in 39,830,933 bytes, because a debug build embeds its source path. Closes #2171
Contributor
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
PR DashboardGenerated at: 2026-08-15 09:00:49 UTC
Summary
Seal Status
|
gHashTag
force-pushed
the
w699-untrusted-input
branch
from
August 15, 2026 09:03
0b869e3 to
fb8d62d
Compare
Contributor
|
📓 NotebookLM Notebook linked to this PR
This notebook contains session context, decisions, and artifacts for this work. |
Contributor
PR DashboardGenerated at: 2026-08-15 09:03:58 UTC
Summary
Seal Status
|
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.
Closes #2171
What was wrong
notebook-sync.ymlinterpolatedgithub.event.pull_request.titleandgithub.event.issue.titleinto arun:block. GitHub substitutes an expressioninto the script text before bash parses it, so the author of a title chose
what the runner executed.
Measured with
scripts/ci/test_untrusted_payloads.py, ten payloads whoseexpected effect is declared in the source before the run:
env:formx$(touch M)yx`touch M`ya"; touch M; echo "bx$(touch${IFS}M)yx$(echo $GITHUB_TOKEN > M)ya "quoted" titleit's a title,Name(T), newlineFive of ten execute a command. One of those five writes
$GITHUB_TOKENto afile. The controls are there so the harness has to demonstrate it can tell the
cases apart; a payload the vulnerable form survives proves nothing about the
safe form, and the run fails if any payload does not match its declared class.
Correction to the record
#2171, the tick D report and a commit message all said the CI failure was caused
by the parenthesis in the title of #2168. That is wrong, and the last table
row refutes it: a parenthesis inside a double-quoted assignment is an ordinary
character.
The entry point was the backticks. They opened a command substitution, and
(was a syntax error inside it -- reproduced byte for byte against the CI log:This makes the finding worse, not better. Had the backticked text been a valid
command rather than a declaration, it would have run. The parenthesis is
what stopped it. Titles in this repository quote code in backticks as a matter
of style, so the dangerous construct is the ordinary one.
The fix
issue_titleis deleted, not sanitised. It was published as a job outputand read by nowhere in
.github/orcontrib/backend/notebooklm/. Untrustedtext that no step consumes is best not carried.
env:; the runner sets them, so no characterin them can be parsed as shell.
^[0-9]+$before it is written.GITHUB_OUTPUTis a newline-delimited file: a value containing a newlinedefines further outputs of its own choosing. Demonstrated at the end of the
payload run. Quoting does not help -- the value is already data and the file
format is what is abused.
SYNC_ARGSbecomes an array. A string is re-split by the shell on whateverIFShappens to be, which is a second route from one value to many arguments.extract-issue-- the job that reads untrusted data -- getspermissions: contents: read. It previously had none declared, so it receivedthe repository default, while its sibling job demonstrably writes to the
default branch.
l1-traceability.ymlrangit fetch origin "${{ github.event.pull_request.head.ref }}:$HEAD_SHA".Measured with
git check-ref-format --branch:x$(id)y,x`id`y,a;id;b,x$(cat${IFS}/etc/hostname)y,x|id,x&&id,x'y,x"yare allaccepted as branch names. Only a space is forbidden, and
${IFS}substitutes for a space. The ref now goes through
env:, is validated, and isrefused if it could be read as an option.
The standing check
scripts/ci/check_untrusted_shell_interp.pyclassifies every${{ }}inside arun:block as untrusted, suspect or safe: 5 untrusted before this change, 0after, across 35 workflows. Warnings dropped 13 -> 2 as a side effect.
It carries no
branches:filter, because a gate that filterspull_requestby branch does not run on a stacked PR and reads as green (#2167).
Its scope is stated in its own output: interpolation into shell only. It
does not check
github-script, or JSON, YAML, Markdown or SQL written by a step.E2: evidential binaries
/tmp/t27c.baseand/tmp/t27c.fixedcarried the difference behind severalloop reports and recorded neither their commit nor their profile. Two earlier
tools were already lost to a sandbox reset.
scripts/ci/artifact_seal.pyrecords commit, dirty-tree flag, build commands,toolchain versions, profile, SHA-256 and size of every artifact and declared
input, test results by digest of their output, and the date obtained.
scripts/ci/rebuild_evidence.shbuilds from a named commit and seals in thesame step, so provenance is captured rather than reconstructed.
Verified end to end:
t27c.master-baselinerebuilt fromb928725to the samedigest
836e8bc4....That only worked after the build path was made a constant. A debug build
embeds its source path, so the same commit built from two randomly named
worktrees produced binaries differing in 39,830,933 bytes, and the sealed
binary literally contained
/tmp/t27_evidence_5UEbqk. Digest comparison is ausable verification method only from a fixed path; both the builder and the
verifier now default to
/tmp/t27_seal_build.Two failures of the tool were found by using it, and both are recorded in
comments where they happened:
--rebuildhad nowhere to fetch from once the worktree was gone;
--rebuildlooked for the sealed filename in the rebuilt tree, did notfind it, and printed
unreproduced-- an absent comparison wearing thecostume of a negative result. It now prints
not-evaluatedand the sealrecords
produced_at.The tick D pair is sealed with its commit field empty
(
provenance: unrecorded-at-build-time). Provenance not captured at build timecannot be recovered afterwards, and writing today's
HEADthere wouldmanufacture it.
verify --rebuildrefuses such a seal instead of guessing.What this PR does not claim
check covers shell interpolation.
notebook-sync.ymlstill passes event datato
actions/github-script, which is a different interpreter and out of scope.permissions:was narrowed on one job only. A repository-wide default is aseparate change.
auto-merge-ready-prs.ymldoes not parse as YAML (diffbin: 'unchanged' counted 286 unmeasured files as agreement; 52% coverage was reported as 100% #2166 work) and is untouchedhere on purpose.
Note for review
docs/loop/LOOP-RULES.mddoes not exist onmaster-- it is in the unmerged#2159 branch. Two rules earned here (a payload the vulnerable form survives
has no evidential force, and an absent comparison may never print as a
comparison that failed) are recorded in the loop ledger and belong in that
file when #2159 lands, rather than in a competing copy created here.
scripts/trionmasteris a thin wrapper aroundt27cand has no loopsubcommands, so
artifact_seal.pyis invoked directly. Wiring it astri artifact-sealbelongs with #2159 for the same reason.