Fix NewPSMT fabricated register reads via non-inclusion proof payload injection - #8674
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
ChangesPSMT proof validation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to PSMT proof construction now rejects forged payloads and truncated proofs that could otherwise expose fabricated register state. Valid proof behavior and the affected attack cases are covered, with no remaining merge-blocking risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| // Bind the payload to the node's hash unconditionally, i.e. WITHOUT trusting pr.Inclusion. | ||
| // The proofs originate from a potentially byzantine source (e.g. an execution node's | ||
| // ChunkDataPack), so pr.Inclusion is attacker-controlled and must not gate hash computation: | ||
| // otherwise a non-inclusion proof could carry a fabricated (non-empty) payload while the | ||
| // node's hash stayed at the honest default, making the fabrication invisible to the root | ||
| // check below and served by GetSinglePayload/Get as authentic state. Computing the hash from | ||
| // the payload here mirrors proof.VerifyTrieProof: an empty payload yields the default hash | ||
| // (so honest non-inclusion proofs are unaffected), while any fabricated payload yields a | ||
| // non-default hash that fails the root check. This likewise defeats a duplicate-path | ||
| // overwrite, since the second proof's payload now necessarily changes the node's hash. | ||
| currentNode.hashValue = ledger.ComputeCompactValue(hash.Hash(path), payload.Value(), currentNode.height) |
There was a problem hiding this comment.
This only protects end-of-proof nodes that remain leaves. forceComputeHash recomputes any node with children from its children and discards the payload-derived hashValue, while pathLookUp still serves that node's payload.
A crafted proof with truncated Steps (Steps=0 lands on the root; a short duplicate lands on an interior ancestor) plus one honest proof passes the root check, and GetSinglePayload returns the fabricated payload.
Reject a proof whose terminal node has or gains children, or verify each proof individually (VerifyTrieProof semantics), or only serve payloads from leaf nodes.
| // secPathP plus a duplicate proof for the same path carrying a fabricated payload. The duplicate's | ||
| // payload now necessarily changes P's node hash, so the reconstructed root no longer matches and | ||
| // NewPSMT rejects the batch. | ||
| func TestNewPSMT_RejectsDuplicatePathOverwrite(t *testing.T) { |
There was a problem hiding this comment.
nit: a related variant is untested: an honest inclusion proof for P plus a duplicate non-inclusion proof carrying an EMPTY payload. Pre-fix this passed the root check while blanking a committed register's value; post-fix it is rejected (empty payload yields the default hash, so the root mismatches).
4 participants
NewPSMT conditioned leaf hash computation on pr.Inclusion, which is attacker-controlled. A byzantine EN could supply a ChunkDataPack.Proof with a fabricated payload on a non-inclusion proof (or a duplicate proof for an already-proven path), causing the fabricated value to pass the root check and be served as authentic state by GetSinglePayload/Get.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Tests