Detect SHA-1 collisions in every digest apk relies on #2407
+589
−54
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| // Copyright 2026 Chainguard, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| // Package sha1cd computes SHA-1 digests with collision detection. | ||
| // | ||
| // apk identifies control sections, signatures and individual files by SHA-1, | ||
| // and signs legacy indexes over SHA-1, so we cannot stop computing it. What we | ||
| // can do is refuse to trust a digest computed over input that carries the | ||
| // cryptanalytic signature of a SHA-1 collision attack. Callers finalise a hash | ||
| // with [Sum], which runs that check and reports [ErrCollision] instead of | ||
| // returning a digest that must not be used. | ||
| // | ||
| // For input with no collision signature the digests are identical to | ||
| // crypto/sha1, so this is a drop-in replacement. For input that does collide | ||
| // the digest differs (the underlying implementation rehashes to a safe value), | ||
| // which is a second reason never to use a digest without checking first. | ||
| package sha1cd | ||
|
|
||
| import ( | ||
| "errors" | ||
| "hash" | ||
|
|
||
| upstream "github.com/pjbgf/sha1cd" | ||
| ) | ||
|
|
||
| // Size is the length in bytes of a SHA-1 digest. | ||
| const Size = upstream.Size | ||
|
|
||
| // ErrCollision reports that the hashed input exhibits the characteristics of a | ||
| // SHA-1 collision attack, so its digest cannot be trusted to identify content. | ||
| var ErrCollision = errors.New("sha1 collision attack detected in hashed input") | ||
|
|
||
| // New returns a [hash.Hash] computing SHA-1 with collision detection. | ||
| // | ||
| // Finalise it with [Sum] rather than its own Sum method: collision detection is | ||
| // only conclusive once the digest is finalised, and Sum is what checks it. | ||
| func New() hash.Hash { | ||
| return upstream.New() | ||
| } | ||
|
|
||
| // Sum finalises h and returns its digest. | ||
| // | ||
| // When h came from [New], the digest is checked before it is returned and | ||
| // ErrCollision is reported instead if the input collides. | ||
| // | ||
| // A hash that cannot detect collisions, such as sha256, is simply finalised. | ||
| // That passthrough is for callers holding a hash.Hash whose algorithm is only | ||
| // known at runtime, so that whichever hash they end up with is checked if it is | ||
| // SHA-1. Where the algorithm is known at the call site, call [SumBytes] or that | ||
| // digest's own package instead, so the code does not read as though SHA-1 were | ||
| // involved when it is not. | ||
| func Sum(h hash.Hash) ([]byte, error) { | ||
| crh, ok := h.(upstream.CollisionResistantHash) | ||
| if !ok { | ||
| return h.Sum(nil), nil | ||
| } | ||
| sum, collision := crh.CollisionResistantSum(nil) | ||
| if collision { | ||
| return nil, ErrCollision | ||
| } | ||
| return sum, nil | ||
| } | ||
|
|
||
| // SumBytes returns the SHA-1 of data, or ErrCollision if data exhibits the | ||
| // characteristics of a SHA-1 collision attack. | ||
| func SumBytes(data []byte) ([]byte, error) { | ||
| sum, collision := upstream.Sum(data) | ||
| if collision { | ||
| return nil, ErrCollision | ||
| } | ||
| return sum[:], nil | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| // Copyright 2026 Chainguard, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package sha1cd | ||
|
|
||
| import ( | ||
| "crypto/sha256" | ||
| "encoding/hex" | ||
| "errors" | ||
| "testing" | ||
|
|
||
| "chainguard.dev/apko/internal/sha1cd/sha1cdtest" | ||
| ) | ||
|
|
||
| // Digests of input with no collision signature must match crypto/sha1, so that | ||
| // swapping the implementation does not change any checksum apk cares about. | ||
| var golden = map[string]string{ | ||
| "": "da39a3ee5e6b4b0d3255bfef95601890afd80709", | ||
| "abc": "a9993e364706816aba3e25717850c26c9cd0d89d", | ||
| "hello world": "2aae6c35c94fcfb415dbe95f408b9ce91ee846ed", | ||
| } | ||
|
|
||
| func TestSumBytesGolden(t *testing.T) { | ||
| for in, want := range golden { | ||
| sum, err := SumBytes([]byte(in)) | ||
| if err != nil { | ||
| t.Errorf("SumBytes(%q): %v", in, err) | ||
| continue | ||
| } | ||
| if got := hex.EncodeToString(sum); got != want { | ||
| t.Errorf("SumBytes(%q) = %s, want %s", in, got, want) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestSumGolden(t *testing.T) { | ||
| for in, want := range golden { | ||
| h := New() | ||
| if _, err := h.Write([]byte(in)); err != nil { | ||
| t.Errorf("Write(%q): %v", in, err) | ||
| continue | ||
| } | ||
| sum, err := Sum(h) | ||
| if err != nil { | ||
| t.Errorf("Sum(%q): %v", in, err) | ||
| continue | ||
| } | ||
| if got := hex.EncodeToString(sum); got != want { | ||
| t.Errorf("Sum(%q) = %s, want %s", in, got, want) | ||
| } | ||
| if len(sum) != Size { | ||
| t.Errorf("Sum(%q) is %d bytes, want %d", in, len(sum), Size) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestSumBytesCollision(t *testing.T) { | ||
| sum, err := SumBytes(sha1cdtest.Shattered(t)) | ||
| if !errors.Is(err, ErrCollision) { | ||
| t.Errorf("SumBytes(shattered) = %x, %v; want ErrCollision", sum, err) | ||
| } | ||
| if sum != nil { | ||
| t.Errorf("SumBytes(shattered) returned a digest %x alongside the error", sum) | ||
| } | ||
| } | ||
|
|
||
| // The collision is only detectable once the digest is finalised, so check that | ||
| // it is caught however the input was fed in. | ||
| func TestSumCollision(t *testing.T) { | ||
| data := sha1cdtest.Shattered(t) | ||
|
|
||
| for _, chunk := range []int{1, 7, 64, 320} { | ||
| h := New() | ||
| for i := 0; i < len(data); i += chunk { | ||
| if _, err := h.Write(data[i:min(i+chunk, len(data))]); err != nil { | ||
| t.Fatalf("chunk %d: write: %v", chunk, err) | ||
| } | ||
| } | ||
| sum, err := Sum(h) | ||
| if !errors.Is(err, ErrCollision) { | ||
| t.Errorf("chunk %d: Sum = %x, %v; want ErrCollision", chunk, sum, err) | ||
| } | ||
| if sum != nil { | ||
| t.Errorf("chunk %d: Sum returned a digest %x alongside the error", chunk, sum) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Sum also finalises hashes that cannot detect collisions, so callers choosing a | ||
| // digest algorithm at runtime have a single finalisation path. | ||
| func TestSumPassesThroughOtherHashes(t *testing.T) { | ||
| h := sha256.New() | ||
| if _, err := h.Write([]byte("hello world")); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| sum, err := Sum(h) | ||
| if err != nil { | ||
| t.Fatalf("Sum(sha256): %v", err) | ||
| } | ||
| const want = "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9" | ||
| if got := hex.EncodeToString(sum); got != want { | ||
| t.Errorf("Sum(sha256) = %s, want %s", got, want) | ||
| } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| // Copyright 2026 Chainguard, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| // Package sha1cdtest provides a real SHA-1 collision, shared by the tests that | ||
| // check collision detection actually fires. | ||
| // | ||
| // This cannot be a shattered_test.go: Go refuses to import a package built only | ||
| // from _test.go files ("no non-test Go files"), so a shared fixture has to live | ||
| // in an ordinary file. Three things keep it out of production code anyway. It is | ||
| // under internal/, so nothing outside apko can reach it. [Shattered] takes a | ||
| // [testing.TB], which only a test can supply. And no non-test file imports this | ||
| // package, so it is never linked into any apko binary — `go list -deps` over the | ||
| // commands does not mention it. | ||
| package sha1cdtest | ||
|
|
||
| import ( | ||
| "encoding/hex" | ||
| "testing" | ||
| ) | ||
|
|
||
| // shattered is the first 320 bytes of shattered-1.pdf, the identical-prefix | ||
| // SHA-1 collision published as SHAttered (https://shattered.io). Both halves of | ||
| // that collision share these bytes, and they are what the collision detection | ||
| // recognises, so hashing them must be refused. | ||
| const shattered = "255044462d312e330a25e2e3cfd30a0a0a312030206f626a0a3c3c2f57696474" + | ||
| "682032203020522f4865696768742033203020522f547970652034203020522f" + | ||
| "537562747970652035203020522f46696c7465722036203020522f436f6c6f72" + | ||
| "53706163652037203020522f4c656e6774682038203020522f42697473506572" + | ||
| "436f6d706f6e656e7420383e3e0a73747265616d0affd8fffe00245348412d31" + | ||
| "20697320646561642121212121852fec092339759c39b1a1c63c4c97e1fffe01" + | ||
| "7346dc9166b67e118f029ab621b2560ff9ca67cca8c7f85ba84c79030c2b3de2" + | ||
| "18f86db3a90901d5df45c14f26fedfb3dc38e96ac22fe7bd728f0e45bce046d2" + | ||
| "3c570feb141398bb552ef5a0a82be331fea48037b8b5d71f0e332edf93ac3500" + | ||
| "eb4ddc0decc1a864790c782c76215660dd309791d06bd0af3f98cda4bc4629b1" | ||
|
|
||
| // Shattered returns a fresh copy of the colliding bytes. Hashing them with | ||
| // chainguard.dev/apko/internal/sha1cd reports sha1cd.ErrCollision. | ||
| func Shattered(tb testing.TB) []byte { | ||
| tb.Helper() | ||
|
|
||
| b, err := hex.DecodeString(shattered) | ||
| if err != nil { | ||
| tb.Fatalf("corrupt shattered vector: %v", err) | ||
| } | ||
| if len(b) != 320 { | ||
| tb.Fatalf("shattered vector is %d bytes, want 320", len(b)) | ||
| } | ||
| return b | ||
| } |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This passthrough silently permits the exact mistake the package exists to prevent:
*sha1.digestisn't aCollisionResistantHash, so it takes the!okbranch and comes back looking like a checked digest. The doc comment above asks callers to useSumByteswhere the algorithm is known statically, but that's a convention, and nothing enforces it. Given thatcrypto.SHA1is still registered in every apko build (see my comment onrsa.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 atexpandapk.go:456is 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:
That turns a silent bypass into a loud one at the only place it can happen, and costs nothing for sha256.
Relatedly:
TestSumPassesThroughOtherHashespins the positive case, but there's no negative case assertingSumrefuses a collision-blind SHA-1 — which is the behaviour that matters here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..... 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed the current shape is the problem — but I'd locate it in
Sum's parameter type, not in the wrapper existing. And I think there's a third option that's better than either of the two you're weighing, including better than what I suggested above.Direct usage with explicit casts is worse for exactly this property. melange#2357 is the evidence: the cast and the flag check are repeated per call site, the error string is duplicated, and
sha1cd.New().(sha1cd.CollisionResistantHash)is an unchecked assertion that panics if upstream ever changesNew()'s return type. apko has ~10 sites. There's no chokepoint, so every new site is one forgottenif collisionaway from a silently collision-blind digest — and that's a code-review problem forever rather than a compiler problem once.The v0.6.0 bump sharpens this. Upstream just changed observable behaviour between v0.5.0 and v0.6.0 (pjbgf/sha1cd#206 removing self-registration). Direct usage spreads that coupling across every call site; a wrapper is the one place to absorb it and the one place you'd notice.
But don't take my earlier suggestion either. I proposed a concrete type embedding
upstream.CollisionResistantHash. That has a trap I missed: embedding promotes a collision-blindSum([]byte) []byte, soh.Sum(nil)still compiles and returns7117b3cb…with no error. You'd end up with a packageSum(*Hash) ([]byte, error)and a methodSum([]byte) []byte— same name, opposite safety. Worse than what it fixes. (Embedding also dropsMarshalBinary, and an exported embedded field lets&sha1cd.Hash{}panic or inject an arbitrary implementation.)The third option: don't satisfy
hash.Hashat all. Checking the call sites, every one consumes the hash purely as anio.Writer—io.Copyatinstall.go:47,package_getter.go:314,resolveapk.go:50,expandapk.go:655;io.TeeReaderatinstall.go:93;io.MultiWriteratinstall_test.go:379. The only place that nameshash.Hashisexpandapk.go:456, and only because one variable is reused for the sha256 branch. Nothing needsSum.I built and tested this.
io.Copy,io.TeeReaderand chunked writes all work, collision detection fires,Resetworks,SumBytesis unchanged. And both bypasses become compile errors:No casts at any call site, and the single type assertion lives in
New()where one test covers it.expandapk.go:456needs a small restructure, since it's the one polymorphic site. Something like keeping the writer and the finaliser separate:That also retires the comment at
:454that currently has to explain whysha1cd.Sumis being handed a sha256 hash.One caller does legitimately use the collision-blind path:
checksums_test.go:67takesh.Sum(nil)to get the mitigated digest for the header, which is what makesTestCheckSumsRejectsCollisionprove the bypass is closed — the sharpest test in the PR, so worth preserving. Simplest is to pin it as a constant insha1cdtestnext to the vector (7117b3cb9225aaf0d8ef1a40e493957b0bf8693d), which also means an upstream change to the safe-rehash derivation fails a test instead of passing quietly.TestSumPassesThroughOtherHashesretires with the passthrough.Worth noting the blast radius is contained either way:
internal/sha1cdis only importable fromchainguard.dev/apko/..., so no consumer outside the module can be affected. Of the 8sha1cd.Sumcall sites, 6 already pass a concretesha1cd.New(); onlyexpandapk.go:491/:526— the same variable in one loop — use thehash.Hashpolymorphism.Net effect on my original point: the "no negative test for the passthrough" gap disappears, because there's nothing left to test — the bypass stops compiling. That's the version of "casts are ugly" I'd argue for: one cast, in one constructor, instead of a convention every future call site has to remember.