From 830a7d02297cf871f9b3c113a826e8af1beae739 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Mon, 8 Jan 2024 11:13:41 -0300 Subject: [PATCH 01/11] Add unprotected-mapping operation detector --- .github/workflows/test-detectors.yml | 1 + .../unprotected-mapping-operation/Cargo.toml | 20 ++ .../unprotected-mapping-operation/src/lib.rs | 201 ++++++++++++++++++ scout-audit-internal/src/detector.rs | 2 + .../src/detector/lint_message.rs | 2 + 5 files changed, 226 insertions(+) create mode 100644 detectors/unprotected-mapping-operation/Cargo.toml create mode 100644 detectors/unprotected-mapping-operation/src/lib.rs diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index fe2365f0..9849f75c 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -104,6 +104,7 @@ jobs: "insufficiently-random-values", "overflow-check", "set-contract-storage", + "unprotected-mapping-operation", "unprotected-update-current-contract-wasm", "unsafe-expect", "unsafe-unwrap", diff --git a/detectors/unprotected-mapping-operation/Cargo.toml b/detectors/unprotected-mapping-operation/Cargo.toml new file mode 100644 index 00000000..a3841f94 --- /dev/null +++ b/detectors/unprotected-mapping-operation/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "unprotected-mapping-operation" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { workspace = true } +if_chain = { workspace = true } + +scout-audit-internal = { workspace = true } + +[dev-dependencies] +dylint_testing = { workspace = true } + + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/unprotected-mapping-operation/src/lib.rs b/detectors/unprotected-mapping-operation/src/lib.rs new file mode 100644 index 00000000..8a7a3e35 --- /dev/null +++ b/detectors/unprotected-mapping-operation/src/lib.rs @@ -0,0 +1,201 @@ +#![feature(rustc_private)] + +extern crate rustc_ast; +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use if_chain::if_chain; +use rustc_hir::{ + def::Res, + intravisit::{walk_body, walk_expr, Visitor}, + Expr, ExprKind, HirId, Param, PatKind, QPath, StmtKind, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::Span; +use scout_audit_internal::Detector; + +dylint_linting::declare_late_lint! { + pub UNPROTECTED_MAPPING_OPERATION, + Warn, + Detector::UnprotectedMappingOperation.get_lint_message() +} + +const SOROBAN_MAP_WITH_ADDRESS: &str = "soroban_sdk::Map LateLintPass<'tcx> for UnprotectedMappingOperation { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: rustc_hir::intravisit::FnKind<'tcx>, + _: &'tcx rustc_hir::FnDecl<'tcx>, + body: &'tcx rustc_hir::Body<'tcx>, + _: Span, + _: rustc_span::def_id::LocalDefId, + ) { + struct AuthStatus { + authed: bool, + } + + struct UnauthorizedAddress { + span: Span, + name: String, + } + + struct UnprotectedMappingOperationFinder<'tcx, 'tcx_ref> { + cx: &'tcx_ref LateContext<'tcx>, + linked_addresses: Vec<(AuthStatus, Vec)>, + unauthorized_span: Vec, + } + + impl<'tcx> Visitor<'tcx> for UnprotectedMappingOperationFinder<'tcx, '_> { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + if let ExprKind::Block(block, _) = &expr.kind { + block.stmts.iter().for_each(|stmt| { + if let StmtKind::Local(local) = &stmt.kind { + if local.init.is_some() + && self.cx.typeck_results().node_type(local.hir_id).to_string() + == SOROBAN_ADDRESS + { + if let PatKind::Binding(_, target_hir_id, _, _) = &local.pat.kind { + let source_hir_id = + UnprotectedMappingOperationFinder::get_expr_hir_id( + local.init.as_ref().unwrap(), + ); + if source_hir_id.is_some() { + // We need to insert into the already existing linked_address, this new address + let source_hir_id = source_hir_id.unwrap(); + self.insert_new_address(source_hir_id, *target_hir_id); + } + } + } + } + }) + } + + if let ExprKind::MethodCall(method_path, method_expr, method_args, _) = &expr.kind { + // Get the method expression type and check if it's a map with address + let method_expr_type = self + .cx + .typeck_results() + .node_type(method_expr.hir_id) + .to_string(); + if method_expr_type.contains(SOROBAN_MAP_WITH_ADDRESS) { + // Iterate through the method arguments and check if any of them is an address and not authed + method_args.iter().for_each(|arg| { + if_chain! { + if let ExprKind::Path(QPath::Resolved(_, path_resolved)) = &arg.kind; + if let Res::Local(id) = path_resolved.res; + if self.cx.typeck_results().node_type(id).to_string().contains(SOROBAN_ADDRESS); + then { + // Obtain the linked_addresses record in wich the address id is contained + let linked_addresses = self.linked_addresses.iter_mut().find(|(_, linked_addresses)| { + linked_addresses.iter().any(|address_hir_id| *address_hir_id == id) + }); + + // If the address does not exist, of if it does but the AuthStatus is false, then we need to add it to the unauthorized_span + if linked_addresses.is_none() || !linked_addresses.unwrap().0.authed { + self.unauthorized_span.push(UnauthorizedAddress { + span: expr.span, + name: self.cx.tcx.hir().name(id).to_string(), + }); + } + } + } + }); + } + + // Check if the method is require_auth and add the address to the authed_addresses + if_chain! { + if method_expr_type.contains(SOROBAN_ADDRESS); + if method_path.ident.name.as_str() == "require_auth"; + if let ExprKind::Path(QPath::Resolved(_, path_resolved)) = &method_expr.kind; + if let Res::Local(id) = path_resolved.res; + then { + self.linked_addresses.iter_mut().for_each(|(auth_status, linked_addresses)| { + linked_addresses.iter().for_each(|address_hir_id| { + if *address_hir_id == id { + auth_status.authed = true; + } + }); + }); + } + } + } + + walk_expr(self, expr); + } + } + + impl<'tcx> UnprotectedMappingOperationFinder<'tcx, '_> { + fn get_expr_hir_id(expr: &Expr) -> Option { + match expr.kind { + ExprKind::MethodCall(_, call_expr, _, _) => { + UnprotectedMappingOperationFinder::get_expr_hir_id(call_expr) + } + ExprKind::Path(qpath_expr) => { + if let QPath::Resolved(_, path) = qpath_expr { + match path.res { + Res::Local(hir_id) => Some(hir_id), + _ => None, + } + } else { + None + } + } + _ => None, + } + } + + fn parse_body_params(&mut self, params: &'tcx [Param<'_>]) { + params.iter().for_each(|param| { + if self.cx.typeck_results().node_type(param.hir_id).to_string() + == SOROBAN_ADDRESS + { + self.linked_addresses + .push((AuthStatus { authed: false }, vec![param.pat.hir_id])); + } + }); + } + + fn insert_new_address(&mut self, source_hir_id: HirId, target_hir_id: HirId) { + self.linked_addresses + .iter_mut() + .find(|(_, linked_addresses)| { + linked_addresses + .iter() + .any(|address_hir_id| *address_hir_id == source_hir_id) + }) + .unwrap() + .1 + .push(target_hir_id); + } + } + + let mut visitor = UnprotectedMappingOperationFinder { + cx, + linked_addresses: Vec::new(), + unauthorized_span: Vec::new(), + }; + + visitor.parse_body_params(body.params); + + walk_body(&mut visitor, body); + + visitor + .unauthorized_span + .iter() + .for_each(|unauthorized_address| { + Detector::UnprotectedMappingOperation.span_lint_and_help( + cx, + UNPROTECTED_MAPPING_OPERATION, + unauthorized_address.span, + &format!( + "Address not authorized, please use `{}.require_auth();` prior to the mapping operation", + unauthorized_address.name + ), + ); + }); + } +} diff --git a/scout-audit-internal/src/detector.rs b/scout-audit-internal/src/detector.rs index 92060139..6b5519c2 100644 --- a/scout-audit-internal/src/detector.rs +++ b/scout-audit-internal/src/detector.rs @@ -33,6 +33,7 @@ pub enum Detector { InsufficientlyRandomValues, OverflowCheck, SetContractStorage, + UnprotectedMappingOperation, UnprotectedUpdateCurrentContractWasm, UnsafeExpect, UnsafeUnwrap, @@ -49,6 +50,7 @@ impl Detector { Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE, Detector::OverflowCheck => OVERFLOW_CHECK_LINT_MESSAGE, Detector::SetContractStorage => SET_CONTRACT_STORAGE_LINT_MESSAGE, + Detector::UnprotectedMappingOperation => UNPROTECTED_MAPPING_OPERATION_LINT_MESSAGE, Detector::UnprotectedUpdateCurrentContractWasm => { UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE } diff --git a/scout-audit-internal/src/detector/lint_message.rs b/scout-audit-internal/src/detector/lint_message.rs index ccda11c2..24d77fff 100644 --- a/scout-audit-internal/src/detector/lint_message.rs +++ b/scout-audit-internal/src/detector/lint_message.rs @@ -9,6 +9,8 @@ pub const INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE: &str = "Use env.prng() to generate random numbers, and remember that all random numbers are under the control of validators"; pub const OVERFLOW_CHECK_LINT_MESSAGE: &str = "Use `overflow-checks = true` in Cargo.toml profile"; pub const SET_CONTRACT_STORAGE_LINT_MESSAGE:&str = "Abitrary users should not have control over keys because it implies writing any value of left mapping, lazy variable, or the main struct of the contract located in position 0 of the storage"; +pub const UNPROTECTED_MAPPING_OPERATION_LINT_MESSAGE: &str = + "This mapping operation is called without access control on a different key than the caller's address"; pub const UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE: &str = "This update_current_contract_wasm is called without access control"; pub const UNSAFE_EXPECT_LINT_MESSAGE: &str = "Unsafe usage of `expect`"; From 43da2cee4f8ebfbe843cfacda7af5875fdac5cbb Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Mon, 8 Jan 2024 11:13:47 -0300 Subject: [PATCH 02/11] Add test cases --- .../remediated-example/Cargo.toml | 31 ++++++++ .../remediated-example/src/lib.rs | 71 +++++++++++++++++++ .../vulnerable-example/Cargo.toml | 31 ++++++++ .../vulnerable-example/src/lib.rs | 66 +++++++++++++++++ 4 files changed, 199 insertions(+) create mode 100644 test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/Cargo.toml create mode 100644 test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/src/lib.rs create mode 100644 test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/Cargo.toml create mode 100644 test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/src/lib.rs diff --git a/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/Cargo.toml b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..c9f82bc0 --- /dev/null +++ b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/Cargo.toml @@ -0,0 +1,31 @@ + +[package] +name = "unprotected-mapping-operation" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "20.0.0" + +[dev_dependencies] +soroban-sdk = { version = "20.0.0", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true diff --git a/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/src/lib.rs b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..f3c98e50 --- /dev/null +++ b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/src/lib.rs @@ -0,0 +1,71 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; + +#[contract] +pub struct UnprotectedMappingOperation; + +#[contracttype] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct State { + balances: Map, +} +const STATE: Symbol = symbol_short!("STATE"); + +#[contractimpl] +impl UnprotectedMappingOperation { + pub fn set_balance(env: Env, address: Address, balance: i128) -> State { + // Authenticate user + address.require_auth(); + + // Get the current state. + let mut state = Self::get_state(env.clone()); + + // Set the new account to have total supply if it doesn't exist. + if !state.balances.contains_key(address.clone()) { + state.balances.set(address, balance); + // Save the state. + env.storage().persistent().set(&STATE, &state); + } + + state + } + + /// Return the current state. + pub fn get_state(env: Env) -> State { + env.storage().persistent().get(&STATE).unwrap_or(State { + balances: Map::new(&env), + }) + } +} + +#[cfg(test)] +const TOTAL_SUPPLY: i128 = 200; + +#[cfg(test)] +mod tests { + + use soroban_sdk::Env; + + use crate::{UnprotectedMappingOperation, UnprotectedMappingOperationClient, TOTAL_SUPPLY}; + + #[test] + fn balance_of_works() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, UnprotectedMappingOperation); + let client = UnprotectedMappingOperationClient::new(&env, &contract_id); + + // When + let state = client + .mock_all_auths() + .set_balance(&contract_id, &TOTAL_SUPPLY); + + // Then + let balance = state + .balances + .get(contract_id) + .expect("Contract should have a balance"); + assert_eq!(TOTAL_SUPPLY, balance); + } +} diff --git a/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/Cargo.toml b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..c9f82bc0 --- /dev/null +++ b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/Cargo.toml @@ -0,0 +1,31 @@ + +[package] +name = "unprotected-mapping-operation" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "20.0.0" + +[dev_dependencies] +soroban-sdk = { version = "20.0.0", features = ["testutils"] } + +[features] +testutils = ["soroban-sdk/testutils"] + +[profile.release] +opt-level = "z" +overflow-checks = true +debug = 0 +strip = "symbols" +debug-assertions = false +panic = "abort" +codegen-units = 1 +lto = true + +[profile.release-with-logs] +inherits = "release" +debug-assertions = true diff --git a/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/src/lib.rs b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..34a88680 --- /dev/null +++ b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/src/lib.rs @@ -0,0 +1,66 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, contracttype, symbol_short, Address, Env, Map, Symbol}; + +#[contract] +pub struct UnprotectedMappingOperation; + +#[contracttype] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct State { + balances: Map, +} +const STATE: Symbol = symbol_short!("STATE"); + +#[contractimpl] +impl UnprotectedMappingOperation { + pub fn set_balance(env: Env, address: Address, balance: i128) -> State { + // Get the current state. + let mut state = Self::get_state(env.clone()); + + // Set the new account to have total supply if it doesn't exist. + if !state.balances.contains_key(address.clone()) { + state.balances.set(address, balance); + // Save the state. + env.storage().persistent().set(&STATE, &state); + } + + state + } + + /// Return the current state. + pub fn get_state(env: Env) -> State { + env.storage().persistent().get(&STATE).unwrap_or(State { + balances: Map::new(&env), + }) + } +} + +#[cfg(test)] +const TOTAL_SUPPLY: i128 = 200; + +#[cfg(test)] +mod tests { + + use soroban_sdk::Env; + + use crate::{UnprotectedMappingOperation, UnprotectedMappingOperationClient, TOTAL_SUPPLY}; + + #[test] + fn balance_of_works() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, UnprotectedMappingOperation); + let client = UnprotectedMappingOperationClient::new(&env, &contract_id); + + // When + let state = client.set_balance(&contract_id, &TOTAL_SUPPLY); + + // Then + let balance = state + .balances + .get(contract_id) + .expect("Contract should have a balance"); + assert_eq!(TOTAL_SUPPLY, balance); + } +} From a842eb254f9b3d5965cf60773f3ddb1e307f2290 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Wed, 13 Mar 2024 09:59:48 -0300 Subject: [PATCH 03/11] Edit detector --- .../unprotected-mapping-operation/src/lib.rs | 275 +++++++++--------- 1 file changed, 137 insertions(+), 138 deletions(-) diff --git a/detectors/unprotected-mapping-operation/src/lib.rs b/detectors/unprotected-mapping-operation/src/lib.rs index 8a7a3e35..cc717c6b 100644 --- a/detectors/unprotected-mapping-operation/src/lib.rs +++ b/detectors/unprotected-mapping-operation/src/lib.rs @@ -21,6 +21,21 @@ dylint_linting::declare_late_lint! { Detector::UnprotectedMappingOperation.get_lint_message() } +struct AuthStatus { + authed: bool, +} + +struct UnauthorizedAddress { + span: Span, + name: String, +} + +struct UnprotectedMappingOperationFinder<'tcx, 'tcx_ref> { + cx: &'tcx_ref LateContext<'tcx>, + linked_addresses: Vec<(AuthStatus, Vec)>, + unauthorized_span: Vec, +} + const SOROBAN_MAP_WITH_ADDRESS: &str = "soroban_sdk::Map LateLintPass<'tcx> for UnprotectedMappingOperation { _: Span, _: rustc_span::def_id::LocalDefId, ) { - struct AuthStatus { - authed: bool, - } + let mut visitor = UnprotectedMappingOperationFinder { + cx, + linked_addresses: Vec::new(), + unauthorized_span: Vec::new(), + }; - struct UnauthorizedAddress { - span: Span, - name: String, - } + visitor.parse_body_params(body.params); - struct UnprotectedMappingOperationFinder<'tcx, 'tcx_ref> { - cx: &'tcx_ref LateContext<'tcx>, - linked_addresses: Vec<(AuthStatus, Vec)>, - unauthorized_span: Vec, - } + walk_body(&mut visitor, body); - impl<'tcx> Visitor<'tcx> for UnprotectedMappingOperationFinder<'tcx, '_> { - fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { - if let ExprKind::Block(block, _) = &expr.kind { - block.stmts.iter().for_each(|stmt| { - if let StmtKind::Local(local) = &stmt.kind { - if local.init.is_some() - && self.cx.typeck_results().node_type(local.hir_id).to_string() - == SOROBAN_ADDRESS - { - if let PatKind::Binding(_, target_hir_id, _, _) = &local.pat.kind { - let source_hir_id = - UnprotectedMappingOperationFinder::get_expr_hir_id( - local.init.as_ref().unwrap(), - ); - if source_hir_id.is_some() { - // We need to insert into the already existing linked_address, this new address - let source_hir_id = source_hir_id.unwrap(); - self.insert_new_address(source_hir_id, *target_hir_id); - } - } - } - } - }) - } + visitor + .unauthorized_span + .iter() + .for_each(|unauthorized_address| { + Detector::UnprotectedMappingOperation.span_lint_and_help( + cx, + UNPROTECTED_MAPPING_OPERATION, + unauthorized_address.span, + &format!( + "Address not authorized, please use `{}.require_auth();` prior to the mapping operation", + unauthorized_address.name + ), + ); + }); + } +} - if let ExprKind::MethodCall(method_path, method_expr, method_args, _) = &expr.kind { - // Get the method expression type and check if it's a map with address - let method_expr_type = self - .cx - .typeck_results() - .node_type(method_expr.hir_id) - .to_string(); - if method_expr_type.contains(SOROBAN_MAP_WITH_ADDRESS) { - // Iterate through the method arguments and check if any of them is an address and not authed - method_args.iter().for_each(|arg| { - if_chain! { - if let ExprKind::Path(QPath::Resolved(_, path_resolved)) = &arg.kind; - if let Res::Local(id) = path_resolved.res; - if self.cx.typeck_results().node_type(id).to_string().contains(SOROBAN_ADDRESS); - then { - // Obtain the linked_addresses record in wich the address id is contained - let linked_addresses = self.linked_addresses.iter_mut().find(|(_, linked_addresses)| { - linked_addresses.iter().any(|address_hir_id| *address_hir_id == id) - }); - - // If the address does not exist, of if it does but the AuthStatus is false, then we need to add it to the unauthorized_span - if linked_addresses.is_none() || !linked_addresses.unwrap().0.authed { - self.unauthorized_span.push(UnauthorizedAddress { - span: expr.span, - name: self.cx.tcx.hir().name(id).to_string(), - }); - } - } - } - }); +impl<'tcx> Visitor<'tcx> for UnprotectedMappingOperationFinder<'tcx, '_> { + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + if let ExprKind::Block(block, _) = &expr.kind { + block.stmts.iter().for_each(|stmt| { + if_chain! { + if let StmtKind::Local(local) = &stmt.kind; + if self.get_node_type(local.hir_id) == SOROBAN_ADDRESS; + if let PatKind::Binding(_, target_hir_id, _, _) = &local.pat.kind; + if let Some(init) = &local.init; + let source_hir_id = self.get_expr_hir_id(init); + if let Some(source_hir_id) = source_hir_id; + then { + // Insert the new address into the linked_addresses + self.insert_new_address(source_hir_id, *target_hir_id); } + } + }) + } + + if let ExprKind::MethodCall(method_path, method_expr, method_args, _) = &expr.kind { + // Get the method expression type and check if it's a map with address + let method_expr_type = self.get_node_type(method_expr.hir_id); - // Check if the method is require_auth and add the address to the authed_addresses + if method_expr_type.contains(SOROBAN_MAP_WITH_ADDRESS) { + // Iterate through the method arguments and check if any of them is an address and not authed + method_args.iter().for_each(|arg| { if_chain! { - if method_expr_type.contains(SOROBAN_ADDRESS); - if method_path.ident.name.as_str() == "require_auth"; - if let ExprKind::Path(QPath::Resolved(_, path_resolved)) = &method_expr.kind; - if let Res::Local(id) = path_resolved.res; + if let Some(id) = self.get_expr_hir_id(arg); + if self.get_node_type(id).contains(SOROBAN_ADDRESS); then { - self.linked_addresses.iter_mut().for_each(|(auth_status, linked_addresses)| { - linked_addresses.iter().for_each(|address_hir_id| { - if *address_hir_id == id { - auth_status.authed = true; - } + // Obtain the linked_addresses record in wich the address id is contained + let linked_address = self.get_linked_address(id); + + // If the address does not exist, of if it does but the AuthStatus is false, then we need to add it to the unauthorized_span + if linked_address.is_none() || !linked_address.unwrap().0.authed { + self.unauthorized_span.push(UnauthorizedAddress { + span: expr.span, + name: self.cx.tcx.hir().name(id).to_string(), }); - }); + } } } - } - - walk_expr(self, expr); + }); } - } - impl<'tcx> UnprotectedMappingOperationFinder<'tcx, '_> { - fn get_expr_hir_id(expr: &Expr) -> Option { - match expr.kind { - ExprKind::MethodCall(_, call_expr, _, _) => { - UnprotectedMappingOperationFinder::get_expr_hir_id(call_expr) - } - ExprKind::Path(qpath_expr) => { - if let QPath::Resolved(_, path) = qpath_expr { - match path.res { - Res::Local(hir_id) => Some(hir_id), - _ => None, - } - } else { - None - } - } - _ => None, + // Check if the method call is a require_auth call and if it is, then we need to update the AuthStatus + if_chain! { + if method_expr_type.contains(SOROBAN_ADDRESS); + if method_path.ident.name.as_str() == "require_auth"; + if let Some(id) = self.get_expr_hir_id(method_expr); + then { + self.auth_address(id) } } + } - fn parse_body_params(&mut self, params: &'tcx [Param<'_>]) { - params.iter().for_each(|param| { - if self.cx.typeck_results().node_type(param.hir_id).to_string() - == SOROBAN_ADDRESS - { - self.linked_addresses - .push((AuthStatus { authed: false }, vec![param.pat.hir_id])); - } - }); - } + walk_expr(self, expr); + } +} - fn insert_new_address(&mut self, source_hir_id: HirId, target_hir_id: HirId) { +impl<'tcx> UnprotectedMappingOperationFinder<'tcx, '_> { + fn parse_body_params(&mut self, params: &'tcx [Param<'_>]) { + params.iter().for_each(|param| { + if self.get_node_type(param.hir_id) == SOROBAN_ADDRESS { self.linked_addresses - .iter_mut() - .find(|(_, linked_addresses)| { - linked_addresses - .iter() - .any(|address_hir_id| *address_hir_id == source_hir_id) - }) - .unwrap() - .1 - .push(target_hir_id); + .push((AuthStatus { authed: false }, vec![param.pat.hir_id])); } + }); + } + + fn get_node_type(&self, hir_id: HirId) -> String { + self.cx.typeck_results().node_type(hir_id).to_string() + } + + fn insert_new_address(&mut self, source_hir_id: HirId, target_hir_id: HirId) { + if let Some((_, linked_addresses)) = self + .linked_addresses + .iter_mut() + .find(|(_, addresses)| addresses.iter().any(|&id| id == source_hir_id)) + { + linked_addresses.push(target_hir_id); } + } - let mut visitor = UnprotectedMappingOperationFinder { - cx, - linked_addresses: Vec::new(), - unauthorized_span: Vec::new(), - }; + fn get_expr_hir_id(&self, expr: &Expr) -> Option { + let mut stack = vec![expr]; + + while let Some(current_expr) = stack.pop() { + match current_expr.kind { + ExprKind::MethodCall(_, call_expr, _, _) => stack.push(call_expr), + ExprKind::Path(QPath::Resolved(_, path)) => match path.res { + Res::Local(hir_id) => return Some(hir_id), + _ => continue, + }, + _ => continue, + } + } - visitor.parse_body_params(body.params); + None + } - walk_body(&mut visitor, body); + fn get_linked_address(&self, id: HirId) -> Option<&(AuthStatus, Vec)> { + self.linked_addresses.iter().find(|(_, linked_addresses)| { + linked_addresses + .iter() + .any(|&address_hir_id| address_hir_id == id) + }) + } - visitor - .unauthorized_span - .iter() - .for_each(|unauthorized_address| { - Detector::UnprotectedMappingOperation.span_lint_and_help( - cx, - UNPROTECTED_MAPPING_OPERATION, - unauthorized_address.span, - &format!( - "Address not authorized, please use `{}.require_auth();` prior to the mapping operation", - unauthorized_address.name - ), - ); + fn auth_address(&mut self, id: HirId) { + self.linked_addresses + .iter_mut() + .for_each(|(auth_status, linked_addresses)| { + if linked_addresses + .iter() + .any(|&address_hir_id| address_hir_id == id) + { + auth_status.authed = true; + } }); } } From 68481d6397bd3f78ac8b0a9e017934c46c30e969 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Wed, 13 Mar 2024 14:56:31 -0300 Subject: [PATCH 04/11] Edit soroban-sdk version --- .../remediated-example/Cargo.toml | 4 ++-- .../vulnerable-example/Cargo.toml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/Cargo.toml b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/Cargo.toml index c9f82bc0..5e367c2e 100644 --- a/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/Cargo.toml +++ b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/remediated-example/Cargo.toml @@ -8,10 +8,10 @@ edition = "2021" crate-type = ["cdylib"] [dependencies] -soroban-sdk = "20.0.0" +soroban-sdk = "=20.0.0" [dev_dependencies] -soroban-sdk = { version = "20.0.0", features = ["testutils"] } +soroban-sdk = { version = "=20.0.0", features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] diff --git a/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/Cargo.toml b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/Cargo.toml index c9f82bc0..5e367c2e 100644 --- a/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/Cargo.toml +++ b/test-cases/unprotected-mapping-operation/unprotected-mapping-operation-1/vulnerable-example/Cargo.toml @@ -8,10 +8,10 @@ edition = "2021" crate-type = ["cdylib"] [dependencies] -soroban-sdk = "20.0.0" +soroban-sdk = "=20.0.0" [dev_dependencies] -soroban-sdk = { version = "20.0.0", features = ["testutils"] } +soroban-sdk = { version = "=20.0.0", features = ["testutils"] } [features] testutils = ["soroban-sdk/testutils"] From c1bd75a1408defa752917c64df2ceb0445a4942a Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 14 Mar 2024 13:57:05 -0300 Subject: [PATCH 05/11] Allow unordered mapping --- detectors/unprotected-mapping-operation/src/lib.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/detectors/unprotected-mapping-operation/src/lib.rs b/detectors/unprotected-mapping-operation/src/lib.rs index cc717c6b..430c2c91 100644 --- a/detectors/unprotected-mapping-operation/src/lib.rs +++ b/detectors/unprotected-mapping-operation/src/lib.rs @@ -36,7 +36,7 @@ struct UnprotectedMappingOperationFinder<'tcx, 'tcx_ref> { unauthorized_span: Vec, } -const SOROBAN_MAP_WITH_ADDRESS: &str = "soroban_sdk::Map LateLintPass<'tcx> for UnprotectedMappingOperation { @@ -99,7 +99,11 @@ impl<'tcx> Visitor<'tcx> for UnprotectedMappingOperationFinder<'tcx, '_> { // Get the method expression type and check if it's a map with address let method_expr_type = self.get_node_type(method_expr.hir_id); - if method_expr_type.contains(SOROBAN_MAP_WITH_ADDRESS) { + println!("Method expression type: {}", method_expr_type); + + if method_expr_type.starts_with(SOROBAN_MAP) + && method_expr_type.contains(SOROBAN_ADDRESS) + { // Iterate through the method arguments and check if any of them is an address and not authed method_args.iter().for_each(|arg| { if_chain! { From 273c1bc186091643c97473b98186ac3d00652fe8 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Thu, 14 Mar 2024 13:57:41 -0300 Subject: [PATCH 06/11] Remove println --- detectors/unprotected-mapping-operation/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/detectors/unprotected-mapping-operation/src/lib.rs b/detectors/unprotected-mapping-operation/src/lib.rs index 430c2c91..96ceed9a 100644 --- a/detectors/unprotected-mapping-operation/src/lib.rs +++ b/detectors/unprotected-mapping-operation/src/lib.rs @@ -99,8 +99,6 @@ impl<'tcx> Visitor<'tcx> for UnprotectedMappingOperationFinder<'tcx, '_> { // Get the method expression type and check if it's a map with address let method_expr_type = self.get_node_type(method_expr.hir_id); - println!("Method expression type: {}", method_expr_type); - if method_expr_type.starts_with(SOROBAN_MAP) && method_expr_type.contains(SOROBAN_ADDRESS) { From ff977795a5dd4b4bf599a4113fc7597f618ccce7 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Mar 2024 15:57:20 -0300 Subject: [PATCH 07/11] Add unprotected mapping operation to detectors table. --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 1cbbccc4..c59a7eeb 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,7 @@ Install Scout from the Marketplace within the Extensions tab of Visual Studio Co | [avoid-unsafe-block](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block) | Using unsafe blocks in risks code safety and reliability.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1) | Critical | | [dos-unbounded-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unbounded-operation) | DoS due to unbounded operation. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-3) | Medium | | [soroban-version](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/soroban-version) | Using a pinned version of Soroban can be dangerous, as it may have bugs or security issues. Use the latest version available. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1) | Enhancement | +| [unprotected-mapping-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-opereation) | Modifying mappings with an arbitrary key given by the user could lead to unintented modifications of critical data, modifying data belonging to other users, causing denial of service, unathorized access, and other potential issues. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/unprotected-mapping-opereation) | Critical | ## CLI Options From bd8c12173104db6481e11cc6e3da242eae2b91aa Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Mar 2024 16:00:54 -0300 Subject: [PATCH 08/11] Add unprotected mapping operation vulnerability class --- test-cases/README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test-cases/README.md b/test-cases/README.md index 096512b0..485e32ae 100644 --- a/test-cases/README.md +++ b/test-cases/README.md @@ -192,3 +192,8 @@ Using a pinned version of Soroban can be dangerous, as it may have bugs or secur We classified this issue, a deviation from best practices which could have security implications, under the [Best practices](#vulnerability-categories) category and assigned it an Enhancement severity. +### Unprotected mapping operation + +Modifying mappings with an arbitrary key given by the user could lead to unintented modifications of critical data, modifying data belonging to other users, causing denial of service, unathorized access, and other potential issues. + +This vulnerability falls under the [Validations and error handling](#vulnerability-categories) category and has a Critical severity. From 5d1c574db3a6177c7e673cbac64225ada1a087f1 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Mar 2024 17:15:44 -0300 Subject: [PATCH 09/11] Add unprotected mapping operation detector documentation --- .../unprotected-mapping-operation/README.md | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 detectors/unprotected-mapping-operation/README.md diff --git a/detectors/unprotected-mapping-operation/README.md b/detectors/unprotected-mapping-operation/README.md new file mode 100644 index 00000000..e06f9710 --- /dev/null +++ b/detectors/unprotected-mapping-operation/README.md @@ -0,0 +1,60 @@ +# Unprotected Mapping Operation + +### What it does + +It warns you if a mapping operation (`insert`, `take`, `remove`) function is called with a user-given `key` field of the type `AccountId`. + +### Why is this bad? + +Modifying mappings with an arbitrary key given by users can be a significant vulnerability for several reasons: + +- Unintended Modifications: Allowing users to provide arbitrary keys can lead to unintended modifications of critical data within the smart contract. If the input validation and sanitation are not done properly, users may be able to manipulate the data in ways that were not intended by the contract's author. + +- Data Corruption: Malicious users could intentionally provide keys that result in the corruption or manipulation of important data stored in the mapping. This could lead to incorrect calculations, unauthorized access, or other undesirable outcomes. + +- Denial-of-Service (DoS) Attacks: If users can set arbitrary keys, they may be able to create mappings with a large number of entries, potentially causing the contract to exceed its gas limit. This could lead to denial-of-service attacks, making the contract unusable for other users. + +### Known problems + +### Example + +```rust + pub fn set_balance(env: Env, address: Address, balance: i128) -> State { + // Get the current state. + let mut state = Self::get_state(env.clone()); + + // Set the new account to have total supply if it doesn't exist. + if !state.balances.contains_key(address.clone()) { + state.balances.set(address, balance); + // Save the state. + env.storage().persistent().set(&STATE, &state); + } + + state + } +``` + +Use instead: + +```rust + pub fn set_balance(env: Env, address: Address, balance: i128) -> State { + // Authenticate user + address.require_auth(); + + // Get the current state. + let mut state = Self::get_state(env.clone()); + + // Set the new account to have total supply if it doesn't exist. + if !state.balances.contains_key(address.clone()) { + state.balances.set(address, balance); + // Save the state. + env.storage().persistent().set(&STATE, &state); + } + + state + } +``` + +### Implementation + +The detector's implementation can be found at [this link](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-operation). From e54f427c4f8ce5aab6e9abe89110b0ff1680ce69 Mon Sep 17 00:00:00 2001 From: Arturo Beccar-Varela <107512933+arturoBeccar@users.noreply.github.com> Date: Thu, 14 Mar 2024 17:20:51 -0300 Subject: [PATCH 10/11] Update test case link for unprotected mapping operation --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index c59a7eeb..a8be55db 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,7 @@ Install Scout from the Marketplace within the Extensions tab of Visual Studio Co | [avoid-unsafe-block](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/avoid-unsafe-block) | Using unsafe blocks in risks code safety and reliability.| [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/avoid-unsafe-block/avoid-unsafe-block-1) | Critical | | [dos-unbounded-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/dos-unbounded-operation) | DoS due to unbounded operation. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-1), [2](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-2), [3](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/dos-unbounded-operation/dos-unbounded-operation-3) | Medium | | [soroban-version](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/soroban-version) | Using a pinned version of Soroban can be dangerous, as it may have bugs or security issues. Use the latest version available. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/soroban-version-1) | Enhancement | -| [unprotected-mapping-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-opereation) | Modifying mappings with an arbitrary key given by the user could lead to unintented modifications of critical data, modifying data belonging to other users, causing denial of service, unathorized access, and other potential issues. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/soroban-version/unprotected-mapping-opereation) | Critical | +| [unprotected-mapping-operation](https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unprotected-mapping-opereation) | Modifying mappings with an arbitrary key given by the user could lead to unintented modifications of critical data, modifying data belonging to other users, causing denial of service, unathorized access, and other potential issues. | [1](https://github.com/CoinFabrik/scout-soroban/tree/main/test-cases/unprotected-mapping-opereation/unprotected-mapping-opereation-1) | Critical | ## CLI Options From 15cc0165240834d4a6d6caff458b587baccb8db3 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Wed, 20 Mar 2024 10:25:48 -0300 Subject: [PATCH 11/11] Update detector to check type and key of mapping --- .../unprotected-mapping-operation/src/lib.rs | 60 ++++++++++--------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/detectors/unprotected-mapping-operation/src/lib.rs b/detectors/unprotected-mapping-operation/src/lib.rs index 96ceed9a..078b6a30 100644 --- a/detectors/unprotected-mapping-operation/src/lib.rs +++ b/detectors/unprotected-mapping-operation/src/lib.rs @@ -12,7 +12,8 @@ use rustc_hir::{ Expr, ExprKind, HirId, Param, PatKind, QPath, StmtKind, }; use rustc_lint::{LateContext, LateLintPass}; -use rustc_span::Span; +use rustc_middle::ty::{Ty, TyKind}; +use rustc_span::{Span, Symbol}; use scout_audit_internal::Detector; dylint_linting::declare_late_lint! { @@ -82,7 +83,7 @@ impl<'tcx> Visitor<'tcx> for UnprotectedMappingOperationFinder<'tcx, '_> { block.stmts.iter().for_each(|stmt| { if_chain! { if let StmtKind::Local(local) = &stmt.kind; - if self.get_node_type(local.hir_id) == SOROBAN_ADDRESS; + if self.get_node_type(local.hir_id).to_string() == SOROBAN_ADDRESS; if let PatKind::Binding(_, target_hir_id, _, _) = &local.pat.kind; if let Some(init) = &local.init; let source_hir_id = self.get_expr_hir_id(init); @@ -99,34 +100,39 @@ impl<'tcx> Visitor<'tcx> for UnprotectedMappingOperationFinder<'tcx, '_> { // Get the method expression type and check if it's a map with address let method_expr_type = self.get_node_type(method_expr.hir_id); - if method_expr_type.starts_with(SOROBAN_MAP) - && method_expr_type.contains(SOROBAN_ADDRESS) - { - // Iterate through the method arguments and check if any of them is an address and not authed - method_args.iter().for_each(|arg| { - if_chain! { - if let Some(id) = self.get_expr_hir_id(arg); - if self.get_node_type(id).contains(SOROBAN_ADDRESS); - then { - // Obtain the linked_addresses record in wich the address id is contained - let linked_address = self.get_linked_address(id); - - // If the address does not exist, of if it does but the AuthStatus is false, then we need to add it to the unauthorized_span - if linked_address.is_none() || !linked_address.unwrap().0.authed { - self.unauthorized_span.push(UnauthorizedAddress { - span: expr.span, - name: self.cx.tcx.hir().name(id).to_string(), - }); + if_chain! { + if let ExprKind::Field(_, _) = &method_expr.kind; + if let TyKind::Adt(adt_def, args) = method_expr_type.kind(); + if self.cx.tcx.def_path_str(adt_def.did()) == SOROBAN_MAP; + if let Some(first_arg) = args.first(); + if first_arg.to_string() == SOROBAN_ADDRESS; + then { + // Iterate through the method arguments and check if any of them is an address and not authed + method_args.iter().for_each(|arg| { + if_chain! { + if let Some(id) = self.get_expr_hir_id(arg); + if self.get_node_type(id).to_string().contains(SOROBAN_ADDRESS); + then { + // Obtain the linked_addresses record in wich the address id is contained + let linked_address = self.get_linked_address(id); + + // If the address does not exist, of if it does but the AuthStatus is false, then we need to add it to the unauthorized_span + if linked_address.is_none() || !linked_address.unwrap().0.authed { + self.unauthorized_span.push(UnauthorizedAddress { + span: expr.span, + name: self.cx.tcx.hir().name(id).to_string(), + }); + } } } - } - }); + }); + } } // Check if the method call is a require_auth call and if it is, then we need to update the AuthStatus if_chain! { - if method_expr_type.contains(SOROBAN_ADDRESS); - if method_path.ident.name.as_str() == "require_auth"; + if method_expr_type.to_string() == SOROBAN_ADDRESS; + if method_path.ident.name == Symbol::intern("require_auth"); if let Some(id) = self.get_expr_hir_id(method_expr); then { self.auth_address(id) @@ -141,15 +147,15 @@ impl<'tcx> Visitor<'tcx> for UnprotectedMappingOperationFinder<'tcx, '_> { impl<'tcx> UnprotectedMappingOperationFinder<'tcx, '_> { fn parse_body_params(&mut self, params: &'tcx [Param<'_>]) { params.iter().for_each(|param| { - if self.get_node_type(param.hir_id) == SOROBAN_ADDRESS { + if self.get_node_type(param.hir_id).to_string() == SOROBAN_ADDRESS { self.linked_addresses .push((AuthStatus { authed: false }, vec![param.pat.hir_id])); } }); } - fn get_node_type(&self, hir_id: HirId) -> String { - self.cx.typeck_results().node_type(hir_id).to_string() + fn get_node_type(&self, hir_id: HirId) -> Ty<'tcx> { + self.cx.typeck_results().node_type(hir_id) } fn insert_new_address(&mut self, source_hir_id: HirId, target_hir_id: HirId) {