test: proof forgery exploit tests for KVValueHash and KVRefValueHash#630
Merged
Conversation
…ValueHash Adds two tests demonstrating the KV→KVValueHash proof forgery attack vector and verifying that KV→KVRefValueHash is NOT exploitable. - kv_to_kvvaluehash_forgery_exploit: Confirms that replacing a KV node with KVValueHash (keeping the real value_hash but injecting a fake value) succeeds on the current codebase. The fix for this is in PR #553. - kv_to_kvrefvaluehash_forgery_exploit: Confirms that the same attack with KVRefValueHash fails because KVRefValueHash incorporates H(value) into the tree hash via combine_hash(node_value_hash, H(referenced_value)). Changing the value changes the root hash, so the forgery is detected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds two new forgery-exploit tests to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grovedb/src/tests/proof_coverage_tests.rs`:
- Around line 6013-6014: The tests call GroveDb::verify_query_raw which returns
a recomputed root but does not assert it matches the DB's trusted root; update
the exploit tests (e.g., KVValueHash and KVRefValueHash) to fetch the actual
trusted root via db.root_hash(grove_version) and assert the recomputed root
equals that trusted root (instead of only checking valid_result.is_ok()). Locate
the verification call using GroveDb::verify_query_raw(&proof_bytes, &path_query,
grove_version) and replace or augment the assert to compare the returned root to
db.root_hash(...) so the tests fail when tampering changes the root; apply the
same change to the similar test blocks referenced (around the other ranges) to
ensure all exploit tests check the trusted root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81da2760-670c-42dd-8e8e-cc40f071e1d7
📒 Files selected for processing (1)
grovedb/src/tests/proof_coverage_tests.rs
Comment on lines
+6013
to
+6014
| let valid_result = GroveDb::verify_query_raw(&proof_bytes, &path_query, grove_version); | ||
| assert!(valid_result.is_ok(), "valid proof should verify"); |
Contributor
There was a problem hiding this comment.
Check the trusted root hash in both exploit tests.
verify_query_raw returns a recomputed root; it does not prove the tampered proof still matches the original DB root. As written, the KVValueHash test can pass even if tampering changes the root, and the KVRefValueHash test can fail even when the safe behavior is just “different root, still structurally decodable.” Assert against db.root_hash(...) in both cases.
🛠️ Suggested fix
let proof_bytes = db
.prove_query(&path_query, None, grove_version)
.unwrap()
.expect("should generate proof");
- let valid_result = GroveDb::verify_query_raw(&proof_bytes, &path_query, grove_version);
- assert!(valid_result.is_ok(), "valid proof should verify");
+ let expected_root = db.root_hash(None, grove_version).unwrap().unwrap();
+ let (valid_root, _) =
+ GroveDb::verify_query_raw(&proof_bytes, &path_query, grove_version)
+ .expect("valid proof should verify");
+ assert_eq!(valid_root, expected_root, "valid proof should reproduce trusted root");
let real_element_bytes = Element::new_item(real_value.clone())
.serialize(grove_version)
.unwrap();
@@
let tampered_result =
GroveDb::verify_query_raw(&tampered_proof_bytes, &path_query, grove_version);
// On develop (no fix), this exploit SUCCEEDS — the verifier accepts the
// fake value. The fix (on the PR branch) rejects KVValueHash nodes
// containing item elements at the merk verification level.
match tampered_result {
- Ok((_, results)) => {
+ Ok((tampered_root, results)) => {
+ assert_eq!(
+ tampered_root, expected_root,
+ "KV→KVValueHash is only a real forgery if the trusted root is unchanged"
+ );
let got_fake = results.iter().any(|pkv| {
let element = Element::deserialize(&pkv.value, grove_version);
matches!(element, Ok(Element::Item(v, _)) if v == fake_value)
});
assert!(
@@
let proof_bytes = db
.prove_query(&path_query, None, grove_version)
.unwrap()
.expect("should generate proof");
- // Sanity: the valid proof verifies correctly
- let valid_result = GroveDb::verify_query_raw(&proof_bytes, &path_query, grove_version);
- assert!(valid_result.is_ok(), "valid proof should verify");
+ let expected_root = db.root_hash(None, grove_version).unwrap().unwrap();
+ let (valid_root, _) =
+ GroveDb::verify_query_raw(&proof_bytes, &path_query, grove_version)
+ .expect("valid proof should verify");
+ assert_eq!(valid_root, expected_root, "valid proof should reproduce trusted root");
@@
let tampered_result =
GroveDb::verify_query_raw(&tampered_proof_bytes, &path_query, grove_version);
- assert!(
- tampered_result.is_err(),
- "KV→KVRefValueHash forgery should be rejected because \
- KVRefValueHash incorporates H(value) into the tree hash via \
- combine_hash, so changing the value changes the root hash"
- );
+ match tampered_result {
+ Ok((tampered_root, _)) => assert_ne!(
+ tampered_root, expected_root,
+ "KV→KVRefValueHash should change the recomputed root hash"
+ ),
+ Err(_) => {}
+ }Also applies to: 6065-6091, 6202-6204, 6266-6274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@grovedb/src/tests/proof_coverage_tests.rs` around lines 6013 - 6014, The
tests call GroveDb::verify_query_raw which returns a recomputed root but does
not assert it matches the DB's trusted root; update the exploit tests (e.g.,
KVValueHash and KVRefValueHash) to fetch the actual trusted root via
db.root_hash(grove_version) and assert the recomputed root equals that trusted
root (instead of only checking valid_result.is_ok()). Locate the verification
call using GroveDb::verify_query_raw(&proof_bytes, &path_query, grove_version)
and replace or augment the assert to compare the returned root to
db.root_hash(...) so the tests fail when tampering changes the root; apply the
same change to the similar test blocks referenced (around the other ranges) to
ensure all exploit tests check the trusted root.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #630 +/- ##
===========================================
+ Coverage 90.70% 90.75% +0.05%
===========================================
Files 182 182
Lines 50734 51082 +348
===========================================
+ Hits 46016 46358 +342
- Misses 4718 4724 +6
🚀 New features to boost your workflow:
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
develop(the fix is in PR fix: prevent KV-to-KVValueHash proof value forgery attack (C1) #553)Details
KV→KVValueHash (vulnerable): An attacker replaces a KV node (tag 0x03) with KVValueHash (tag 0x04), keeping the real
value_hashbut injecting a fake value. The merk verifier uses the providedvalue_hashdirectly in the tree hash, so the root hash is unchanged. The fake value passes through to the GroveDB result set.KV→KVRefValueHash (NOT vulnerable): The same attack fails because
KVRefValueHashcomputes its tree hash as:The
H(referenced_value)term means changing the value changes the tree hash, breaking root hash verification. This is a structural defense — no additional check is needed.Test plan
kv_to_kvvaluehash_forgery_exploit— confirms exploit succeeds on developkv_to_kvrefvaluehash_forgery_exploit— confirms exploit fails for KVRefValueHashNote:
kv_to_kvvaluehash_forgery_exploitwill need to be updated when PR #553 merges (the exploit will be blocked).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes