Skip to content

fix(release): harden make-tarball's manifest writing the way #1311 hardened reading - #1332

Merged
justinjoy merged 1 commit into
mainfrom
fix/1316-tarball-hardening
Sep 4, 2026
Merged

fix(release): harden make-tarball's manifest writing the way #1311 hardened reading#1332
justinjoy merged 1 commit into
mainfrom
fix/1316-tarball-hardening

Conversation

@justinjoy

Copy link
Copy Markdown
Collaborator

Closes #1316. Stacked on #1330 (fix/1315-manifest-unescape) — review that first.

The change

make-tarball.sh wrote its checksum manifests missing the three hardenings #1311 applied to
the reading side. Each has a distinct failure, and each now has a fixture no other kills:

hardening failure
CDPATH= cd on a relative path lands in whichever CDPATH entry matched first — manifest written in the wrong place, or sha256sum finds no archive and the run aborts. release-tag.yml:274,344 pass a relative out_dir, so this is production shape.
-- an out_dir beginning with - is parsed as options by mkdir, dirname, basename and cd.
-P cd is logical: with link -> y/target, x/dir/link/.. is y — the link target's parent — for the kernel that writes the archive, but x/dir for a logical cd.

Scope note: mkdir -p --

Line 22 is hardened although the issue's criterion 1 names only the two subshells. Without it
the -- hardening is unreachablemkdir fails first with invalid option -- 'o', so no
fixture 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

mutant FAILs
this PR 0
no CDPATH= CDPATH case only
no -P symlink case only
no -- (all) dash case only
no -- (subshells only) dash case only
no -- (mkdir only) dash case only
pre-change all three

Byte-identity holds (criterion 4): archive, .sha256 and .blake3 are cmp-identical to
pre-change output for an ordinary out_dir, and verify-release.sh round-trips them. Suite
passes 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 -P I first used link/sub/.., where logical
and physical cd agree; 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 to sha256sum, not to cd.
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 are
gone, and the comment now states the binding explicitly so the error is not re-derived.

The -P example was off by one path component — I wrote "where the link points" when my
own measurement had shown the link target's parent.

Left out deliberately

make-tarball.sh:8's git rev-parse --verify "$ref^{commit}" also lacks --. Filed as
#1331 rather than folded in: the mkdir widening was forced by the criteria conflict
above, this one is not required by anything, and widening twice on my own judgment in one
change is how scope creep gets rationalised.

…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
justinjoy force-pushed the fix/1316-tarball-hardening branch from c0271af to f13da1a Compare September 4, 2026 00:39
@justinjoy
justinjoy enabled auto-merge (rebase) September 4, 2026 00:44
@justinjoy
justinjoy merged commit 4420342 into main Sep 4, 2026
26 checks passed
@justinjoy
justinjoy deleted the fix/1316-tarball-hardening branch September 4, 2026 01:43
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.

make-tarball.sh: manifest writing lacks the CDPATH/--/-P hardening applied to the reading side

1 participant