From 8af61442dcda07ce42ac206a2220e48c51c52ac3 Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Sun, 13 Oct 2024 16:48:54 -0500 Subject: [PATCH 1/9] Correct doc comment --- src/proofs/tree.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/proofs/tree.rs b/src/proofs/tree.rs index 68abe74..64de4c2 100644 --- a/src/proofs/tree.rs +++ b/src/proofs/tree.rs @@ -116,8 +116,8 @@ impl Tree { } } - /// Attaches the child to the `Tree`'s given side. Panics if there is - /// already a child attached to this side. + /// Attaches the child to the `Tree`'s given side. Returns an error if + /// there is already a child attached to this side. pub(crate) fn attach(&mut self, left: bool, child: Tree) -> Result<()> { if self.child(left).is_some() { return Err(Error::Attach( From 3d5c1bc55b9c3f49cf8f21f644aa58c91de547af Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Sun, 13 Oct 2024 16:50:18 -0500 Subject: [PATCH 2/9] Correct algorithm document --- docs/algorithms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/algorithms.md b/docs/algorithms.md index 63f1d9a..5284174 100644 --- a/docs/algorithms.md +++ b/docs/algorithms.md @@ -29,7 +29,7 @@ node_hash = H(kv_hash, left_child_hash, right_child_hash) Note that the `left_child_hash` and/or `right_child_hash` values may be null since it is possible for the node to have no children or only one child. -In our implementation, the hash function used is Blake2b (chosen for its performance characteristics) but this choice is trivially swappable. +In our implementation, the hash function used is SHA512/256 (SHA512 with output truncated to 256 bits) but this choice is trivially swappable. #### Database Representation From 776a72fed3aec64cf94015fdb4ffa58d5eef5448 Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Sun, 13 Oct 2024 16:51:36 -0500 Subject: [PATCH 3/9] Correct doc comment for hashed key/value length limits --- src/tree/hash.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/tree/hash.rs b/src/tree/hash.rs index e31f2dc..8c387be 100644 --- a/src/tree/hash.rs +++ b/src/tree/hash.rs @@ -14,9 +14,6 @@ pub const NULL_HASH: Hash = [0; HASH_LENGTH]; pub type Hash = [u8; HASH_LENGTH]; /// Hashes a key/value pair. -/// -/// **NOTE:** This will fail if the key is longer than 255 bytes, or the value -/// is longer than 65,535 bytes. pub fn kv_hash(key: &[u8], value: &[u8]) -> Result { let mut hasher = D::new(); hasher.update([0]); From a32ff88753050909edc4c2f903097b2eaaf9f376 Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Sun, 13 Oct 2024 16:55:06 -0500 Subject: [PATCH 4/9] Return error instead of panicking for invalid number of chunks in restore --- src/merk/restore.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/merk/restore.rs b/src/merk/restore.rs index 8477d88..fb39e98 100644 --- a/src/merk/restore.rs +++ b/src/merk/restore.rs @@ -173,8 +173,11 @@ impl Restorer { 0 }; - // FIXME: this one shouldn't be an assert because it comes from a peer - assert_eq!(self.stated_length, chunks_remaining + 1); + if self.stated_length != chunks_remaining + 1 { + return Err(Error::ChunkProcessing( + "Stated length does not match calculated number of chunks".into(), + )); + } // note that these writes don't happen atomically, which is fine here // because if anything fails during the restore process we will just From e4fd299124f93a7db9b279874663719858b54ef6 Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Sun, 13 Oct 2024 17:11:19 -0500 Subject: [PATCH 5/9] Add unit test for stack overflow in malicious trunk proof verification --- src/proofs/chunk.rs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/proofs/chunk.rs b/src/proofs/chunk.rs index 16a6d14..2ab45fd 100644 --- a/src/proofs/chunk.rs +++ b/src/proofs/chunk.rs @@ -268,12 +268,11 @@ pub(crate) fn verify_trunk>>(ops: I) -> Result<(Pr #[cfg(test)] mod tests { - use std::usize; - use super::super::tree::Tree; use super::*; use crate::test_utils::*; use crate::tree::{NoopCommit, PanicSource, Tree as BaseTree}; + use ed::Encode; #[derive(Default)] struct NodeCounts { @@ -467,4 +466,20 @@ mod tests { assert_eq!(counts.hash, 0); assert_eq!(counts.kvhash, 0); } + + #[test] + fn test_verify_height_stack_overflow() { + let height = 5_000u32; + let push_op = |i: u32| Op::Push(Node::KV(i.to_be_bytes().to_vec(), vec![])); + let mut ops = Vec::with_capacity((height * 2) as usize); + ops.push(push_op(0)); + for i in 1..height { + ops.push(push_op(i)); + ops.push(Op::Parent) + } + assert!(ops.encoding_length().unwrap() < 50_000); + println!("Len: {}", ops.encoding_length().unwrap()); + let (_, result_height) = verify_trunk(ops.into_iter().map(Ok)).unwrap(); + assert_eq!(height, result_height as u32); + } } From 097bc8f1cef4d03aaf5bc80b541be67b65b67f40 Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Sun, 13 Oct 2024 17:11:35 -0500 Subject: [PATCH 6/9] Prevent stack overflows in trunk verification --- src/proofs/chunk.rs | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/proofs/chunk.rs b/src/proofs/chunk.rs index 2ab45fd..c659789 100644 --- a/src/proofs/chunk.rs +++ b/src/proofs/chunk.rs @@ -197,17 +197,20 @@ pub(crate) fn verify_leaf>>( #[cfg(feature = "full")] pub(crate) fn verify_trunk>>(ops: I) -> Result<(ProofTree, usize)> { fn verify_height_proof(tree: &ProofTree) -> Result { - Ok(match tree.child(true) { - Some(child) => { - if let Node::Hash(_) = child.tree.node { - return Err(Error::UnexpectedNode( - "Expected height proof to only contain KV and KVHash nodes".into(), - )); - } - verify_height_proof(&child.tree)? + 1 + let mut height = 1; + let mut cursor = tree; + while let Some(child) = cursor.child(true) { + if let Node::Hash(_) = child.tree.node { + return Err(Error::UnexpectedNode( + "Expected height proof to only contain KV and KVHash + nodes" + .into(), + )); } - None => 1, - }) + height += 1; + cursor = &child.tree; + } + Ok(height) } fn verify_completeness(tree: &ProofTree, remaining_depth: usize, leftmost: bool) -> Result<()> { @@ -253,6 +256,11 @@ pub(crate) fn verify_trunk>>(ops: I) -> Result<(Pr })?; let height = verify_height_proof(&tree)?; + if height > 64 { + // This is a sanity check to prevent stack overflows in `verify_completeness`, + // but any tree above 64 is probably an error (~3.7e19 nodes). + return Err(Error::Tree("Tree is too large".into())); + } let trunk_height = height / 2; if trunk_height < MIN_TRUNK_HEIGHT { From 61e0018196d2eff0dbb254c4043c42257cb75c2a Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Fri, 18 Oct 2024 16:57:43 -0500 Subject: [PATCH 7/9] Fix stack overflow test --- src/proofs/chunk.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/proofs/chunk.rs b/src/proofs/chunk.rs index c659789..df05328 100644 --- a/src/proofs/chunk.rs +++ b/src/proofs/chunk.rs @@ -476,6 +476,7 @@ mod tests { } #[test] + #[should_panic(expected = "Tree is too large")] fn test_verify_height_stack_overflow() { let height = 5_000u32; let push_op = |i: u32| Op::Push(Node::KV(i.to_be_bytes().to_vec(), vec![])); From 65152283844572add475b3f4f89a812596960067 Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Fri, 18 Oct 2024 16:58:11 -0500 Subject: [PATCH 8/9] Prevent attaching children to Hash nodes in proofs --- src/proofs/tree.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/proofs/tree.rs b/src/proofs/tree.rs index 64de4c2..b161d10 100644 --- a/src/proofs/tree.rs +++ b/src/proofs/tree.rs @@ -125,6 +125,10 @@ impl Tree { )); } + if let Node::Hash(_) = self.node { + return Err(Error::Attach("Tried to attach to Hash node".into())); + } + self.height = self.height.max(child.height + 1); let hash = child.hash()?; From 770782d385e47fdf3a783d429ce438e1c275aad3 Mon Sep 17 00:00:00 2001 From: Matt Bell Date: Fri, 18 Oct 2024 16:59:50 -0500 Subject: [PATCH 9/9] Add test for attaching to hash nodes --- src/proofs/query/mod.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/proofs/query/mod.rs b/src/proofs/query/mod.rs index 2dc7e73..2bd2ad7 100644 --- a/src/proofs/query/mod.rs +++ b/src/proofs/query/mod.rs @@ -474,6 +474,7 @@ mod test { use super::*; use crate::test_utils::make_tree_seq; use crate::tree::{NoopCommit, PanicSource, RefWalker, Tree}; + use ed::Encode; fn make_3_node_tree() -> Result { let mut tree = Tree::new(vec![5], vec![5])? @@ -1477,4 +1478,18 @@ mod test { let _result = verify_query(bytes.as_slice(), &query, [42; 32]).expect("verify failed"); } + + #[test] + #[should_panic(expected = "Tried to attach to Hash node")] + fn hash_attach() { + let mut target = make_3_node_tree().expect("tree construction failed"); + + let mut proof = Vec::new(); + proof.push(Op::Push(Node::KV(vec![42], vec![42]))); + proof.push(Op::Push(Node::Hash(target.hash()))); + proof.push(Op::Parent); + + let map = verify(&proof.encode().unwrap(), target.hash()).unwrap(); + assert_eq!(map.get(&[42]).unwrap().unwrap(), &[42]) + } }