release: verify checksums without putting the path in the hash line - #1314
Open
justinjoy wants to merge 1 commit into
Open
release: verify checksums without putting the path in the hash line#1314justinjoy wants to merge 1 commit into
justinjoy wants to merge 1 commit into
Conversation
verify-release.sh hashed the archive by full path. GNU coreutils escapes
a filename containing a backslash or a newline by prefixing the whole
line with a literal backslash, so `sha256sum "$archive" | awk '{print
$1}'` yielded "\<hash>", the comparison could never match, and the script
reported "SHA256 mismatch" on a byte-perfect archive.
This is the only script here that third parties run -- the docs publish
it as the verification recipe, and that recipe passes an absolute path --
so the failure mode was this project telling a downstream consumer their
download was corrupt or tampered with. b3sum uses the identical
convention, so both halves were affected.
The fix hashes from inside the archive's directory against the bare
basename, which is how make-tarball.sh already writes its manifests and
why those were never affected. The path never enters the output.
The first version of this fix was worse than the bug. `cd` and `pwd`
without -P are logical, so a path like a/link/../f.tar.gz resolved to
a/f.tar.gz for the shell while the kernel -- and the -f test, cosign, gh
attestation and git get-tar-commit-id -- opened the physical target. The
checksum then described a different file than the one being verified, and
a tampered archive reached through a symlinked parent reported "verified
checksums for". Turning a false failure into a false pass, in a tamper
detection tool, is the wrong direction to be wrong in; -P puts the
checksum back in the same frame as every other stage. Both directions
are pinned by tests.
`--` on dirname and basename fixes a separate pre-existing bug: a
relative archive named -weird.tar.gz died with "dirname: invalid option".
The leading-backslash strip is deliberately not covered by a test, and
the comment says so rather than leaving it looking covered. Reaching it
needs a backslash in the archive's NAME, and such an archive cannot get
that far: the manifest-name check reads its field raw, so a
coreutils-written manifest fails there first. Only a hand-written
unescaped manifest would exercise it. Closing the class means unescaping
on the manifest-reading side too, which is filed separately.
Refs #1311.
This was referenced Sep 2, 2026
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
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.
Refs #1311. Stacked on #1310, which stacks on #1296. Merge bottom-up.
The defect
actual_sha=$(sha256sum "$archive" | awk '{print $1}')GNU coreutils escapes a filename containing a backslash or newline by prefixing the whole line with a literal
\, so$1became\<hash>and the comparison could never match:verify-release.shis the only script here third parties run — the docs publish it as the verification recipe, and that recipe passes an absolute path. So this was the project telling a downstream consumer their download was corrupt.b3sumuses the identical convention (verified on 1.8.7), so both halves were affected.The fix hashes from inside the archive's directory against the bare basename — how
make-tarball.shalready writes its manifests, and why those were unaffected.My first version of this fix was worse than the bug
cd/pwdwithout-Pare logical. A path likea/link/../f.tar.gzresolves toa/f.tar.gzfor the shell, while the kernel — and the-ftest,cosign,gh attestation,git get-tar-commit-id— open the physical target:Turning a false failure into a false pass, in a tamper-detection tool, is the wrong direction to be wrong in.
-Pputs the checksum back in the same frame as every other stage. Both directions are now pinned; reverting to logicalcdfailsa tampered physical target is rejected, not masked by the logical path.--ondirname/basenamefixes a separate pre-existing bug: a relative archive named-weird.tar.gzdied withdirname: invalid option -- 'w'.One line is deliberately untested, and says so
The leading-backslash strip cannot be reached end-to-end: a backslash in the archive's name fails the manifest-name check first, because that check reads its field raw. Only a hand-written unescaped manifest would exercise it. I wrote a fixture for it, found it couldn't pass, and removed it rather than pin a line through a scenario that cannot occur — the comment states the gap instead of leaving it looking covered.
Closing the class properly means unescaping on the manifest-reading side too. Filed separately.
Validation
cd(the fail-open regression), the original full-path hashing, and an always-pass hasher./name,../ok/name, symlinked archive file, symlinked parent without.., a two-level symlink chain, explicit relative manifests from a third cwd, spaces and newlines in the directory, poisonedCDPATH, execute-only ancestors, and>PATH_MAX— all agree with HEAD except the cases this fixesmktemp -dReviewed twice; the blocking finding was the fail-open regression above, which I reproduced before fixing.