fix(release): harden make-tarball's manifest writing the way #1311 hardened reading - #1332
Open
justinjoy wants to merge 2 commits into
Open
fix(release): harden make-tarball's manifest writing the way #1311 hardened reading#1332justinjoy wants to merge 2 commits into
justinjoy wants to merge 2 commits into
Conversation
This was referenced Sep 2, 2026
make-tarball.sh: git rev-parse lacks --, so a ref beginning with a dash is parsed as an option
#1331
Open
verify-release.sh rejected any archive whose name coreutils escapes, before
comparing a single hash:
checksum manifest does not name back\slash.tar.gz: back\\slash.tar.gz
GNU coreutils escapes exactly three characters in a name -- backslash as `\\`,
newline as `\n`, carriage return as `\r` -- and prefixes the whole line with a
literal `\`. The set was determined by scanning all 254 legal filename bytes
through sha256sum, not from documentation: tab, other control bytes and UTF-8
are not escaped. b3sum escapes the same three with the same marker.
A third failure had nothing to do with escaping. The name was read as awk's
`$2`, and coreutils does not escape spaces, so `has space.tar.gz` was truncated
to `has` and reported as a name mismatch. The name is now taken as everything
after the two-character separator, which is also correct for binary mode.
Compare against the ESCAPED form of the expected name rather than unescaping the
manifest's. Unescaping is ambiguous in the wrong direction: a file genuinely
named `a\nb.tar.gz` is written `a\\nb.tar.gz`, and replacing `\\` then `\n` in
sequence turns it into a newline. Measured -- the naive form corrupts that name.
Escaping is one-way and has no such case, which is why `a\nb.tar.gz` is a
regression case rather than an incidental one.
Every path still fails closed. The name check only selects which manifest line
is trusted; the hash is recomputed independently, so no name-check outcome can
produce a false "verified".
This makes #1311's leading-backslash strip in hash_of reachable for the first
time and pins it, so the comment declaring it deliberately unpinned is replaced.
Refs #1315
justinjoy
force-pushed
the
fix/1315-manifest-unescape
branch
from
September 3, 2026 09:25
679bb8c to
329ce3e
Compare
…rdened reading
make-tarball.sh wrote its checksum manifests with
(cd "$(dirname "$archive")" && sha256sum "$(basename "$archive")" > ...)
missing the three hardenings #1311 applied to verify-release.sh. Each has a
distinct failure, and each is now covered by a fixture that no other kills:
CDPATH= With CDPATH set, `cd` on a relative path lands in whichever CDPATH
entry matched first, so the manifest is written in the wrong place
or sha256sum finds no archive there and the run aborts. It also
echoes the resolved path on its own stdout, which the archive_dir
capture takes as part of the value. release-tag.yml:274,344 pass a
relative out_dir, so this is the production shape.
It does NOT corrupt the manifest -- the redirection binds to
sha256sum, not to cd. An earlier draft of this change claimed
otherwise and added a test assertion for that non-existent
mechanism; both are gone.
-- An out_dir beginning with `-` is parsed as options.
-P `cd` is logical, so with `link -> y/target` an out_dir of
`x/dir/link/..` is `y` -- the link target's parent -- for the kernel
that writes the archive, but `x/dir` for a logical cd.
`mkdir -p --` at line 22 is included although the issue's criteria name only
the two subshells. Without it the `--` hardening is unreachable: mkdir fails
first with `invalid option -- 'o'`, so no fixture could kill it, and the issue
separately requires each hardening to be individually killable. Verified both
directions -- the mkdir-only mutant and the subshells-only mutant are each
caught by the dash fixture.
Archive and both manifests are byte-identical to pre-change output for an
ordinary out_dir, and verify-release.sh round-trips them.
Refs #1316
justinjoy
force-pushed
the
fix/1316-tarball-hardening
branch
from
September 3, 2026 09:25
abaa89b to
c0271af
Compare
justinjoy
force-pushed
the
fix/1315-manifest-unescape
branch
from
September 3, 2026 14:29
329ce3e to
72efd1b
Compare
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 #1316. Stacked on #1330 (
fix/1315-manifest-unescape) — review that first.The change
make-tarball.shwrote its checksum manifests missing the three hardenings #1311 applied tothe reading side. Each has a distinct failure, and each now has a fixture no other kills:
CDPATH=cdon a relative path lands in whichever CDPATH entry matched first — manifest written in the wrong place, orsha256sumfinds no archive and the run aborts.release-tag.yml:274,344pass a relativeout_dir, so this is production shape.--out_dirbeginning with-is parsed as options bymkdir,dirname,basenameandcd.-Pcdis logical: withlink -> y/target,x/dir/link/..isy— the link target's parent — for the kernel that writes the archive, butx/dirfor a logicalcd.Scope note:
mkdir -p --Line 22 is hardened although the issue's criterion 1 names only the two subshells. Without it
the
--hardening is unreachable —mkdirfails first withinvalid option -- 'o', so nofixture could kill it, while criterion 3 separately requires each hardening to be individually
killable. Those two criteria conflict as written.
Verified both directions: the mkdir-only mutant and the subshells-only mutant are each caught
by the dash fixture, so neither substitutes for the other.
Mutation matrix
CDPATH=-P--(all)--(subshells only)--(mkdir only)Byte-identity holds (criterion 4): archive,
.sha256and.blake3arecmp-identical topre-change output for an ordinary
out_dir, andverify-release.shround-trips them. Suitepasses under GNU bash 3.2.57 as well as 5.3.
What I got wrong, since it shaped the result
Two of my three fixtures were wrong before I measured them, and both passed against the
unhardened script while covering nothing: for
-PI first usedlink/sub/.., where logicaland physical
cdagree; for--I used./-outdir, whose dirname is..I described the CDPATH mechanism incorrectly in three places, and wrote a test assertion for
it. I claimed
cd's CDPATH chatter is captured into the manifest. It is not — in(cd DIR && sha256sum NAME > NAME.sha256)the redirection binds tosha256sum, not tocd.The reviewer caught it and showed that the assertion I added for that mechanism
(
[ "$(grep -c . -- "$man")" = 1 ]) can never fire. Both the claim and the dead assertion aregone, and the comment now states the binding explicitly so the error is not re-derived.
The
-Pexample was off by one path component — I wrote "where the link points" when myown measurement had shown the link target's parent.
Left out deliberately
make-tarball.sh:8'sgit rev-parse --verify "$ref^{commit}"also lacks--. Filed as#1331 rather than folded in: the
mkdirwidening was forced by the criteria conflictabove, this one is not required by anything, and widening twice on my own judgment in one
change is how scope creep gets rationalised.