Skip to content

release: verify checksums without putting the path in the hash line - #1314

Open
justinjoy wants to merge 1 commit into
fix/1295-release-script-coveragefrom
fix/1311-checksum-escaping
Open

release: verify checksums without putting the path in the hash line#1314
justinjoy wants to merge 1 commit into
fix/1295-release-script-coveragefrom
fix/1311-checksum-escaping

Conversation

@justinjoy

Copy link
Copy Markdown
Collaborator

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 $1 became \<hash> and the comparison could never match:

before:  SHA256 mismatch for /tmp/back\slash-repro/wirelog-0.60.0.tar.gz
after:   verified checksums for /tmp/back\slash-repro/wirelog-0.60.0.tar.gz

verify-release.sh is 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. b3sum uses 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.sh already writes its manifests, and why those were unaffected.

My first version of this fix was worse than the bug

cd/pwd without -P are logical. A path like a/link/../f.tar.gz resolves to a/f.tar.gz for the shell, while the kernel — and the -f test, cosign, gh attestation, git get-tar-commit-id — open the physical target:

first fix:  verified checksums for .../a/link/../f.tar.gz   exit 0   ← physical target TAMPERED
original:   SHA256 mismatch for  .../a/link/../f.tar.gz              ← correctly rejects

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 now pinned; reverting to logical cd fails a tampered physical target is rejected, not masked by the logical path.

-- on dirname/basename fixes a separate pre-existing bug: a relative archive named -weird.tar.gz died with dirname: 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

  • 38 assertions; full suite 306 Ok / 0 Fail / 12 Skipped
  • Mutations killed: logical cd (the fail-open regression), the original full-path hashing, and an always-pass hasher
  • Reviewer independently probed relative paths, ./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, poisoned CDPATH, execute-only ancestors, and >PATH_MAX — all agree with HEAD except the cases this fixes
  • No residue; the symlink fixture lives under the harness mktemp -d

Reviewed twice; the blocking finding was the fail-open regression above, which I reproduced before fixing.

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.
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.

1 participant