Repository navigation
fix(verify): don't panic on a bundle entry with no hash - #5127
Conversation
bundleHash dereferenced the algorithm and value of a Rekor entry hash without checking them. The hash is optional in all five of these schemas, and unmarshalling does not always populate it: a rekord v0.0.1 entry that carries data.content but no data.hash, and an intoto v0.0.2 entry whose envelope payload is empty, both unmarshal cleanly with a nil hash. The body comes from the bundle attached to the signature, so it is read from the registry before the certificate has been validated and before the signature has been checked. A body that omits the hash therefore crashes the verifier rather than failing verification. Check each level before dereferencing it and return an error instead. Signed-off-by: ashvinctrl <sharmaashvin27@gmail.com>
| return "", "", err | ||
| } | ||
|
|
||
| // The hash is optional in each of these schemas and unmarshalling an entry |
There was a problem hiding this comment.
Can you update these comments to either describe behavior that is not obvious or remove them? The code seems fairly self-explanatory.
Drop the hashFields doc comment and cut the bundleHash comment down to the part that is not visible from the code: the Rekor models mark the hash fields required, but per-type validation does not always enforce it. Signed-off-by: ashvinctrl <sharmaashvin27@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5127 +/- ##
==========================================
+ Coverage 40.10% 45.82% +5.71%
==========================================
Files 155 207 +52
Lines 10044 11846 +1802
==========================================
+ Hits 4028 5428 +1400
- Misses 5530 6418 +888
+ Partials 486 0 -486 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| }, { | ||
| name: "intoto v0.0.2 without content.hash", | ||
| body: `{"apiVersion":"0.0.2","kind":"intoto","spec":{"content":{"envelope":{"payloadType":"application/vnd.in-toto+json","payload":"","signatures":[{"publicKey":"YQ==","sig":"YQ=="}]}}}}`, | ||
| }} |
There was a problem hiding this comment.
These tests are hitting the "No hash found in bundle entry" error because they are missing the whole hash object, but not the "bundle entry hash is missing its algorithm or value" error for missing one value or the other. For complete code coverage do you think those extra cases should be added?
I might also add tests for the other entry types even though you say they're not reachable, for complete code coverage since you changed their error handling.
There was a problem hiding this comment.
Added in 90b2157.
TestBundleHash now has a success case for each of the five entry types. TestBundleHashWithMissingHash covers a missing hash for every type, plus a hash object missing only its algorithm or only its value. Only rekord v0.0.1 and intoto v0.0.2 get far enough to hit the "no hash found" error. Rekor rejects the rest earlier (schema validation, or the type's own validate()), so for those cases the tests only check that an error comes back and nothing panics.
A partial hash object fails Rekor's validation for every entry type, so the "missing its algorithm or value" error can't be reached through bundleHash. I added TestHashFields to cover it directly instead. With these changes, every block in bundleHash and hashFields is covered except the default arm, which was already there before this PR.
Add success cases for all five entry types, missing-hash cases for each type (including partial hash objects, which Rekor rejects before the switch), and a direct test of hashFields for a nil algorithm or value. Signed-off-by: ashvinctrl <sharmaashvin27@gmail.com>
bundleHash dereferences a Rekor entry hash's algorithm and value without checking them. The hash is optional in all five of these schemas and unmarshalling an entry does not always populate it, so a bundle body that omits it panics instead of failing verification.
Two of the arms are reachable with a nil hash today:
The other three are guarded as things stand (hashedrekord v0.0.1 requires data.hash.algorithm through schema validation, intoto v0.0.1 errors on a missing envelope hash, dsse v0.0.1 requires envelopeHash on the committed path). I checked all five and guarded all five anyway, so a schema or validate() change doesn't quietly reintroduce this.
This is worth more than a routine nil check because of where it runs. The body comes from the bundle attached to the signature, and VerifyBundle is called at the top of verifyInternal, before ValidateAndUnpackCertWithIntermediates, before the signature is checked cryptographically, and before ClaimVerifier. compareSigs and comparePublicKey do run first, but they only compare the bundle against the signature artifact being verified, and both sides of that come from the same place. So the panic lands on input that has not been trusted yet. It is a crash, not a verification bypass: cosign never returns a verified result for these inputs.
I reported this through the private advisory process first and that report was closed, so I am opening it here as an ordinary fix.
Two regression tests are included, one on bundleHash directly and one through VerifyImageSignature. Both panic on main and pass with this change.