From 8395e1eb1fee21e488f1e2ae64b166ba4d6cd529 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Mon, 19 Aug 2024 14:40:23 -0300 Subject: [PATCH 1/5] Edit test-cases and detector --- detectors/dynamic-instance-storage/src/lib.rs | 98 +++++++++++++++++++ .../remediated-example/src/lib.rs | 63 ++++++++++++ .../vulnerable-example/src/lib.rs | 43 ++++++++ .../remediated-example/src/lib.rs | 44 +++++++++ .../vulnerable-example/src/lib.rs | 52 ++++++++++ 5 files changed, 300 insertions(+) create mode 100644 detectors/dynamic-instance-storage/src/lib.rs create mode 100644 test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/src/lib.rs create mode 100644 test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/src/lib.rs create mode 100644 test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs create mode 100644 test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs diff --git a/detectors/dynamic-instance-storage/src/lib.rs b/detectors/dynamic-instance-storage/src/lib.rs new file mode 100644 index 00000000..37b19f80 --- /dev/null +++ b/detectors/dynamic-instance-storage/src/lib.rs @@ -0,0 +1,98 @@ +#![feature(rustc_private)] + +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use clippy_utils::diagnostics::span_lint; +use if_chain::if_chain; +use rustc_hir::{ + intravisit::{walk_expr, FnKind, Visitor}, + Body, Expr, ExprKind, FnDecl, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::{Ty, TyKind}; +use rustc_span::{def_id::LocalDefId, Span, Symbol}; +use utils::{get_node_type_opt, is_soroban_storage, SorobanStorageType}; + +const LINT_MESSAGE: &str = "This function may lead to excessive instance storage growth, which could increase execution costs or potentially cause DoS"; + +dylint_linting::impl_late_lint! { + pub DYNAMIC_INSTANCE_STORAGE, + Warn, + LINT_MESSAGE, + DynamicInstanceStorage, + { + name: "Dynamic Instance Storage Analyzer", + long_message: "Detects potential misuse of instance storage that could lead to unnecessary growth or storage-related vulnerabilities.", + severity: "Warning", + help: "https://coinfabrik.github.io/scout-soroban/docs/detectors/dynamic-instance-storage", + vulnerability_class: "Resource Management", + } +} + +#[derive(Default)] +struct DynamicInstanceStorage; + +impl<'tcx> LateLintPass<'tcx> for DynamicInstanceStorage { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: FnKind<'tcx>, + _: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + span: Span, + _: LocalDefId, + ) { + if span.from_expansion() { + return; + } + + let mut storage_warn_visitor = DynamicInstanceStorageVisitor { cx }; + storage_warn_visitor.visit_body(body); + } +} + +struct DynamicInstanceStorageVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, +} + +impl<'a, 'tcx> Visitor<'tcx> for DynamicInstanceStorageVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + if_chain! { + // Detect calls to `set` method + if let ExprKind::MethodCall(path, receiver, args, _) = &expr.kind; + if path.ident.name == Symbol::intern("set"); + // Get the type of the receiver and check if it is an instance storage + if let Some(receiver_ty) = get_node_type_opt(self.cx, &receiver.hir_id); + if is_soroban_storage(self.cx, receiver_ty, SorobanStorageType::Instance) + || is_soroban_storage(self.cx, receiver_ty, SorobanStorageType::Persistent); + // Check if the value being set is a dynamic type + if args.len() >= 2; + if let Some(value_type) = get_node_type_opt(self.cx, &args[1].hir_id); + if is_dynamic_type(self.cx, &value_type); + then { + span_lint(self.cx, DYNAMIC_INSTANCE_STORAGE, expr.span, LINT_MESSAGE) + } + } + + walk_expr(self, expr) + } +} + +fn is_dynamic_type(cx: &LateContext, ty: &Ty) -> bool { + match ty.kind() { + TyKind::Str => true, + TyKind::Slice(_) => true, + TyKind::Dynamic(..) => true, + TyKind::Array(element_ty, _) => is_dynamic_type(cx, element_ty), + TyKind::Adt(adt_def, _) => { + let type_name = cx.tcx.item_name(adt_def.did()); + matches!(type_name.as_str(), "Vec" | "String" | "Map" | "LinkedList") + } + TyKind::RawPtr(ty, _) => is_dynamic_type(cx, ty), + TyKind::Ref(_, ty, _) => is_dynamic_type(cx, ty), + TyKind::Tuple(substs) => substs.iter().any(|ty| is_dynamic_type(cx, &ty)), + _ => false, + } +} diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..db7a3faa --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/src/lib.rs @@ -0,0 +1,63 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, contracttype, Env, Vec}; + +#[contract] +pub struct VectorStorage; + +#[derive(Clone)] +#[contracttype] +pub enum DataKey { + VecElement(u32), +} + +#[contractimpl] +impl VectorStorage { + pub fn store_vector(e: Env, data: Vec) { + for (i, value) in data.iter().enumerate() { + let key = DataKey::VecElement(i as u32); + e.storage().persistent().set(&key, &value); + } + } + + pub fn get_vector(e: Env) -> Vec { + let mut result = Vec::new(&e); + let mut i = 0; + + while let Some(value) = VectorStorage::get_element(e.clone(), i) { + result.push_back(value); + i += 1; + } + + result + } + + pub fn get_element(e: Env, index: u32) -> Option { + let key = DataKey::VecElement(index); + e.storage().persistent().get(&key) + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{vec, Env}; + + #[test] + fn test_vector_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, VectorStorage); + let client = VectorStorageClient::new(&env, &contract_id); + + // When + let test_vec = vec![&env, 1, 2, 3, 4, 5]; + client.store_vector(&test_vec); + + // Then + let retrieved_vec = client.get_vector(); + assert_eq!(test_vec, retrieved_vec); + + assert_eq!(client.get_element(&2), Some(3)); + assert_eq!(client.get_element(&5), None); + } +} diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..f6be86d0 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/src/lib.rs @@ -0,0 +1,43 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, Symbol, Vec}; + +#[contract] +pub struct VectorStorage; + +#[contractimpl] +impl VectorStorage { + pub fn store_vector(e: Env, data: Vec) { + e.storage() + .persistent() + .set(&Symbol::new(&e, "vector_data"), &data); + } + + pub fn get_vector(e: Env) -> Vec { + e.storage() + .persistent() + .get(&Symbol::new(&e, "vector_data")) + .unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{vec, Env}; + + #[test] + fn test_vector_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, VectorStorage); + let client = VectorStorageClient::new(&env, &contract_id); + + // When + let test_vec = vec![&env, 1, 2, 3, 4, 5]; + client.store_vector(&test_vec); + + // Then + let retrieved_vec = client.get_vector(); + assert_eq!(test_vec, retrieved_vec); + } +} diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs new file mode 100644 index 00000000..de7647e6 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs @@ -0,0 +1,44 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, Map, Symbol}; + +#[contract] +pub struct MapStorage; + +#[contractimpl] +impl MapStorage { + pub fn store_map(e: Env, data: Map) { + data.iter().for_each(|(key, value)| { + e.storage().persistent().set(&key, &value); + }); + } + + pub fn get_key(e: Env, key: Symbol) -> i32 { + e.storage().persistent().get(&key).unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{symbol_short, Env, Map}; + + #[test] + fn test_map_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, MapStorage); + let client = MapStorageClient::new(&env, &contract_id); + + // When + let mut test_map = Map::new(&env); + test_map.set(symbol_short!("key1"), 1); + test_map.set(symbol_short!("key2"), 2); + client.store_map(&test_map); + + // Then + let key1 = client.get_key(&symbol_short!("key1")); + let key2 = client.get_key(&symbol_short!("key2")); + assert_eq!(test_map.get(symbol_short!("key1")).unwrap(), key1); + assert_eq!(test_map.get(symbol_short!("key2")).unwrap(), key2); + } +} diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..f81a77f2 --- /dev/null +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs @@ -0,0 +1,52 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, Map, Symbol}; + +#[contract] +pub struct MapStorage; + +#[contractimpl] +impl MapStorage { + pub fn store_map(e: Env, data: Map) { + e.storage() + .persistent() + .set(&Symbol::new(&e, "map_data"), &data); + } + + pub fn get_map(e: Env) -> Map { + e.storage() + .persistent() + .get(&Symbol::new(&e, "map_data")) + .unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{symbol_short, Env, Map}; + + #[test] + fn test_map_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, MapStorage); + let client = MapStorageClient::new(&env, &contract_id); + + // When + let mut test_map = Map::new(&env); + test_map.set(symbol_short!("key1"), 1); + test_map.set(symbol_short!("key2"), 2); + client.store_map(&test_map); + + // Then + let retrieved_map = client.get_map(); + assert_eq!( + test_map.get(symbol_short!("key1")), + retrieved_map.get(symbol_short!("key1")) + ); + assert_eq!( + test_map.get(symbol_short!("key2")), + retrieved_map.get(symbol_short!("key2")) + ); + } +} From bf96775a8321d6cfb8b770050c05858ca5445752 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Mon, 19 Aug 2024 14:43:47 -0300 Subject: [PATCH 2/5] Delete second test-case and edit --- .../remediated-example/src/lib.rs | 41 ++++++++------- .../vulnerable-example/src/lib.rs | 43 +++++++++------ .../remediated-example/Cargo.toml | 16 ------ .../remediated-example/src/lib.rs | 44 ---------------- .../vulnerable-example/Cargo.toml | 16 ------ .../vulnerable-example/src/lib.rs | 52 ------------------- 6 files changed, 47 insertions(+), 165 deletions(-) delete mode 100644 test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/Cargo.toml delete mode 100644 test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs delete mode 100644 test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/Cargo.toml delete mode 100644 test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/src/lib.rs index 354b0abc..de7647e6 100644 --- a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/src/lib.rs +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/src/lib.rs @@ -1,43 +1,44 @@ #![no_std] -use soroban_sdk::{contract, contractimpl, Env, String, Symbol}; +use soroban_sdk::{contract, contractimpl, Env, Map, Symbol}; #[contract] -pub struct StringStorage; +pub struct MapStorage; #[contractimpl] -impl StringStorage { - pub fn store_string(e: Env, data: String) { - e.storage() - .persistent() - .set(&Symbol::new(&e, "string_data"), &data); +impl MapStorage { + pub fn store_map(e: Env, data: Map) { + data.iter().for_each(|(key, value)| { + e.storage().persistent().set(&key, &value); + }); } - pub fn get_string(e: Env) -> String { - e.storage() - .persistent() - .get(&Symbol::new(&e, "string_data")) - .unwrap() + pub fn get_key(e: Env, key: Symbol) -> i32 { + e.storage().persistent().get(&key).unwrap() } } #[cfg(test)] mod test { use super::*; - use soroban_sdk::Env; + use soroban_sdk::{symbol_short, Env, Map}; #[test] - fn test_string_storage() { + fn test_map_storage() { // Given let env = Env::default(); - let contract_id = env.register_contract(None, StringStorage); - let client = StringStorageClient::new(&env, &contract_id); + let contract_id = env.register_contract(None, MapStorage); + let client = MapStorageClient::new(&env, &contract_id); // When - let test_string = String::from_str(&env, "Hello, Soroban!"); - client.store_string(&test_string); + let mut test_map = Map::new(&env); + test_map.set(symbol_short!("key1"), 1); + test_map.set(symbol_short!("key2"), 2); + client.store_map(&test_map); // Then - let retrieved_string = client.get_string(); - assert_eq!(test_string, retrieved_string); + let key1 = client.get_key(&symbol_short!("key1")); + let key2 = client.get_key(&symbol_short!("key2")); + assert_eq!(test_map.get(symbol_short!("key1")).unwrap(), key1); + assert_eq!(test_map.get(symbol_short!("key2")).unwrap(), key2); } } diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/src/lib.rs index 79a36aa0..f81a77f2 100644 --- a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/src/lib.rs +++ b/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/src/lib.rs @@ -1,21 +1,21 @@ #![no_std] -use soroban_sdk::{contract, contractimpl, Env, String, Symbol}; +use soroban_sdk::{contract, contractimpl, Env, Map, Symbol}; #[contract] -pub struct StringStorage; +pub struct MapStorage; #[contractimpl] -impl StringStorage { - pub fn store_string(e: Env, data: String) { +impl MapStorage { + pub fn store_map(e: Env, data: Map) { e.storage() - .instance() - .set(&Symbol::new(&e, "string_data"), &data); + .persistent() + .set(&Symbol::new(&e, "map_data"), &data); } - pub fn get_string(e: Env) -> String { + pub fn get_map(e: Env) -> Map { e.storage() - .instance() - .get(&Symbol::new(&e, "string_data")) + .persistent() + .get(&Symbol::new(&e, "map_data")) .unwrap() } } @@ -23,21 +23,30 @@ impl StringStorage { #[cfg(test)] mod test { use super::*; - use soroban_sdk::Env; + use soroban_sdk::{symbol_short, Env, Map}; #[test] - fn test_string_storage() { + fn test_map_storage() { // Given let env = Env::default(); - let contract_id = env.register_contract(None, StringStorage); - let client = StringStorageClient::new(&env, &contract_id); + let contract_id = env.register_contract(None, MapStorage); + let client = MapStorageClient::new(&env, &contract_id); // When - let test_string = String::from_str(&env, "Hello, Soroban!"); - client.store_string(&test_string); + let mut test_map = Map::new(&env); + test_map.set(symbol_short!("key1"), 1); + test_map.set(symbol_short!("key2"), 2); + client.store_map(&test_map); // Then - let retrieved_string = client.get_string(); - assert_eq!(test_string, retrieved_string); + let retrieved_map = client.get_map(); + assert_eq!( + test_map.get(symbol_short!("key1")), + retrieved_map.get(symbol_short!("key1")) + ); + assert_eq!( + test_map.get(symbol_short!("key2")), + retrieved_map.get(symbol_short!("key2")) + ); } } diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/Cargo.toml b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/Cargo.toml deleted file mode 100644 index fa1d3059..00000000 --- a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/Cargo.toml +++ /dev/null @@ -1,16 +0,0 @@ -[package] -edition = "2021" -name = "dynamic-instance-storage-remediated-3" -version = "0.1.0" - -[lib] -crate-type = ["cdylib"] - -[dependencies] -soroban-sdk = { workspace = true } - -[dev-dependencies] -soroban-sdk = { workspace = true, features = ["testutils"] } - -[features] -testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs deleted file mode 100644 index de7647e6..00000000 --- a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/remediated-example/src/lib.rs +++ /dev/null @@ -1,44 +0,0 @@ -#![no_std] -use soroban_sdk::{contract, contractimpl, Env, Map, Symbol}; - -#[contract] -pub struct MapStorage; - -#[contractimpl] -impl MapStorage { - pub fn store_map(e: Env, data: Map) { - data.iter().for_each(|(key, value)| { - e.storage().persistent().set(&key, &value); - }); - } - - pub fn get_key(e: Env, key: Symbol) -> i32 { - e.storage().persistent().get(&key).unwrap() - } -} - -#[cfg(test)] -mod test { - use super::*; - use soroban_sdk::{symbol_short, Env, Map}; - - #[test] - fn test_map_storage() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, MapStorage); - let client = MapStorageClient::new(&env, &contract_id); - - // When - let mut test_map = Map::new(&env); - test_map.set(symbol_short!("key1"), 1); - test_map.set(symbol_short!("key2"), 2); - client.store_map(&test_map); - - // Then - let key1 = client.get_key(&symbol_short!("key1")); - let key2 = client.get_key(&symbol_short!("key2")); - assert_eq!(test_map.get(symbol_short!("key1")).unwrap(), key1); - assert_eq!(test_map.get(symbol_short!("key2")).unwrap(), key2); - } -} diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/Cargo.toml b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/Cargo.toml deleted file mode 100644 index 5f0d3c3d..00000000 --- a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/Cargo.toml +++ /dev/null @@ -1,16 +0,0 @@ -[package] -edition = "2021" -name = "dynamic-instance-storage-vulnerable-3" -version = "0.1.0" - -[lib] -crate-type = ["cdylib"] - -[dependencies] -soroban-sdk = { workspace = true } - -[dev-dependencies] -soroban-sdk = { workspace = true, features = ["testutils"] } - -[features] -testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs b/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs deleted file mode 100644 index f81a77f2..00000000 --- a/test-cases/dynamic-instance-storage/dynamic-instance-storage-3/vulnerable-example/src/lib.rs +++ /dev/null @@ -1,52 +0,0 @@ -#![no_std] -use soroban_sdk::{contract, contractimpl, Env, Map, Symbol}; - -#[contract] -pub struct MapStorage; - -#[contractimpl] -impl MapStorage { - pub fn store_map(e: Env, data: Map) { - e.storage() - .persistent() - .set(&Symbol::new(&e, "map_data"), &data); - } - - pub fn get_map(e: Env) -> Map { - e.storage() - .persistent() - .get(&Symbol::new(&e, "map_data")) - .unwrap() - } -} - -#[cfg(test)] -mod test { - use super::*; - use soroban_sdk::{symbol_short, Env, Map}; - - #[test] - fn test_map_storage() { - // Given - let env = Env::default(); - let contract_id = env.register_contract(None, MapStorage); - let client = MapStorageClient::new(&env, &contract_id); - - // When - let mut test_map = Map::new(&env); - test_map.set(symbol_short!("key1"), 1); - test_map.set(symbol_short!("key2"), 2); - client.store_map(&test_map); - - // Then - let retrieved_map = client.get_map(); - assert_eq!( - test_map.get(symbol_short!("key1")), - retrieved_map.get(symbol_short!("key1")) - ); - assert_eq!( - test_map.get(symbol_short!("key2")), - retrieved_map.get(symbol_short!("key2")) - ); - } -} From 3e16e4f8831f42125ebb3c95a535221efc54dbb2 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Tue, 20 Aug 2024 11:52:36 -0300 Subject: [PATCH 3/5] Rename test-cases to dynamic-storage --- .../Cargo.toml | 4 +- .../remediated-example}/Cargo.toml | 2 +- .../remediated-example/src/lib.rs | 0 .../vulnerable-example}/Cargo.toml | 2 +- .../vulnerable-example/src/lib.rs | 0 .../remediated-example}/Cargo.toml | 2 +- .../remediated-example/src/lib.rs | 0 .../vulnerable-example}/Cargo.toml | 2 +- .../vulnerable-example/src/lib.rs | 0 .../remediated-example/Cargo.toml | 16 ++++++ .../remediated-example/src/lib.rs | 44 ++++++++++++++++ .../vulnerable-example/Cargo.toml | 16 ++++++ .../vulnerable-example/src/lib.rs | 52 +++++++++++++++++++ 13 files changed, 134 insertions(+), 6 deletions(-) rename test-cases/{dynamic-instance-storage => dynamic-storage}/Cargo.toml (79%) rename test-cases/{dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example => dynamic-storage/dynamic-storage-1/remediated-example}/Cargo.toml (84%) rename test-cases/{dynamic-instance-storage/dynamic-instance-storage-1 => dynamic-storage/dynamic-storage-1}/remediated-example/src/lib.rs (100%) rename test-cases/{dynamic-instance-storage/dynamic-instance-storage-1/remediated-example => dynamic-storage/dynamic-storage-1/vulnerable-example}/Cargo.toml (84%) rename test-cases/{dynamic-instance-storage/dynamic-instance-storage-1 => dynamic-storage/dynamic-storage-1}/vulnerable-example/src/lib.rs (100%) rename test-cases/{dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example => dynamic-storage/dynamic-storage-2/remediated-example}/Cargo.toml (84%) rename test-cases/{dynamic-instance-storage/dynamic-instance-storage-2 => dynamic-storage/dynamic-storage-2}/remediated-example/src/lib.rs (100%) rename test-cases/{dynamic-instance-storage/dynamic-instance-storage-2/remediated-example => dynamic-storage/dynamic-storage-2/vulnerable-example}/Cargo.toml (84%) rename test-cases/{dynamic-instance-storage/dynamic-instance-storage-2 => dynamic-storage/dynamic-storage-2}/vulnerable-example/src/lib.rs (100%) create mode 100644 test-cases/dynamic-storage/dynamic-storage-3/remediated-example/Cargo.toml create mode 100644 test-cases/dynamic-storage/dynamic-storage-3/remediated-example/src/lib.rs create mode 100644 test-cases/dynamic-storage/dynamic-storage-3/vulnerable-example/Cargo.toml create mode 100644 test-cases/dynamic-storage/dynamic-storage-3/vulnerable-example/src/lib.rs diff --git a/test-cases/dynamic-instance-storage/Cargo.toml b/test-cases/dynamic-storage/Cargo.toml similarity index 79% rename from test-cases/dynamic-instance-storage/Cargo.toml rename to test-cases/dynamic-storage/Cargo.toml index 2af55099..6ac24264 100644 --- a/test-cases/dynamic-instance-storage/Cargo.toml +++ b/test-cases/dynamic-storage/Cargo.toml @@ -1,10 +1,10 @@ [workspace] exclude = [".cargo", "target"] -members = ["dynamic-instance-storage-*/*"] +members = ["dynamic-storage-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "=21.4.0" } +soroban-sdk = { version = "=21.5.1" } [profile.release] codegen-units = 1 diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/Cargo.toml b/test-cases/dynamic-storage/dynamic-storage-1/remediated-example/Cargo.toml similarity index 84% rename from test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/Cargo.toml rename to test-cases/dynamic-storage/dynamic-storage-1/remediated-example/Cargo.toml index 9241c0f4..f096e323 100644 --- a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/Cargo.toml +++ b/test-cases/dynamic-storage/dynamic-storage-1/remediated-example/Cargo.toml @@ -1,6 +1,6 @@ [package] edition = "2021" -name = "dynamic-instance-storage-vulnerable-2" +name = "dynamic-storage-remediated-1" version = "0.1.0" [lib] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/src/lib.rs b/test-cases/dynamic-storage/dynamic-storage-1/remediated-example/src/lib.rs similarity index 100% rename from test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/src/lib.rs rename to test-cases/dynamic-storage/dynamic-storage-1/remediated-example/src/lib.rs diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/Cargo.toml b/test-cases/dynamic-storage/dynamic-storage-1/vulnerable-example/Cargo.toml similarity index 84% rename from test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/Cargo.toml rename to test-cases/dynamic-storage/dynamic-storage-1/vulnerable-example/Cargo.toml index 6da39cf1..f1ef1214 100644 --- a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/remediated-example/Cargo.toml +++ b/test-cases/dynamic-storage/dynamic-storage-1/vulnerable-example/Cargo.toml @@ -1,6 +1,6 @@ [package] edition = "2021" -name = "dynamic-instance-storage-remediated-1" +name = "dynamic-storage-vulnerable-1" version = "0.1.0" [lib] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/src/lib.rs b/test-cases/dynamic-storage/dynamic-storage-1/vulnerable-example/src/lib.rs similarity index 100% rename from test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/src/lib.rs rename to test-cases/dynamic-storage/dynamic-storage-1/vulnerable-example/src/lib.rs diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/Cargo.toml b/test-cases/dynamic-storage/dynamic-storage-2/remediated-example/Cargo.toml similarity index 84% rename from test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/Cargo.toml rename to test-cases/dynamic-storage/dynamic-storage-2/remediated-example/Cargo.toml index 7122885e..e0bfcd9c 100644 --- a/test-cases/dynamic-instance-storage/dynamic-instance-storage-1/vulnerable-example/Cargo.toml +++ b/test-cases/dynamic-storage/dynamic-storage-2/remediated-example/Cargo.toml @@ -1,6 +1,6 @@ [package] edition = "2021" -name = "dynamic-instance-storage-vulnerable-1" +name = "dynamic-storage-remediated-2" version = "0.1.0" [lib] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/src/lib.rs b/test-cases/dynamic-storage/dynamic-storage-2/remediated-example/src/lib.rs similarity index 100% rename from test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/src/lib.rs rename to test-cases/dynamic-storage/dynamic-storage-2/remediated-example/src/lib.rs diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/Cargo.toml b/test-cases/dynamic-storage/dynamic-storage-2/vulnerable-example/Cargo.toml similarity index 84% rename from test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/Cargo.toml rename to test-cases/dynamic-storage/dynamic-storage-2/vulnerable-example/Cargo.toml index 7d90d213..93954019 100644 --- a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/remediated-example/Cargo.toml +++ b/test-cases/dynamic-storage/dynamic-storage-2/vulnerable-example/Cargo.toml @@ -1,6 +1,6 @@ [package] edition = "2021" -name = "dynamic-instance-storage-remediated-2" +name = "dynamic-storage-vulnerable-2" version = "0.1.0" [lib] diff --git a/test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/src/lib.rs b/test-cases/dynamic-storage/dynamic-storage-2/vulnerable-example/src/lib.rs similarity index 100% rename from test-cases/dynamic-instance-storage/dynamic-instance-storage-2/vulnerable-example/src/lib.rs rename to test-cases/dynamic-storage/dynamic-storage-2/vulnerable-example/src/lib.rs diff --git a/test-cases/dynamic-storage/dynamic-storage-3/remediated-example/Cargo.toml b/test-cases/dynamic-storage/dynamic-storage-3/remediated-example/Cargo.toml new file mode 100644 index 00000000..32465e65 --- /dev/null +++ b/test-cases/dynamic-storage/dynamic-storage-3/remediated-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dynamic-storage-remediated-3" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dynamic-storage/dynamic-storage-3/remediated-example/src/lib.rs b/test-cases/dynamic-storage/dynamic-storage-3/remediated-example/src/lib.rs new file mode 100644 index 00000000..60f47e41 --- /dev/null +++ b/test-cases/dynamic-storage/dynamic-storage-3/remediated-example/src/lib.rs @@ -0,0 +1,44 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, Map, Symbol}; + +#[contract] +pub struct MapStorage; + +#[contractimpl] +impl MapStorage { + pub fn store_map(e: Env, data: Map) { + data.iter().for_each(|(key, value)| { + e.storage().instance().set(&key, &value); + }); + } + + pub fn get_key(e: Env, key: Symbol) -> i32 { + e.storage().instance().get(&key).unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{symbol_short, Env, Map}; + + #[test] + fn test_map_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, MapStorage); + let client = MapStorageClient::new(&env, &contract_id); + + // When + let mut test_map = Map::new(&env); + test_map.set(symbol_short!("key1"), 1); + test_map.set(symbol_short!("key2"), 2); + client.store_map(&test_map); + + // Then + let key1 = client.get_key(&symbol_short!("key1")); + let key2 = client.get_key(&symbol_short!("key2")); + assert_eq!(test_map.get(symbol_short!("key1")).unwrap(), key1); + assert_eq!(test_map.get(symbol_short!("key2")).unwrap(), key2); + } +} diff --git a/test-cases/dynamic-storage/dynamic-storage-3/vulnerable-example/Cargo.toml b/test-cases/dynamic-storage/dynamic-storage-3/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..37605628 --- /dev/null +++ b/test-cases/dynamic-storage/dynamic-storage-3/vulnerable-example/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "dynamic-storage-vulnerable-3" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { workspace = true } + +[dev-dependencies] +soroban-sdk = { workspace = true, features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/dynamic-storage/dynamic-storage-3/vulnerable-example/src/lib.rs b/test-cases/dynamic-storage/dynamic-storage-3/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..20a33671 --- /dev/null +++ b/test-cases/dynamic-storage/dynamic-storage-3/vulnerable-example/src/lib.rs @@ -0,0 +1,52 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, Env, Map, Symbol}; + +#[contract] +pub struct MapStorage; + +#[contractimpl] +impl MapStorage { + pub fn store_map(e: Env, data: Map) { + e.storage() + .instance() + .set(&Symbol::new(&e, "map_data"), &data); + } + + pub fn get_map(e: Env) -> Map { + e.storage() + .instance() + .get(&Symbol::new(&e, "map_data")) + .unwrap() + } +} + +#[cfg(test)] +mod test { + use super::*; + use soroban_sdk::{symbol_short, Env, Map}; + + #[test] + fn test_map_storage() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, MapStorage); + let client = MapStorageClient::new(&env, &contract_id); + + // When + let mut test_map = Map::new(&env); + test_map.set(symbol_short!("key1"), 1); + test_map.set(symbol_short!("key2"), 2); + client.store_map(&test_map); + + // Then + let retrieved_map = client.get_map(); + assert_eq!( + test_map.get(symbol_short!("key1")), + retrieved_map.get(symbol_short!("key1")) + ); + assert_eq!( + test_map.get(symbol_short!("key2")), + retrieved_map.get(symbol_short!("key2")) + ); + } +} From b143c0bcf6a7a8dc66eb53baf8cd02a49607e705 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Tue, 20 Aug 2024 11:58:19 -0300 Subject: [PATCH 4/5] Rename detector --- .../Cargo.toml | 0 .../src/lib.rs | 26 +++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) rename detectors/{dynamic-instance-storage => dynamic-storage}/Cargo.toml (100%) rename detectors/{dynamic-instance-storage => dynamic-storage}/src/lib.rs (75%) diff --git a/detectors/dynamic-instance-storage/Cargo.toml b/detectors/dynamic-storage/Cargo.toml similarity index 100% rename from detectors/dynamic-instance-storage/Cargo.toml rename to detectors/dynamic-storage/Cargo.toml diff --git a/detectors/dynamic-instance-storage/src/lib.rs b/detectors/dynamic-storage/src/lib.rs similarity index 75% rename from detectors/dynamic-instance-storage/src/lib.rs rename to detectors/dynamic-storage/src/lib.rs index 37b19f80..151a0c52 100644 --- a/detectors/dynamic-instance-storage/src/lib.rs +++ b/detectors/dynamic-storage/src/lib.rs @@ -15,26 +15,26 @@ use rustc_middle::ty::{Ty, TyKind}; use rustc_span::{def_id::LocalDefId, Span, Symbol}; use utils::{get_node_type_opt, is_soroban_storage, SorobanStorageType}; -const LINT_MESSAGE: &str = "This function may lead to excessive instance storage growth, which could increase execution costs or potentially cause DoS"; +const LINT_MESSAGE: &str = "Using dynamic types in instance or persistent storage can lead to unnecessary growth or storage-related vulnerabilities."; dylint_linting::impl_late_lint! { - pub DYNAMIC_INSTANCE_STORAGE, + pub DYNAMIC_STORAGE, Warn, LINT_MESSAGE, - DynamicInstanceStorage, + DynamicStorage, { - name: "Dynamic Instance Storage Analyzer", - long_message: "Detects potential misuse of instance storage that could lead to unnecessary growth or storage-related vulnerabilities.", + name: "Dynamic Storage Analyzer", + long_message: "Using dynamic types in instance or persistent storage can lead to unnecessary growth or storage-related vulnerabilities.", severity: "Warning", - help: "https://coinfabrik.github.io/scout-soroban/docs/detectors/dynamic-instance-storage", + help: "https://coinfabrik.github.io/scout-soroban/docs/detectors/dynamic-storage", vulnerability_class: "Resource Management", } } #[derive(Default)] -struct DynamicInstanceStorage; +struct DynamicStorage; -impl<'tcx> LateLintPass<'tcx> for DynamicInstanceStorage { +impl<'tcx> LateLintPass<'tcx> for DynamicStorage { fn check_fn( &mut self, cx: &LateContext<'tcx>, @@ -48,22 +48,22 @@ impl<'tcx> LateLintPass<'tcx> for DynamicInstanceStorage { return; } - let mut storage_warn_visitor = DynamicInstanceStorageVisitor { cx }; + let mut storage_warn_visitor = DynamicStorageVisitor { cx }; storage_warn_visitor.visit_body(body); } } -struct DynamicInstanceStorageVisitor<'a, 'tcx> { +struct DynamicStorageVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, } -impl<'a, 'tcx> Visitor<'tcx> for DynamicInstanceStorageVisitor<'a, 'tcx> { +impl<'a, 'tcx> Visitor<'tcx> for DynamicStorageVisitor<'a, 'tcx> { fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { if_chain! { // Detect calls to `set` method if let ExprKind::MethodCall(path, receiver, args, _) = &expr.kind; if path.ident.name == Symbol::intern("set"); - // Get the type of the receiver and check if it is an instance storage + // Get the type of the receiver and check if it is an instance or persistent storage if let Some(receiver_ty) = get_node_type_opt(self.cx, &receiver.hir_id); if is_soroban_storage(self.cx, receiver_ty, SorobanStorageType::Instance) || is_soroban_storage(self.cx, receiver_ty, SorobanStorageType::Persistent); @@ -72,7 +72,7 @@ impl<'a, 'tcx> Visitor<'tcx> for DynamicInstanceStorageVisitor<'a, 'tcx> { if let Some(value_type) = get_node_type_opt(self.cx, &args[1].hir_id); if is_dynamic_type(self.cx, &value_type); then { - span_lint(self.cx, DYNAMIC_INSTANCE_STORAGE, expr.span, LINT_MESSAGE) + span_lint(self.cx, DYNAMIC_STORAGE, expr.span, LINT_MESSAGE) } } From 5bcd65b6228bdc96b2504a71eb7bb8d192ac9753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Garc=C3=ADa=20Crosta?= Date: Tue, 20 Aug 2024 13:47:18 -0300 Subject: [PATCH 5/5] Update Cargo.toml --- detectors/dynamic-storage/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/detectors/dynamic-storage/Cargo.toml b/detectors/dynamic-storage/Cargo.toml index 3060bf9f..c564b4ff 100644 --- a/detectors/dynamic-storage/Cargo.toml +++ b/detectors/dynamic-storage/Cargo.toml @@ -1,6 +1,6 @@ [package] edition = "2021" -name = "dynamic-instance-storage" +name = "dynamic-storage" version = "0.1.0" [lib]