Detect SHA-1 collisions in every digest apk relies on

[xnox]

apk identifies control sections, signatures and individual files by SHA-1, and
verifies legacy indexes against RSA/SHA-1 signatures. The on-disk format cannot
change, but we can refuse to trust a digest computed over input that carries the
cryptanalytic signature of a SHA-1 collision attack.

Every use of crypto/sha1 now goes through github.com/pjbgf/sha1cd behind a
small internal/sha1cd helper. Its Sum/SumBytes finalise the hash, check the
collision flag, and return ErrCollision instead of a digest.

Why check at finalisation

Two reasons the check has to sit between finalisation and use:

• The collision flag is only conclusive once the padding block has been
  compressed, so it cannot be read before finalising.
• On colliding input sha1cd returns a different digest than crypto/sha1 —
  it rehashes to a safe value. A caller that ignored the flag would both trust
  attacker-shaped input and compute a checksum matching nothing.

Digests of ordinary input are byte-identical to crypto/sha1, so no existing
checksum, cache entry or signature is affected. Golden tests pin that.

Converted

Cached control and signature files, control and signature streams in
ResolveApk and ExpandApk, per-file checksums in checkSums and
installRegularFile, existing-file sums in writeOneFile, control sections in
ParsePackageInfo, conflicting in-memory files in tarfs, and index digests in
parseRepositoryIndex.

Two indirect uses needed restructuring:

• parseRepositoryIndex called sig.DigestAlgorithm.New(), which resolved to
  crypto/sha1 via a blank import. It now calls a hashIndex helper that
  digests SHA-1 through the checked path and SHA-256 directly.
• With that gone nothing calls crypto.SHA1.New(), so signature/rsa.go no
  longer registers crypto/sha1. Leaving it registered would keep a
  collision-blind SHA-1 available. Verification needs only the digest size,
  which is a static lookup; rsa.VerifyPKCS1v15 calls hash.New() only under
  GODEBUG=fips140=only, where SHA-1 is rejected anyway. A test signs and
  verifies a legacy RSA/SHA-1 index signature to confirm that path still works.

Breaking change

apk.ParsePackageInfo now returns the finalised control-section digest
([]byte) rather than a hash.Hash. Handing back an unfinalised hash leaves it
to the caller whether the collision check ever runs, which defeats the point.
Callers drop their .Sum(nil); melange will need the matching one-line update.

Tests

• Golden digests matching crypto/sha1, and detection across write sizes (1, 7,
  64 and 320-byte chunks) — the flag must survive however the input is streamed.
• End to end: a file inside an APK whose content is the SHAttered collision is
  rejected even when its header carries the digest collision detection
  produces, and a colliding index is refused before signature verification.
• The collision vector is the first 320 bytes of shattered-1.pdf, living in
  internal/sha1cd/sha1cdtest. No non-test package imports it and Shattered
  requires a testing.TB, so it is never linked into a binary.

Scope

This detects the disturbance-vector families used by the known identical-prefix
and chosen-prefix attacks (SHAttered, Shambles). It does not make SHA-1 collision
resistant, and apk's format still identifies content by SHA-1.

🤖 Generated with Claude Code

[stevebeattie]

This comment's central claim isn't true, and the blank-import removal it justifies doesn't buy anything.

crypto/sha1 is still linked into every apko binary and its init() still calls crypto.RegisterHash(crypto.SHA1, New). It arrives via crypto/x509 — imported by this very file — plus crypto/tls, github.com/google/uuid, go-jose/v3, golang.org/x/crypto/ssh, keystore-go/v4, ProtonMail/go-crypto/openpgp/packet and sigs.k8s.io/release-utils/hash. None of those are removable from a registry client.

Probing from inside package signature on this branch:

crypto.SHA1.Available() = true
crypto.SHA1.New() works, collision-blind digest = a9993e364706816aba3e25717850c26c9cd0d89d

So a collision-blind crypto.SHA1 is still one call away, and dropping _ "crypto/sha1" from this file changed nothing about that.

It does add a small cost. RSAVerifyDigest(…, crypto.SHA1, …) now depends on rsa.VerifyPKCS1v15 never reaching hash.New() and on x509 continuing to pull sha1 in. The first half holds today — in go1.26 New() is only called under fips140only.Enforced(), and crypto.Hash.Size() is a static table lookup — so the PR body is right about that. But TestVerifyLegacySHA1IndexSignature can't actually validate it: the test passes because sha1 is registered transitively, so it would pass even if the reasoning were wrong.

Suggestion: restore _ "crypto/sha1", or keep the removal and drop the security rationale from this comment. The real guard against reintroducing plain SHA-1 is gosec G505, already enabled in .golangci.yml (only G115 is excluded) — worth citing that instead. Two caveats worth a word: G505 is exactly what the //nolint:gosec comments this PR removes were suppressing, so it's bypassable the same way; and gosec is excluded for _test.go, so tests can reintroduce crypto/sha1 silently.

Note also that the PR body's FIPS reasoning has one ordering slip. In crypto/rsa/fips.go:344,387 the hash.New() inside fips140only.ApprovedHash(fips140hash.Unwrap(hash.New())) is evaluated before SHA-1 can be rejected, and crypto.Hash.New() panics on an unregistered hash. So under GODEBUG=fips140=only an actually-unregistered SHA-1 would panic here, not return the intended error. Harmless today for the same reason the rest of this comment is inaccurate — x509 keeps it registered — but it means the removal has downside without upside. (_ "crypto/sha256" is genuinely needed for that same path, so it should stay.)

[xnox]

init() of modules are performed in import order. thus we can replace _ "crypto/sha1" import with a direct import of "github.com/pjbgf/sha1cd" maybe? because ideally, i want to ensure that sha1cd is the last registered handler of sha1, even if prior imports indirectly pulled in crypto/sha1 already.

alternatively, I could write custom init() that calls into internal/sha1cd to trigger hash registration? that could work too.

I am also very worried about using sha1cd universally - because it creates different hash for the collision detected inputs; but unfortunately that hash is still stable - meaning signature created with sha1cd would verify with sha1cd!

Lack of hash bindings to keys is also very annoying due to pure RSA PKCSv1.5 usage here; as RSA-PSS would be much better - cause then there is hash binding to the key.

[stevebeattie]

You're remembering real behaviour, but it was removed upstream. sha1cd used to self-register — init() { crypto.RegisterHash(crypto.SHA1, New) } in both sha1cd.go and cgo/sha1.go — and pjbgf/sha1cd#206 deleted it, merged 2026-04-27 and released in v0.6.0. Rationale given: "This library is primarily used by go-git, which does not rely on the crypto package's registration mechanism… Therefore, it will no longer automatically self-register itself." So v0.5.0 and earlier would have done what you describe; the v0.6.0 this PR pins does not.

Which makes the import-order approach inert now: go list -deps over all of sha1cd's packages contains no crypto entry at all, only klauspost/cpuid. Importing it registers nothing, so it can't become the last registered handler. And init order is dependency order, not source import order regardless — crypto/sha1 is a transitive dep of crypto/x509, which this file imports, so its init() runs first however the import block is arranged.

The history is also the strongest argument against building on this. melange#2357 pinned sha1cd v0.4.0, which did self-register, so melange was overriding crypto.SHA1 process-wide from February onward. That override disappeared when melange moved to v0.6.0 — silently, with no compile error and no failing test. A security property that can evaporate in a routine dependency bump isn't a property you want to depend on.

The custom-init() variant would work — crypto.RegisterHash is hashes[h] = f with no double-registration panic, and an init() in internal/sha1cd runs after its transitive deps. I checked that empirically: the override wins, silently. But that's the argument against it, not for it.

Overriding crypto.SHA1 would break legitimate SHA-1 verification elsewhere in the process. x509 and tls both route SHA-1 through the registry on their signature paths — crypto/x509/x509.go:1009 (h := hashType.New() in checkSignature, crypto.SHA1 for SHA1WithRSA/ECDSAWithSHA1/DSAWithSHA1, so CSRs, CRLs and OCSP), crypto/crypto.go:250 (crypto.SignMessage, reached from x509.go:1599 for signing), and crypto/tls/auth.go:23 (verifyHandshakeSignature, including TLS 1.2's default peer algorithm set). Overriding it and re-verifying an honestly-signed SHA1WithRSA CSR gives crypto/rsa: verification error.

Because sha1cd rehashes colliding input, the damage is shaped badly: those paths behave normally on ordinary input and diverge only on adversarial input — in code that has nothing to do with apk. And it still wouldn't cover the direct callers (x509.go:1755 sha1.Sum for SubjectKeyId, tls/prf.go, tls/key_agreement.go), so you'd get partial coverage plus global breakage.

So I'd keep gosec G505 as the mechanism, and reduce this comment to what's true — something like:

// digestType may be crypto.SHA1, for the legacy apk index signatures that are
// still in the wild. Verification needs only the digest size and algorithm OID,
// not a registered hash: rsa.VerifyPKCS1v15 reaches hash.New() only under
// GODEBUG=fips140=only. Every SHA-1 digest apko *computes* goes through
// chainguard.dev/apko/internal/sha1cd, which refuses digests of input bearing
// the signature of a collision attack; gosec G505 is what keeps crypto/sha1
// from being reintroduced.

On the stable-mitigated-digest worry — real critique of sha1cd, and I read it as an argument for this PR's shape. Because Sum returns ErrCollision instead of the digest, apko never uses the mitigated value, so it can't sign or verify over one; melange does the same in #2357, failing the build rather than emitting it. It's also the sharpest reason not to override the registry, since that would hand mitigated digests to libraries that never asked.

On PKCS#1 v1.5 vs PSS — v1.5 already binds the hash algorithm, via the DigestInfo prefix carrying the algorithm OID (hashPrefixes["SHA-1"] decodes to 1.3.14.3.2.26). One wrinkle: Go rejects a SHA-1-vs-SHA-256 mixup on the digest-length pre-check before it ever compares prefixes, so demonstrating the OID binding needs a same-length pair — SHA-256 vs SHA-512/256 reaches the prefix comparison and fails there. Either way the binding exists, so PSS wouldn't add it.

What's actually missing is a binding from key to permitted algorithm: apk reuses one key for .SIGN.RSA.<key> and .SIGN.RSA256.<key>, so a key can't be declared SHA-256-only. That's an apk format and keyring gap, and it's why a publisher who has moved to SHA-256 stays exposed while still emitting an RSA/SHA-1 signature clients accept — the cross-algorithm rejection point you +1'd above: an index whose SHA-1 signature verifies while its SHA-256 signature fails has no benign explanation, and rejecting it catches any collision rather than the 32 disturbance vectors in sha1cd's table.

[stevebeattie]

Worth calling out a behaviour change here that isn't in the PR description — though on inspection it's smaller than it first looks, and this code is the right response to it.

Adopting sha1cd means any package that legitimately ships a known collision blob — sha1collisiondetection or hashclash test data, the SHAttered PDFs in a PDF or security-training corpus, AV/YARA sample sets — becomes uninstallable. That's true independently of the ErrCollision return, because sha1cd rehashes colliding input to a different digest. For the fixture in this PR:

crypto/sha1 : f92d74e3874587aaf443d1db961d4e26dde13e9c
sha1cd      : 7117b3cb9225aaf0d8ef1a40e493957b0bf8693d

A package built by abuild/apk-tools carries the first value in APK-TOOLS.checksum.SHA1, so without this check the failure would surface as checksum mismatch: … header was f92d74e3…, computed 7117b3cb… — accurate but baffling. Returning ErrCollision turns that into a real diagnosis, so this is an improvement rather than the cause. Two asks:

1. A line in the PR description. "A package containing a known SHA-1 collision blob can no longer be installed" is user-visible, and the cause isn't obvious from a build failure. Worth stating next to the Scope section.
2. Is an escape hatch wanted? There's no opt-out and the failure is permanent for that package — a re-fetch hits the same detector, and isRetryableError is allowlist-based so it correctly declines to retry rather than looping. Probability is low: sha1cd tests 32 specific disturbance vectors rather than heuristics, so false positives on non-adversarial input are negligible, and Wolfi packages are generally stripped of test data. Worth noting this site is defence-in-depth either way — the data section is already authenticated by the signed index → control-section digest → datahash (SHA-256) chain, so the per-file SHA-1 tag isn't the trust anchor here. Your call whether that adds up to a flag.

[xnox]

Note that melange which creates .apks for us has had sha1cd implementation since February this year - intentionally to ensure as many APK-TOOLS.checksum.SHA1 are already known to be sha1cd safe, see:

• tarball: switch from crypto/sha1 to sha1cd melange#2357

Anybody who legitimately wants to ship collision blobs; should encode them in some other format - like for example the hex-encoded representation used in this test suite.

Note git also uses sha1cd - and rejects storing collision blobs outright.

[stevebeattie]

This passthrough silently permits the exact mistake the package exists to prevent:

sha1cd.Sum(sha1.New())  // returns a digest and a nil error; no collision check, no signal

*sha1.digest isn't a CollisionResistantHash, so it takes the !ok branch and comes back looking like a checked digest. The doc comment above asks callers to use SumBytes where the algorithm is known statically, but that's a convention, and nothing enforces it. Given that crypto.SHA1 is still registered in every apko build (see my comment on rsa.go) and gosec G505 is both //nolint-suppressible and excluded for _test.go, this is the whole guarantee resting on documentation.

The var h hash.Hash = sha1cd.New() pattern at expandapk.go:456 is also the only site that actually needs the polymorphism, and even there the two possibilities are known at compile time.

Cheap fix that keeps the sha256 path working — reject any non-detecting hash of SHA-1's size:

crh, ok := h.(upstream.CollisionResistantHash)
if !ok {
	if h.Size() == Size {
		return nil, fmt.Errorf("refusing to finalise a %d-byte hash that cannot detect collisions", Size)
	}
	return h.Sum(nil), nil
}

That turns a silent bypass into a loud one at the only place it can happen, and costs nothing for sha256.

Relatedly: TestSumPassesThroughOtherHashes pins the positive case, but there's no negative case asserting Sum refuses a collision-blind SHA-1 — which is the behaviour that matters here.

[xnox]

..... and the package was created in the first place because "casts" were assumed to be ugly.

I actually originally wanted to directly import sha1cd library, and indeed cast each hash explicitely. Cause imho that's a better way than an arbitrary internal indirection - which is not obvious directly. Like it was done in melange before at https://github.com/chainguard-dev/melange/pull/2357/changes

Maybe i should rewrite this code with direct sha1cd usage.

[stevebeattie]

Behaviour change worth noting: this aborts the whole index fetch, where the old code fell through to the next signature on a per-signature failure. Since indexDigest memoizes per algorithm, the only way to reach it is a SHA-1 collision on a SHA-1-signed index — and no repo currently dual-signs across algorithms (Wolfi and apk.cgr.dev are RSA256-only, three keys on the latter; Alpine v3.21/v3.22/edge are RSA-only), so with nothing to fall through to, continue and return reach the same outcome today. Still marginally prefer continue alongside the Warnf already in the else branch below, for consistency with how every other per-signature failure is handled.

Separately, and probably its own PR rather than this one: an index where the SHA-1 signature verifies but the SHA-256 signature fails should be rejected outright, and breaking on first success currently accepts it. That asymmetry has no benign explanation — a SHA-1 collision preserves the SHA-1 signature and necessarily breaks the SHA-256 one, while corruption would break both — which makes it a strictly better collision detector than counter-cryptanalysis, catching any collision rather than the 32 disturbance vectors in sha1cd's table. It's inert against the current state of the Chainguard and Alpine indexes per above, so nothing regresses and nothing improves today; the value is having it in place before Alpine adds RSA256 alongside RSA, which is exactly when the attack becomes attractive.