fix(drive): verify root hash consistency in double-proof identity lookup#3341
Conversation
In verify_full_identity_by_non_unique_public_key_hash_v0, the root hash from the second proof (identity data) was silently discarded on line 76. Only the first proof's root hash was validated against the quorum signature. A malicious full node could serve a legitimate first proof but a stale/historical second proof containing manipulated identity data (different balance, keys, revision), and the light client would accept it as verified. Fix: compare the identity proof's root hash against the public key hash proof's root hash. If they differ, return a CorruptedProof error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3341 +/- ##
============================================
- Coverage 74.12% 74.12% -0.01%
============================================
Files 3296 3296
Lines 276765 276772 +7
============================================
+ Hits 205163 205166 +3
- Misses 71602 71606 +4
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Correct security fix: the double-proof identity lookup was not verifying that both proofs share the same tree state root hash, allowing a malicious full node to serve stale/fabricated identity data in the second proof. The fix properly compares root hashes and rejects mismatches. The unique-key variants (verify_full_identity_by_unique_public_key_hash, verify_full_identities_by_public_key_hashes) are not affected because they use a single proof blob with in_subset = true, so both lookups inherently share the same root hash.
Reviewed commit: 8189534
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/v0/mod.rs`:
- [SUGGESTION] lines 78-85: No test for mismatched root hash rejection
The existing tests use proofs generated from the same Drive instance, so both proofs naturally have matching root hashes and always pass the new check. There is no test that verifies the new guard actually rejects mismatched root hashes.
A negative test would construct an `IdentityAndNonUniquePublicKeyHashDoubleProof` where `identity_proof` comes from a different Drive state than `identity_id_public_key_hash_proof`, then assert that verification returns `ProofError::CorruptedProof`. This is important because the entire point of this PR is catching this mismatch — without a test, a future refactor could regress it silently.
| // Both proofs must come from the same tree state. If the root | ||
| // hashes differ, the identity proof may be from a stale or | ||
| // fabricated state that does not match the quorum-signed root. | ||
| if identity_root_hash != root_hash { | ||
| return Err(Error::Proof(ProofError::CorruptedProof( | ||
| "identity proof root hash does not match the public key hash proof root hash".to_string(), | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No test for mismatched root hash rejection
The existing tests use proofs generated from the same Drive instance, so both proofs naturally have matching root hashes and always pass the new check. There is no test that verifies the new guard actually rejects mismatched root hashes.
A negative test would construct an IdentityAndNonUniquePublicKeyHashDoubleProof where identity_proof comes from a different Drive state than identity_id_public_key_hash_proof, then assert that verification returns ProofError::CorruptedProof. This is important because the entire point of this PR is catching this mismatch — without a test, a future refactor could regress it silently.
source: ['coordinator']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/v0/mod.rs`:
- [SUGGESTION] lines 78-85: No test for mismatched root hash rejection
The existing tests use proofs generated from the same Drive instance, so both proofs naturally have matching root hashes and always pass the new check. There is no test that verifies the new guard actually rejects mismatched root hashes.
A negative test would construct an `IdentityAndNonUniquePublicKeyHashDoubleProof` where `identity_proof` comes from a different Drive state than `identity_id_public_key_hash_proof`, then assert that verification returns `ProofError::CorruptedProof`. This is important because the entire point of this PR is catching this mismatch — without a test, a future refactor could regress it silently.
2 participants
Issue being fixed or feature implemented
Security audit discovered that the double-proof identity lookup (
verify_full_identity_by_non_unique_public_key_hash) does not verify that both proofs come from the same tree state.What was done?
The function uses a two-proof structure (acknowledged as "a hack" in the protobuf definition):
A malicious full node could serve a legitimate first proof but a stale or fabricated second proof containing manipulated identity data (different balance, different public keys, different revision). The light client would accept it as verified because only the first proof's root hash was checked against the quorum signature.
Fix
After verifying the second proof's Merkle consistency, compare its root hash against the first proof's root hash. If they differ, return
ProofError::CorruptedProof. This ensures both proofs must come from the same quorum-signed tree state.How Has This Been Tested?
All 3 existing tests pass:
The existing tests use proofs generated from the same Drive instance, so both proofs naturally have matching root hashes. The fix adds a guard that would reject proofs from different tree states.
Breaking Changes
None. Valid proofs from the same tree state will continue to verify. Only mismatched proofs (which indicate tampering) will now be rejected.
Checklist:
🤖 Generated with Claude Code