arcup: verify_checksum_file silently skips filename validation when .sha256 entry has no filename field

[Sertug17]

Summary

verify_checksum_file in arcup/arcup reads both hash and filename from the .sha256 file:

if ! read -r expected_checksum expected_name < "$checksum_path"; then
    error "Checksum file is empty: $checksum_path"
fi

The filename validation is guarded by:

if [[ -n "$expected_name" ]]; then
    ...
fi

If the .sha256 file contains only a bare hash (no filename field), expected_name is empty and the filename cross-check is silently skipped. The archive is accepted purely on hash match, without verifying the checksum file was intended for this specific archive name.

Impact

Low in practice (GitHub release .sha256 files always include the filename), but the function's contract "verify this checksum file is for $archive_name" is silently violated for bare-hash inputs.

Fix

Add an elif [[ -n "$archive_name" ]]; then error "..." branch to require the filename field when archive_name is provided:

elif [[ -n "$archive_name" ]]; then
    error "Checksum file contains no filename field; expected '$archive_name'"
fi

[osr21]

Confirmed the report — the code reads exactly as quoted, and a bare-hash .sha256 does silently skip the filename cross-check. I'd adjust the severity framing in one direction and raise two adjacent bugs in the same six lines that I think are worse than the reported one.

The premise checks out, with a caveat that makes this more reachable

"GitHub release .sha256 files always include the filename" is true for Circle's release automation. The real v0.8.0 asset:

$ stat -c %s arc-node-v0.8.0-aarch64-apple-darwin.tar.gz.sha256
110
$ cat arc-node-v0.8.0-aarch64-apple-darwin.tar.gz.sha256
33daa925c748a032a601921d4d7e6aa794f830868e6152d8ef91558c797eabb3  arc-node-v0.8.0-aarch64-apple-darwin.tar.gz

110 bytes = 64 hash + 2 spaces + 43-char filename + LF, so the format is exactly as assumed and read -r splits it cleanly.

The caveat: arcup does not only install from Circle's releases.

REPO="${ARC_REPO:-circlefin/arc-node}"          # arcup:11
GITHUB_API_URL="${GITHUB_API_URL:-https://api.github.com}"   # arcup:24

Both are supported overrides, so releases can come from a fork, a self-hosted GitHub Enterprise, or a mirror. Those .sha256 files are frequently hand-rolled rather than produced by the same automation — which is precisely where a bare hash shows up. So the reported gap is reachable in a supported configuration, not just theoretical. That strengthens the case for the fix.

But I'd drop the security framing entirely

This is worth pinning down before the fix lands, so it isn't mistaken for hardening. The filename check is not a security control, and cannot be made into one here.

GPG verification is currently dead code:

# TODO: set GPG key fingerprint once release signing key is published
GPG_KEY_FINGERPRINT=""                          # arcup:17

The entire signature block at arcup:788 is gated on that being non-empty, so it never runs, and install only emits warn "GPG signature verification is disabled". Nothing authenticates the .sha256, and it arrives from the same source as the archive it describes.

That means an attacker able to substitute the checksum file can equally write the expected filename into it — stripping the filename field is the harder path, not the easier one. So the check cannot stop a malicious downgrade or cross-platform swap; it only catches accidental mispairing, such as release tooling attaching the wrong .sha256, or a user passing a mismatched file by hand.

"Low in practice" is the right call, then, but for a firmer reason than the one given: not because bare-hash inputs are rare, but because the check is a consistency guard rather than a boundary. Worth fixing on contract grounds — the function promises to verify the file is for $archive_name and silently doesn't. Just not worth describing as a security fix.

(Credit where due: the authors clearly thought about this threat model — self-update deliberately pins ARCUP_BIN_URL to the canonical repo with the comment "prevent hijack via ARC_REPO".)

Two worse bugs in the same six lines

I ran the surrounding read through the input shapes a non-Circle release repo would realistically produce:

input	read RC	result
hash name\n	0	works
hash name (no trailing newline)	nonzero	errors "Checksum file is empty"
hash\n (reported case)	0	filename check silently skipped
hash (no newline)	nonzero	errors "Checksum file is empty"
hash name\r\n (CRLF)	0	filename comparison fails
multi-entry SHA256SUMS	0	only first line honoured

1. A missing trailing newline reports the file as empty. read returns nonzero at EOF-without-newline even though it populated both variables correctly. Since the check keys off read's exit status rather than the content, a perfectly valid one-line checksum file fails with Checksum file is empty — an error that sends the operator looking for a truncated download that didn't happen. printf '%s %s' ... and several CI snippets produce exactly this.

2. CRLF produces an error message where both sides look identical. With Windows line endings expected_name retains the trailing \r, so the comparison fails and you get:

error: Checksum file is for 'arc-node-v0.8.0-x86_64-unknown-linux-gnu.tar.gz', expected 'arc-node-v0.8.0-x86_64-unknown-linux-gnu.tar.gz'

Two strings that print the same and compare unequal, with nothing on screen to explain why. (A bare hash with CRLF fails differently again — the \r lands in $expected_checksum and trips the ^[0-9A-Fa-f]{64}$ regex, giving "invalid SHA-256 hash" for a valid hash.)

Both are strictly worse than the reported gap: the reported case fails open but silently, while these fail closed with actively misleading diagnostics on valid input.

A fix covering all four

Normalising the line first handles the newline, CRLF and multi-entry cases, and makes the reported elif the natural last piece:

local line
line=$(tr -d '\r' < "$checksum_path" | head -n1)
if [[ -z "$line" ]]; then
    error "Checksum file is empty: $checksum_path"
fi
read -r expected_checksum expected_name <<< "$line"

Command substitution strips the trailing newline, so the empty check now tests content rather than read's exit status; tr -d '\r' removes the CRLF trap; and head -n1 makes the first-line-only behaviour explicit instead of incidental.

Then the reported branch on top:

elif [[ -n "$archive_name" ]]; then
    error "Checksum file contains no filename field; expected '$archive_name'"
fi

One note on that branch: it will reject bare-hash checksum files that some fork release processes legitimately produce. That's the correct call given the stated contract, but it is a behaviour change for ARC_REPO users rather than a pure bug fix, so it belongs in the release notes.