From 34b71c863090f9cd652a6752892b5e7562cebbe0 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Oct 2022 14:59:24 +0200 Subject: [PATCH 1/7] Implement encoding proof node values --- bin/fuzz/Cargo.lock | 118 +++++++++--------- bin/fuzz/fuzz_targets/proof-node-decode.rs | 15 ++- bin/light-base/src/runtime_service.rs | 2 +- bin/light-base/src/sync_service.rs | 2 +- bin/wasm-node/CHANGELOG.md | 1 + src/trie.rs | 2 +- src/trie/nibble.rs | 46 ++++++- src/trie/proof_node_decode.rs | 133 ++++++++++++++++++++- src/util.rs | 17 +++ 9 files changed, 267 insertions(+), 69 deletions(-) diff --git a/bin/fuzz/Cargo.lock b/bin/fuzz/Cargo.lock index 7c3a2ce6bf..aea905a409 100644 --- a/bin/fuzz/Cargo.lock +++ b/bin/fuzz/Cargo.lock @@ -265,7 +265,7 @@ version = "0.10.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b9cf849ee05b2ee5fba5e36f97ff8ec2533916700fc0758d40d92136a42f3388" dependencies = [ - "digest 0.10.3", + "digest 0.10.5", ] [[package]] @@ -436,19 +436,21 @@ dependencies = [ [[package]] name = "cranelift-bforest" -version = "0.87.1" +version = "0.88.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9f91425bea5a5ac6d76b788477064944a7e21f0e240fd93f6f368a774a3efdd1" +checksum = "44409ccf2d0f663920cab563d2b79fcd6b2e9a2bcc6e929fef76c8f82ad6c17a" dependencies = [ "cranelift-entity", ] [[package]] name = "cranelift-codegen" -version = "0.87.1" +version = "0.88.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8b83b4bbf7bc96db77b7b5b5e41fafc4001536e9f0cbfd702ed7d4d8f848dc06" +checksum = "98de2018ad96eb97f621f7d6b900a0cc661aec8d02ea4a50e56ecb48e5a2fcaf" dependencies = [ + "arrayvec 0.7.2", + "bumpalo", "cranelift-bforest", "cranelift-codegen-meta", "cranelift-codegen-shared", @@ -463,33 +465,33 @@ dependencies = [ [[package]] name = "cranelift-codegen-meta" -version = "0.87.1" +version = "0.88.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "da02e8fff048c381b313a3dfef4deb2343976fb6d7acc8e7d9c86d4c93e3fa06" +checksum = "5287ce36e6c4758fbaf298bd1a8697ad97a4f2375a3d1b61142ea538db4877e5" dependencies = [ "cranelift-codegen-shared", ] [[package]] name = "cranelift-codegen-shared" -version = "0.87.1" +version = "0.88.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9abc2a06e8fc29e36660ebbc9e2503e18a051057072acbb1e75e7f7cf19cb95e" +checksum = "2855c24219e2f08827f3f4ffb2da92e134ae8d8ecc185b11ec8f9878cf5f588e" [[package]] name = "cranelift-entity" -version = "0.87.1" +version = "0.88.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "aeced7874890fc25d85cacc5e626c4d67931c7c25aad1c2ad521684744c1ff5c" +checksum = "0b65673279d75d34bf11af9660ae2dbd1c22e6d28f163f5c72f4e1dc56d56103" dependencies = [ "serde", ] [[package]] name = "cranelift-frontend" -version = "0.87.1" +version = "0.88.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fc1d301ccad6fce05d9c9793d433d225fafdd57661b98d268d8d162e9291ff2e" +checksum = "3ed2b3d7a4751163f6c4a349205ab1b7d9c00eecf19dcea48592ef1f7688eefc" dependencies = [ "cranelift-codegen", "log", @@ -499,15 +501,15 @@ dependencies = [ [[package]] name = "cranelift-isle" -version = "0.87.1" +version = "0.88.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bd7b100db19320848986b4df1da19501dbddeb706a799f502222f72f889b0fab" +checksum = "3be64cecea9d90105fc6a2ba2d003e98c867c1d6c4c86cc878f97ad9fb916293" [[package]] name = "cranelift-native" -version = "0.87.1" +version = "0.88.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7be18d8b976cddc822e52343f328b7593d26dd2f1aeadd90da071596a210d524" +checksum = "c4a03a6ac1b063e416ca4b93f6247978c991475e8271465340caa6f92f3c16a4" dependencies = [ "cranelift-codegen", "libc", @@ -516,9 +518,9 @@ dependencies = [ [[package]] name = "cranelift-wasm" -version = "0.87.1" +version = "0.88.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2f9e48bb632a2e189b38a9fa89fa5a6eea687a5a4c613bbef7c2b7522c3ad0e0" +checksum = "c699873f7b30bc5f20dd03a796b4183e073a46616c91704792ec35e45d13f913" dependencies = [ "cranelift-codegen", "cranelift-entity", @@ -667,9 +669,9 @@ dependencies = [ [[package]] name = "digest" -version = "0.10.3" +version = "0.10.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f2fb860ca6fafa5552fb6d0e816a69c8e49f0908bf524e30a90d97c85892d506" +checksum = "adfbc57365a37acbd2ebf2b64d7e69bb766e2fea813521ed536f5d0520dcf86c" dependencies = [ "block-buffer 0.10.2", "crypto-common", @@ -976,7 +978,7 @@ version = "0.12.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6c49c37c09c17a53d937dfbb742eb3a961d65a994e6bcdcf37e7399d0cc8ab5e" dependencies = [ - "digest 0.10.3", + "digest 0.10.5", ] [[package]] @@ -1035,9 +1037,9 @@ checksum = "1ea37f355c05dde75b84bba2d767906ad522e97cd9e2eef2be7a4ab7fb442c06" [[package]] name = "itertools" -version = "0.10.3" +version = "0.10.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a9a9d19fa1e79b6215ff29b9d6880b706147f16e9b1dbb1e4e5947b5b02bc5e3" +checksum = "b0fd2260e829bddf4cb6ea802289de2f86d6a7a690192fbe91b3f46e0f2c8473" dependencies = [ "either", ] @@ -1343,9 +1345,9 @@ checksum = "624a8340c38c1b80fd549087862da4ba43e08858af025b236e509b6649fc13d5" [[package]] name = "parity-scale-codec" -version = "3.1.5" +version = "3.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9182e4a71cae089267ab03e67c99368db7cd877baf50f931e5d6d4b71e195ac0" +checksum = "366e44391a8af4cfd6002ef6ba072bae071a96aafca98d7d448a34c5dca38b6a" dependencies = [ "arrayvec 0.7.2", "byte-slice-cast", @@ -1412,7 +1414,7 @@ version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "83a0692ec44e4cf1ef28ca317f14f8f07da2d95ec3fa01f86e4467b725e60917" dependencies = [ - "digest 0.10.3", + "digest 0.10.5", ] [[package]] @@ -1674,18 +1676,18 @@ checksum = "8cb243bdfdb5936c8dc3c45762a19d12ab4550cdc753bc247637d4ec35a040fd" [[package]] name = "serde" -version = "1.0.144" +version = "1.0.145" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0f747710de3dcd43b88c9168773254e809d8ddbdf9653b84e2554ab219f17860" +checksum = "728eb6351430bccb993660dfffc5a72f91ccc1295abaa8ce19b27ebe4f75568b" dependencies = [ "serde_derive", ] [[package]] name = "serde_derive" -version = "1.0.144" +version = "1.0.145" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "94ed3a816fb1d101812f83e789f888322c34e291f894f19590dc310963e87a00" +checksum = "81fa1584d3d1bcacd84c277a0dfe21f5b0f6accf4a23d04d4c6d61f1af522b4c" dependencies = [ "proc-macro2", "quote", @@ -1731,13 +1733,13 @@ dependencies = [ [[package]] name = "sha2" -version = "0.10.5" +version = "0.10.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cf9db03534dff993187064c4e0c05a5708d2a9728ace9a8959b77bedf415dac5" +checksum = "82e6b795fe2e3b1e845bafcb27aa35405c4d47cdfc92af5fc8d3002f76cebdc0" dependencies = [ "cfg-if", "cpufeatures", - "digest 0.10.3", + "digest 0.10.5", ] [[package]] @@ -1763,9 +1765,9 @@ checksum = "03b634d87b960ab1a38c4fe143b508576f075e7c978bfad18217645ebfdfa2ec" [[package]] name = "smallvec" -version = "1.9.0" +version = "1.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2fd0db749597d91ff862fd1d55ea87f7855a744a8425a64695b6fca237d1dad1" +checksum = "a507befe795404456341dfab10cef66ead4c041f62b8b11bbb92bffe5d0953e0" [[package]] name = "smoldot" @@ -1806,7 +1808,7 @@ dependencies = [ "schnorrkel", "serde", "serde_json", - "sha2 0.10.5", + "sha2 0.10.6", "siphasher", "slab", "smallvec", @@ -1842,7 +1844,7 @@ dependencies = [ "curve25519-dalek 4.0.0-pre.1", "rand_core 0.6.3", "rustc_version", - "sha2 0.10.5", + "sha2 0.10.6", "subtle", ] @@ -2156,18 +2158,18 @@ dependencies = [ [[package]] name = "wasmparser" -version = "0.88.0" +version = "0.89.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fb8cf7dd82407fe68161bedcd57fde15596f32ebf6e9b3bdbf3ae1da20e38e5e" +checksum = "ab5d3e08b13876f96dd55608d03cd4883a0545884932d5adf11925876c96daef" dependencies = [ "indexmap", ] [[package]] name = "wasmtime" -version = "0.40.1" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a020a3f6587fa7a7d98a021156177735ebb07212a6239a85ab5f14b2f728508f" +checksum = "f1f511c4917c83d04da68333921107db75747c4e11a2f654a8e909cc5e0520dc" dependencies = [ "anyhow", "async-trait", @@ -2193,18 +2195,18 @@ dependencies = [ [[package]] name = "wasmtime-asm-macros" -version = "0.40.1" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fed4ada1fdd4d9a2aa37be652abcc31ae3188ad0efcefb4571ef4f785be2d777" +checksum = "39bf3debfe744bf19dd3732990ce6f8c0ced7439e2370ba4e1d8f5a3660a3178" dependencies = [ "cfg-if", ] [[package]] name = "wasmtime-cranelift" -version = "0.40.1" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1fc59c28fe895112db09e262fb9c483f9e7b82c78a82a6ded69567ccc0e9795b" +checksum = "058217e28644b012bdcdf0e445f58d496d78c2e0b6a6dd93558e701591dad705" dependencies = [ "anyhow", "cranelift-codegen", @@ -2223,9 +2225,9 @@ dependencies = [ [[package]] name = "wasmtime-environ" -version = "0.40.1" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "11086e573d2635a45ac0d44697a8e4586e058cf1b190f76bea466ca2ec36c30a" +checksum = "c7af06848df28b7661471d9a80d30a973e0f401f2e3ed5396ad7e225ed217047" dependencies = [ "anyhow", "cranelift-entity", @@ -2242,9 +2244,9 @@ dependencies = [ [[package]] name = "wasmtime-fiber" -version = "0.40.1" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6e27d519024f462fb69cc1733184c3e35f60982a6b5a04e940da7ce1a6c2c47a" +checksum = "18fc45a6497f557382fc19b8782ad5d47ce3fced469bdaf303ec6b5b2e83c1a6" dependencies = [ "cc", "cfg-if", @@ -2255,9 +2257,9 @@ dependencies = [ [[package]] name = "wasmtime-jit" -version = "0.40.1" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d5444a78b74144718633f8642eccd7c4858f4c6f0c98ae6a3668998adf177ba2" +checksum = "9028fb63a54185b3c192b7500ef8039c7bb8d7f62bfc9e7c258483a33a3d13bb" dependencies = [ "addr2line", "anyhow", @@ -2279,18 +2281,18 @@ dependencies = [ [[package]] name = "wasmtime-jit-debug" -version = "0.40.1" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c2bf6a667d2a29b2b0ed42bcf7564f00c595d92c24acb4d241c7c4d950b1910c" +checksum = "25e82d4ef93296785de7efca92f7679dc67fe68a13b625a5ecc8d7503b377a37" dependencies = [ "once_cell", ] [[package]] name = "wasmtime-runtime" -version = "0.40.1" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ee064ce7b563cc201cdf3bb1cc4b233f386d8c57a96e55f4c4afe6103f4bd6a1" +checksum = "9f0e9bea7d517d114fe66b930b2124ee086516ee93eeebfd97f75f366c5b0553" dependencies = [ "anyhow", "cc", @@ -2313,9 +2315,9 @@ dependencies = [ [[package]] name = "wasmtime-types" -version = "0.40.1" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "01e104bd9e625181d53ead85910bbc0863aa5f0c6ef96836fe9a5cc65da11b69" +checksum = "69b83e93ed41b8fdc936244cfd5e455480cf1eca1fd60c78a0040038b4ce5075" dependencies = [ "cranelift-entity", "serde", diff --git a/bin/fuzz/fuzz_targets/proof-node-decode.rs b/bin/fuzz/fuzz_targets/proof-node-decode.rs index 53ce1bf796..fd927fade2 100644 --- a/bin/fuzz/fuzz_targets/proof-node-decode.rs +++ b/bin/fuzz/fuzz_targets/proof-node-decode.rs @@ -18,5 +18,18 @@ #![no_main] libfuzzer_sys::fuzz_target!(|data: &[u8]| { - let _ = smoldot::trie::proof_node_decode::decode(data); + if let Ok(decoded) = smoldot::trie::proof_node_decode::decode(data) { + assert_eq!( + smoldot::trie::proof_node_decode::encode(decoded.clone()).fold( + Vec::new(), + |mut a, b| { + a.extend_from_slice(b.as_ref()); + a + } + ), + data, + "{:?}", + decoded + ); + } }); diff --git a/bin/light-base/src/runtime_service.rs b/bin/light-base/src/runtime_service.rs index feca2fa872..e82b5e2a47 100644 --- a/bin/light-base/src/runtime_service.rs +++ b/bin/light-base/src/runtime_service.rs @@ -892,7 +892,7 @@ impl<'a> RuntimeCallLock<'a> { | proof_verify::StorageValue::HashKnownValueMissing(_) ) { assert_eq!(key.len() % 2, 0); - output.push(trie::nibbles_to_bytes_extend(key.iter().copied()).collect::>()); + output.push(trie::nibbles_to_bytes_suffix_extend(key.iter().copied()).collect::>()); } match node_info.children { diff --git a/bin/light-base/src/sync_service.rs b/bin/light-base/src/sync_service.rs index 290a171871..fdcb2ac96c 100644 --- a/bin/light-base/src/sync_service.rs +++ b/bin/light-base/src/sync_service.rs @@ -493,7 +493,7 @@ impl SyncService { protocol::StorageProofRequestConfig { block_hash: *block_hash, keys: prefix_scan.requested_keys().map(|nibbles| { - trie::nibbles_to_bytes_extend(nibbles).collect::>() + trie::nibbles_to_bytes_suffix_extend(nibbles).collect::>() }), }, timeout_per_request, diff --git a/bin/wasm-node/CHANGELOG.md b/bin/wasm-node/CHANGELOG.md index b9feba2265..49c069f35b 100644 --- a/bin/wasm-node/CHANGELOG.md +++ b/bin/wasm-node/CHANGELOG.md @@ -7,6 +7,7 @@ - Syncing no longer stalls if the gap between the finalized and latest block is more than 100 blocks. ([#2801](https://github.com/paritytech/smoldot/pull/2801)) - No longer silently discard justifications when receive a block from the network that was already known locally. ([#2800](https://github.com/paritytech/smoldot/pull/2800)) - CPU-heavy operations such as verifying finality proofs or compiling the runtime will now better respect the CPU rate limit. ([#2803](https://github.com/paritytech/smoldot/pull/2803)) +- Fix some Merkle proofs and SCALE-encoded structures being accepted as correct when they are actually invalid. This is a very minor fix that can presumably not be used as an attack vector. ## 0.7.0 - 2022-09-28 diff --git a/src/trie.rs b/src/trie.rs index 1d2f7b05a8..2cc0e1d19c 100644 --- a/src/trie.rs +++ b/src/trie.rs @@ -98,7 +98,7 @@ pub mod proof_verify; pub mod trie_structure; pub use nibble::{ - all_nibbles, bytes_to_nibbles, nibbles_to_bytes_extend, BytesToNibbles, Nibble, + all_nibbles, bytes_to_nibbles, nibbles_to_bytes_suffix_extend, BytesToNibbles, Nibble, NibbleFromU8Error, }; diff --git a/src/trie/nibble.rs b/src/trie/nibble.rs index 77874a4e2f..bc8cda8158 100644 --- a/src/trie/nibble.rs +++ b/src/trie/nibble.rs @@ -61,7 +61,9 @@ pub fn all_nibbles() -> impl ExactSizeIterator { /// Turns an iterator of nibbles into an iterator of bytes. /// /// If the number of nibbles is uneven, adds a `0` nibble at the end. -pub fn nibbles_to_bytes_extend>(nibbles: I) -> impl Iterator { +pub fn nibbles_to_bytes_suffix_extend>( + nibbles: I, +) -> impl Iterator { struct Iter(I); impl> Iterator for Iter { @@ -87,6 +89,48 @@ pub fn nibbles_to_bytes_extend>(nibbles: I) -> impl I Iter(nibbles) } +/// Turns an iterator of nibbles into an iterator of bytes. +/// +/// If the number of nibbles is uneven, adds a `0` nibble at the beginning. +pub fn nibbles_to_bytes_prefix_extend>( + nibbles: I, +) -> impl ExactSizeIterator { + struct Iter(I, bool); + + impl> Iterator for Iter { + type Item = u8; + + fn next(&mut self) -> Option { + let n1 = if self.1 { + self.1 = false; + Nibble(0) + } else { + self.0.next()? + }; + let n2 = self.0.next()?; + let byte = (n1.0 << 4) | n2.0; + Some(byte) + } + + fn size_hint(&self) -> (usize, Option) { + let inner_len = self.0.len(); + let len = if self.1 { + debug_assert_eq!(inner_len % 2, 1); + (inner_len / 2) + 1 + } else { + debug_assert_eq!(inner_len % 2, 0); + inner_len / 2 + }; + (len, Some(len)) + } + } + + impl> ExactSizeIterator for Iter {} + + let has_prefix_nibble = (nibbles.len() % 2) != 0; + Iter(nibbles, has_prefix_nibble) +} + /// Turns an iterator of bytes into an iterator of nibbles corresponding to these bytes. /// /// For each byte, the iterator yields a nibble containing the 4 most significant bits then a diff --git a/src/trie/proof_node_decode.rs b/src/trie/proof_node_decode.rs index 58e0714ed3..cd012f2d1c 100644 --- a/src/trie/proof_node_decode.rs +++ b/src/trie/proof_node_decode.rs @@ -16,7 +16,106 @@ // along with this program. If not, see . use super::nibble; -use core::{fmt, iter, slice}; +use core::{cmp, fmt, iter, slice}; + +/// Encodes the components of a node value into the node value itself. +/// +/// This function returns an iterator of buffers. The actual node value is the concatenation of +/// these buffers put together. +/// +/// > **Note**: The returned iterator might contain a reference to the storage value and children +/// > values in the [`Decoded`]. By returning an iterator of buffers, we avoid copying +/// > these storage value and children values. +/// +/// This encoding is independent of the trie version. +pub fn encode( + decoded: Decoded<'_>, +) -> impl Iterator + '_ + Clone> + Clone + '_ { + // The return value is composed of three parts: + // - Before the storage value. + // - The storage value (which can be empty). + // - The children nodes. + + // Contains the encoding before the storage value. + let mut before_storage_value: Vec = Vec::with_capacity(decoded.partial_key.len() / 2 + 32); + + let has_children = decoded.children.iter().any(Option::is_some); + + // We first push the node header. + // See https://spec.polkadot.network/#defn-node-header + { + let (first_byte_msb, pk_len_first_byte_bits): (u8, _) = + match (has_children, decoded.storage_value) { + (false, StorageValue::Unhashed(_)) => (0b01, 6), + (true, StorageValue::None) => (0b10, 6), + (true, StorageValue::Unhashed(_)) => (0b11, 6), + (false, StorageValue::Hashed(_)) => (0b001, 5), + (true, StorageValue::Hashed(_)) => (0b0001, 4), + // TODO: it's invalid to have a non-empty partial key in that situation; this isn't problematic in practice + (false, StorageValue::None) => (0, 6), + }; + + let max_representable_in_first_byte = (1 << pk_len_first_byte_bits) - 1; + let first_byte = (first_byte_msb << pk_len_first_byte_bits) + | u8::try_from(cmp::min( + decoded.partial_key.len(), + max_representable_in_first_byte, + )) + .unwrap(); + before_storage_value.push(first_byte); + + // Note that if the partial key length is exactly equal to `pk_len_first_byte_bits`, we + // need to push a `0` afterwards in order to avoid an ambiguity. Similarly, if + // `remain_pk_len` is at any point equal to 255, we must push an additional `0` + // afterwards. + let mut remain_pk_len = decoded + .partial_key + .len() + .checked_sub(max_representable_in_first_byte); + while let Some(pk_len_inner) = remain_pk_len { + before_storage_value.push(u8::try_from(cmp::min(pk_len_inner, 255)).unwrap()); + remain_pk_len = pk_len_inner.checked_sub(255); + } + } + + // We then push the partial key. + before_storage_value.extend(nibble::nibbles_to_bytes_prefix_extend( + decoded.partial_key.clone(), + )); + + // After the partial key, the node value optionally contains a bitfield of child nodes. + if has_children { + before_storage_value.extend_from_slice(&decoded.children_bitmap().to_le_bytes()); + } + + // Then, the storage value. + let storage_value = match decoded.storage_value { + StorageValue::Hashed(hash) => &hash[..], + StorageValue::None => &[][..], + StorageValue::Unhashed(storage_value) => { + before_storage_value.extend_from_slice( + crate::util::encode_scale_compact_usize(storage_value.len()).as_ref(), + ); + storage_value + } + }; + + // Finally, the children node values. + let children_nodes = decoded + .children + .into_iter() + .filter_map(|c| c) + .flat_map(|child_value| { + let size = crate::util::encode_scale_compact_usize(child_value.len()); + [either::Left(size), either::Right(child_value)].into_iter() + }); + + // The return value is the combination of these components. + iter::once(either::Left(before_storage_value)) + .chain(iter::once(either::Right(storage_value))) + .map(either::Left) + .chain(children_nodes.map(either::Right)) +} /// Decodes a node value found in a proof into its components. /// @@ -63,6 +162,12 @@ pub fn decode(mut node_value: &[u8]) -> Result { accumulator }; + // No children and no storage value can only indicate the root of an empty trie, in which case + // a non-empty partial key is invalid. + if pk_len != 0 && !has_children && storage_value_hashed.is_none() { + return Err(Error::EmptyTrieWithPartialKey); + } + // Iterator to the partial key found in the node value of `proof_iter`. let partial_key = { // Length of the partial key, in bytes. @@ -91,6 +196,9 @@ pub fn decode(mut node_value: &[u8]) -> Result { return Err(Error::ChildrenBitmapTooShort); } let val = u16::from_le_bytes(<[u8; 2]>::try_from(&node_value[..2]).unwrap()); + if val == 0 { + return Err(Error::ZeroChildrenBitmap); + } node_value = &node_value[2..]; val } else { @@ -153,8 +261,8 @@ pub fn decode(mut node_value: &[u8]) -> Result { }) } -/// Decoded node value. Returned by [`decode`]. -#[derive(Debug)] +/// Decoded node value. Returned by [`decode`] or passed as parameter to [`encode`]. +#[derive(Debug, Clone)] pub struct Decoded<'a> { /// Iterator to the nibbles of the partial key of the node. pub partial_key: PartialKey<'a>, @@ -260,6 +368,8 @@ pub enum Error { InvalidPartialKeyPadding, /// End of data within the children bitmap. ChildrenBitmapTooShort, + /// The children bitmap is equal to 0 despite the header indicating the presence of children. + ZeroChildrenBitmap, /// Error while decoding length of child. ChildLenDecode, /// Node value ends within a child value. @@ -270,6 +380,9 @@ pub enum Error { StorageValueTooShort, /// Node value is longer than expected. TooLong, + /// Node value indicates that it is the root of an empty trie but contains a non-empty partial + /// key. + EmptyTrieWithPartialKey, } #[cfg(test)] @@ -278,14 +391,22 @@ mod tests { #[test] fn basic() { - let decoded = super::decode(&[ + let encoded_bytes = &[ 194, 99, 192, 0, 0, 128, 129, 254, 111, 21, 39, 188, 215, 18, 139, 76, 128, 157, 108, 33, 139, 232, 34, 73, 0, 21, 202, 54, 18, 71, 145, 117, 47, 222, 189, 93, 119, 68, 128, 108, 211, 105, 98, 122, 206, 246, 73, 77, 237, 51, 77, 26, 166, 1, 52, 179, 173, 43, 89, 219, 104, 196, 190, 208, 128, 135, 177, 13, 185, 111, 175, - ]) - .unwrap(); + ]; + let decoded = super::decode(encoded_bytes).unwrap(); + + assert_eq!( + super::encode(decoded.clone()).fold(Vec::new(), |mut a, b| { + a.extend_from_slice(b.as_ref()); + a + }), + encoded_bytes + ); assert_eq!( decoded.partial_key.collect::>(), vec![ diff --git a/src/util.rs b/src/util.rs index dfa1b6f20a..06eca557b1 100644 --- a/src/util.rs +++ b/src/util.rs @@ -143,6 +143,15 @@ macro_rules! decode_scale_compact { let byte0 = u16::from(bytes[0] >> 2); let byte1 = u16::from(bytes[1]); + + // Value is invalid if highest byte is 0. + if byte1 == 0 { + return Err(nom::Err::Error(nom::error::make_error( + bytes, + nom::error::ErrorKind::Satisfy, + ))) + } + let value = (byte1 << 6) | byte0; Ok((&bytes[2..], <$num_ty>::from(value))) } @@ -164,6 +173,14 @@ macro_rules! decode_scale_compact { let byte2 = u32::from(bytes[2]).checked_shl(14).unwrap(); let byte3 = u32::from(bytes[3]).checked_shl(22).unwrap(); + // Value is invalid if highest byte is 0. + if byte3 == 0 { + return Err(nom::Err::Error(nom::error::make_error( + bytes, + nom::error::ErrorKind::Satisfy, + ))) + } + let value = byte3 | byte2 | byte1 | byte0; let value = match <$num_ty>::try_from(value) { Ok(v) => v, From b402b99bce641440cfb8fc76d57830d7ac331d73 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Oct 2022 15:00:37 +0200 Subject: [PATCH 2/7] proof-node-decode -> proof-node-codec --- bin/fuzz/Cargo.toml | 4 ++-- .../{proof-node-decode.rs => proof-node-codec.rs} | 4 ++-- src/trie.rs | 2 +- .../{proof_node_decode.rs => proof_node_codec.rs} | 4 ++-- src/trie/proof_verify.rs | 12 ++++++------ src/util.rs | 4 ++-- 6 files changed, 15 insertions(+), 15 deletions(-) rename bin/fuzz/fuzz_targets/{proof-node-decode.rs => proof-node-codec.rs} (88%) rename src/trie/{proof_node_decode.rs => proof_node_codec.rs} (99%) diff --git a/bin/fuzz/Cargo.toml b/bin/fuzz/Cargo.toml index f8fb17ebe9..a9617e6b77 100644 --- a/bin/fuzz/Cargo.toml +++ b/bin/fuzz/Cargo.toml @@ -86,8 +86,8 @@ test = false doc = false [[bin]] -name = "proof-node-decode" -path = "fuzz_targets/proof-node-decode.rs" +name = "proof-node-codec" +path = "fuzz_targets/proof-node-codec.rs" test = false doc = false diff --git a/bin/fuzz/fuzz_targets/proof-node-decode.rs b/bin/fuzz/fuzz_targets/proof-node-codec.rs similarity index 88% rename from bin/fuzz/fuzz_targets/proof-node-decode.rs rename to bin/fuzz/fuzz_targets/proof-node-codec.rs index fd927fade2..fb2be5210a 100644 --- a/bin/fuzz/fuzz_targets/proof-node-decode.rs +++ b/bin/fuzz/fuzz_targets/proof-node-codec.rs @@ -18,9 +18,9 @@ #![no_main] libfuzzer_sys::fuzz_target!(|data: &[u8]| { - if let Ok(decoded) = smoldot::trie::proof_node_decode::decode(data) { + if let Ok(decoded) = smoldot::trie::proof_node_codec::decode(data) { assert_eq!( - smoldot::trie::proof_node_decode::encode(decoded.clone()).fold( + smoldot::trie::proof_node_codec::encode(decoded.clone()).fold( Vec::new(), |mut a, b| { a.extend_from_slice(b.as_ref()); diff --git a/src/trie.rs b/src/trie.rs index 2cc0e1d19c..de9153e3b5 100644 --- a/src/trie.rs +++ b/src/trie.rs @@ -93,7 +93,7 @@ mod nibble; pub mod calculate_root; pub mod node_value; pub mod prefix_proof; -pub mod proof_node_decode; +pub mod proof_node_codec; pub mod proof_verify; pub mod trie_structure; diff --git a/src/trie/proof_node_decode.rs b/src/trie/proof_node_codec.rs similarity index 99% rename from src/trie/proof_node_decode.rs rename to src/trie/proof_node_codec.rs index cd012f2d1c..4bb60770a8 100644 --- a/src/trie/proof_node_decode.rs +++ b/src/trie/proof_node_codec.rs @@ -401,7 +401,7 @@ mod tests { let decoded = super::decode(encoded_bytes).unwrap(); assert_eq!( - super::encode(decoded.clone()).fold(Vec::new(), |mut a, b| { + proof_node_codec::encode(decoded.clone()).fold(Vec::new(), |mut a, b| { a.extend_from_slice(b.as_ref()); a }), @@ -416,7 +416,7 @@ mod tests { ); assert_eq!( decoded.storage_value, - super::StorageValue::Unhashed(&[][..]) + proof_node_codec::StorageValue::Unhashed(&[][..]) ); assert_eq!(decoded.children.iter().filter(|c| c.is_some()).count(), 2); diff --git a/src/trie/proof_verify.rs b/src/trie/proof_verify.rs index 8ffcc04e93..3f2a0efe78 100644 --- a/src/trie/proof_verify.rs +++ b/src/trie/proof_verify.rs @@ -45,7 +45,7 @@ //! > corresponding to the storage entries necessary for a certain runtime call. //! -use super::{nibble, proof_node_decode}; +use super::{nibble, proof_node_codec}; use alloc::vec::Vec; @@ -176,7 +176,7 @@ pub fn trie_node_info<'a, 'b>( loop { // Decodes `node_value` into its components. let decoded_node_value = - proof_node_decode::decode(node_value).map_err(Error::InvalidNodeValue)?; + proof_node_codec::decode(node_value).map_err(Error::InvalidNodeValue)?; // Iterating over this partial key, checking if it matches `expected_nibbles_iter`. for nibble in decoded_node_value.partial_key.clone() { @@ -236,7 +236,7 @@ pub fn trie_node_info<'a, 'b>( // The current node (i.e. `node_value`) exactly matches the requested key. return Ok(TrieNodeInfo { storage_value: match decoded_node_value.storage_value { - proof_node_decode::StorageValue::Hashed(hash) => { + proof_node_codec::StorageValue::Hashed(hash) => { // If the node contains a hash, the un-hashed value should also be found // in the proof as a standalone item. match merkle_values.iter().position(|v| v[..] == *hash) { @@ -247,8 +247,8 @@ pub fn trie_node_info<'a, 'b>( } } } - proof_node_decode::StorageValue::Unhashed(v) => StorageValue::Known(v), - proof_node_decode::StorageValue::None => StorageValue::None, + proof_node_codec::StorageValue::Unhashed(v) => StorageValue::Known(v), + proof_node_codec::StorageValue::None => StorageValue::None, }, children: Children::Multiple { children_bitmap: decoded_node_value.children_bitmap(), @@ -316,7 +316,7 @@ pub enum Error { /// One of the node values in the proof has an invalid format. // TODO: indicate which one? complicated because of inline nodes #[display(fmt = "A node of the proof has an invalid format: {}", _0)] - InvalidNodeValue(proof_node_decode::Error), + InvalidNodeValue(proof_node_codec::Error), /// Missing an entry in the proof. #[display( fmt = "An entry is missing from the proof (closest ancestor nibbles: {})", diff --git a/src/util.rs b/src/util.rs index 06eca557b1..fa09e52dbf 100644 --- a/src/util.rs +++ b/src/util.rs @@ -149,7 +149,7 @@ macro_rules! decode_scale_compact { return Err(nom::Err::Error(nom::error::make_error( bytes, nom::error::ErrorKind::Satisfy, - ))) + ))); } let value = (byte1 << 6) | byte0; @@ -178,7 +178,7 @@ macro_rules! decode_scale_compact { return Err(nom::Err::Error(nom::error::make_error( bytes, nom::error::ErrorKind::Satisfy, - ))) + ))); } let value = byte3 | byte2 | byte1 | byte0; From 7b3b3230596cbf6fd68b30a2812a2d12bd9f1a5e Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Oct 2022 15:00:46 +0200 Subject: [PATCH 3/7] Rustfmt --- bin/light-base/src/runtime_service.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bin/light-base/src/runtime_service.rs b/bin/light-base/src/runtime_service.rs index e82b5e2a47..956e21ef28 100644 --- a/bin/light-base/src/runtime_service.rs +++ b/bin/light-base/src/runtime_service.rs @@ -892,7 +892,9 @@ impl<'a> RuntimeCallLock<'a> { | proof_verify::StorageValue::HashKnownValueMissing(_) ) { assert_eq!(key.len() % 2, 0); - output.push(trie::nibbles_to_bytes_suffix_extend(key.iter().copied()).collect::>()); + output.push( + trie::nibbles_to_bytes_suffix_extend(key.iter().copied()).collect::>(), + ); } match node_info.children { From 4d6db2e6d96a3253305231130f97abd8b3e5a084 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Oct 2022 15:04:50 +0200 Subject: [PATCH 4/7] PR number --- bin/wasm-node/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/wasm-node/CHANGELOG.md b/bin/wasm-node/CHANGELOG.md index 49c069f35b..d8765006c1 100644 --- a/bin/wasm-node/CHANGELOG.md +++ b/bin/wasm-node/CHANGELOG.md @@ -7,7 +7,7 @@ - Syncing no longer stalls if the gap between the finalized and latest block is more than 100 blocks. ([#2801](https://github.com/paritytech/smoldot/pull/2801)) - No longer silently discard justifications when receive a block from the network that was already known locally. ([#2800](https://github.com/paritytech/smoldot/pull/2800)) - CPU-heavy operations such as verifying finality proofs or compiling the runtime will now better respect the CPU rate limit. ([#2803](https://github.com/paritytech/smoldot/pull/2803)) -- Fix some Merkle proofs and SCALE-encoded structures being accepted as correct when they are actually invalid. This is a very minor fix that can presumably not be used as an attack vector. +- Fix some Merkle proofs and SCALE-encoded structures being accepted as correct when they are actually invalid. This is a very minor fix that can presumably not be used as an attack vector. ([#2819](https://github.com/paritytech/smoldot/pull/2819)) ## 0.7.0 - 2022-09-28 From d7b3835dbcf5e4dad463814a2f6d2c316170fbd8 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Oct 2022 15:07:53 +0200 Subject: [PATCH 5/7] Fix missing import for no_std support --- src/trie/proof_node_codec.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/trie/proof_node_codec.rs b/src/trie/proof_node_codec.rs index 4bb60770a8..0625da532b 100644 --- a/src/trie/proof_node_codec.rs +++ b/src/trie/proof_node_codec.rs @@ -16,6 +16,7 @@ // along with this program. If not, see . use super::nibble; +use alloc::vec::Vec; use core::{cmp, fmt, iter, slice}; /// Encodes the components of a node value into the node value itself. From b3b5c0f6751865c2ad9af74e59fd0efa6ad977f9 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Oct 2022 15:14:13 +0200 Subject: [PATCH 6/7] My IDE somehow replaced `super::` with `proof_node_codec::` --- src/trie/proof_node_codec.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/trie/proof_node_codec.rs b/src/trie/proof_node_codec.rs index 0625da532b..7da36a22d5 100644 --- a/src/trie/proof_node_codec.rs +++ b/src/trie/proof_node_codec.rs @@ -402,7 +402,7 @@ mod tests { let decoded = super::decode(encoded_bytes).unwrap(); assert_eq!( - proof_node_codec::encode(decoded.clone()).fold(Vec::new(), |mut a, b| { + super::encode(decoded.clone()).fold(Vec::new(), |mut a, b| { a.extend_from_slice(b.as_ref()); a }), @@ -417,7 +417,7 @@ mod tests { ); assert_eq!( decoded.storage_value, - proof_node_codec::StorageValue::Unhashed(&[][..]) + super::StorageValue::Unhashed(&[][..]) ); assert_eq!(decoded.children.iter().filter(|c| c.is_some()).count(), 2); From 383b95ca1e5f6bcc09b97577c4894376347ee311 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 4 Oct 2022 15:43:35 +0200 Subject: [PATCH 7/7] Oops, fix overly refusing --- src/util.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util.rs b/src/util.rs index fa09e52dbf..6db932dcda 100644 --- a/src/util.rs +++ b/src/util.rs @@ -173,8 +173,8 @@ macro_rules! decode_scale_compact { let byte2 = u32::from(bytes[2]).checked_shl(14).unwrap(); let byte3 = u32::from(bytes[3]).checked_shl(22).unwrap(); - // Value is invalid if highest byte is 0. - if byte3 == 0 { + // Value is invalid if value could have been encoded with 2 fewer bytes. + if byte2 == 0 && byte3 == 0 { return Err(nom::Err::Error(nom::error::make_error( bytes, nom::error::ErrorKind::Satisfy,