From c265d7c7712817d53b306419cf93fcabdc568539 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Fri, 19 Apr 2024 18:46:37 -0300 Subject: [PATCH 1/5] Add vulnerable and remediated cases --- detectors/unsafe-expect/src/lib.rs | 1 + test-cases/unsafe-map-get/Cargo.toml | 21 +++++ .../remediated-example/Cargo.toml | 17 ++++ .../remediated-example/src/lib.rs | 83 +++++++++++++++++++ .../vulnerable-example/Cargo.toml | 17 ++++ .../vulnerable-example/src/lib.rs | 39 +++++++++ 6 files changed, 178 insertions(+) create mode 100644 test-cases/unsafe-map-get/Cargo.toml create mode 100644 test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/Cargo.toml create mode 100644 test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/src/lib.rs create mode 100644 test-cases/unsafe-map-get/unsafe-map-get-1/vulnerable-example/Cargo.toml create mode 100644 test-cases/unsafe-map-get/unsafe-map-get-1/vulnerable-example/src/lib.rs diff --git a/detectors/unsafe-expect/src/lib.rs b/detectors/unsafe-expect/src/lib.rs index e331b61a..9eccba05 100644 --- a/detectors/unsafe-expect/src/lib.rs +++ b/detectors/unsafe-expect/src/lib.rs @@ -4,6 +4,7 @@ extern crate rustc_hir; extern crate rustc_span; +use std::thread::current; use std::{collections::HashSet, hash::Hash}; use if_chain::if_chain; diff --git a/test-cases/unsafe-map-get/Cargo.toml b/test-cases/unsafe-map-get/Cargo.toml new file mode 100644 index 00000000..1bc3e364 --- /dev/null +++ b/test-cases/unsafe-map-get/Cargo.toml @@ -0,0 +1,21 @@ +[workspace] +exclude = [".cargo", "target"] +members = ["unsafe-map-get-*/*"] +resolver = "2" + +[workspace.dependencies] +soroban-sdk = { version = "20.5.0" } + +[profile.release] +codegen-units = 1 +debug = 0 +debug-assertions = false +lto = true +opt-level = "z" +overflow-checks = true +panic = "abort" +strip = "symbols" + +[profile.release-with-logs] +debug-assertions = true +inherits = "release" diff --git a/test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/Cargo.toml b/test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..c70637a3 --- /dev/null +++ b/test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/Cargo.toml @@ -0,0 +1,17 @@ + +[package] +edition = "2021" +name = "unsafe-map-get-remediated-1" +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/unsafe-map-get/unsafe-map-get-1/remediated-example/src/lib.rs b/test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..4f0f41b8 --- /dev/null +++ b/test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/src/lib.rs @@ -0,0 +1,83 @@ +// #![no_std] + +use soroban_sdk::{contract, contractimpl, map, Env, Error, IntoVal, Map, TryIntoVal, Val}; + +#[contract] +pub struct UnsafeMapGet; + +#[contractimpl] +impl UnsafeMapGet { + pub fn get_map_with_different_values(env: Env, key: i32) -> Result, Error> { + let map: Map = map![ + &env, + (1i32.into_val(&env), 2i32.into_val(&env)), + (3i32.into_val(&env), 4i64.into_val(&env)), + ]; + let map: Val = map.into(); + let map: Map = map.try_into_val(&env).unwrap(); + map.try_get(key).map_err(Error::from) + } + + pub fn get_map_with_different_keys(env: Env, key: i32) -> Result, Error> { + let map: Map = map![ + &env, + (1i32.into_val(&env), 2i32.into_val(&env)), + (3i64.into_val(&env), 4i32.into_val(&env)), + ]; + let map: Val = map.into(); + let map: Map = map.try_into_val(&env).unwrap(); + map.try_get(key).map_err(Error::from) + } +} + +#[cfg(test)] +mod tests { + use soroban_sdk::{ + xdr::{ScError, ScErrorCode}, + Env, Error, + }; + + use crate::{UnsafeMapGet, UnsafeMapGetClient}; + + #[test] + fn try_get_errors_on_different_values() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, UnsafeMapGet); + let client = UnsafeMapGetClient::new(&env, &contract_id); + + // When + let value_ok = client.try_get_map_with_different_values(&1); + let value_err = client.try_get_map_with_different_values(&3); + + // Then + assert_eq!(value_ok.unwrap(), Ok(Some(2))); + assert_eq!( + value_err.err().unwrap(), + Ok(Error::from_scerror(ScError::Context( + ScErrorCode::InvalidAction + ))) + ); + } + + #[test] + fn try_get_errors_on_different_keys() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, UnsafeMapGet); + let client = UnsafeMapGetClient::new(&env, &contract_id); + + // When + let key_ok = client.try_get_map_with_different_values(&1); + let key_err = client.try_get_map_with_different_values(&3); + + // Then + assert_eq!(key_ok.unwrap(), Ok(Some(2))); + assert_eq!( + key_err.err().unwrap(), + Ok(Error::from_scerror(ScError::Context( + ScErrorCode::InvalidAction + ))) + ); + } +} diff --git a/test-cases/unsafe-map-get/unsafe-map-get-1/vulnerable-example/Cargo.toml b/test-cases/unsafe-map-get/unsafe-map-get-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..5c661412 --- /dev/null +++ b/test-cases/unsafe-map-get/unsafe-map-get-1/vulnerable-example/Cargo.toml @@ -0,0 +1,17 @@ + +[package] +edition = "2021" +name = "unsafe-map-get-vulnerable-1" +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/unsafe-map-get/unsafe-map-get-1/vulnerable-example/src/lib.rs b/test-cases/unsafe-map-get/unsafe-map-get-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..269cb5cb --- /dev/null +++ b/test-cases/unsafe-map-get/unsafe-map-get-1/vulnerable-example/src/lib.rs @@ -0,0 +1,39 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, map, Env, IntoVal, Map, TryIntoVal, Val}; + +#[contract] +pub struct UnsafeMapGet; + +#[contractimpl] +impl UnsafeMapGet { + pub fn get_from_map(env: Env) -> Option { + let map: Map = map![&env, (1i32.into_val(&env), 2i64.into_val(&env))]; + let map: Val = map.into(); + let map: Map = map.try_into_val(&env).unwrap(); + map.get(1) + } +} + +#[cfg(test)] +mod tests { + use soroban_sdk::Env; + + use crate::{UnsafeMapGet, UnsafeMapGetClient}; + + #[test] + #[should_panic(expected = "ConversionError")] + fn test_insert_balances() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, UnsafeMapGet); + let client = UnsafeMapGetClient::new(&env, &contract_id); + + // When + let _value = client.get_from_map(); + + // Then + + // Test should panic + } +} From 530dec9eb4491d70521715405ea1e7c4eb4dba2e Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Sat, 20 Apr 2024 14:14:18 -0300 Subject: [PATCH 2/5] Add detector --- detectors/Cargo.toml | 1 + detectors/unsafe-map-get/Cargo.toml | 16 ++++++ detectors/unsafe-map-get/src/lib.rs | 88 +++++++++++++++++++++++++++++ utils/src/lib.rs | 3 + utils/src/lint_utils/mod.rs | 13 +++++ utils/src/soroban_utils/mod.rs | 25 +++++--- 6 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 detectors/unsafe-map-get/Cargo.toml create mode 100644 detectors/unsafe-map-get/src/lib.rs create mode 100644 utils/src/lint_utils/mod.rs diff --git a/detectors/Cargo.toml b/detectors/Cargo.toml index c2f2d36e..844c937b 100644 --- a/detectors/Cargo.toml +++ b/detectors/Cargo.toml @@ -7,3 +7,4 @@ resolver = "2" dylint_linting = { package = "scout-audit-dylint-linting", version = "3.0.1" } if_chain = "1.0.2" scout-audit-clippy-utils = { version = "=0.2.3" } +utils = { path = "../utils" } diff --git a/detectors/unsafe-map-get/Cargo.toml b/detectors/unsafe-map-get/Cargo.toml new file mode 100644 index 00000000..debc884a --- /dev/null +++ b/detectors/unsafe-map-get/Cargo.toml @@ -0,0 +1,16 @@ +[package] +edition = "2021" +name = "unsafe-map-get" +version = "0.1.0" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +dylint_linting = { workspace = true } +if_chain = { workspace = true } +scout-audit-clippy-utils = { workspace = true } +utils = { workspace = true } + +[package.metadata.rust-analyzer] +rustc_private = true diff --git a/detectors/unsafe-map-get/src/lib.rs b/detectors/unsafe-map-get/src/lib.rs new file mode 100644 index 00000000..4e845582 --- /dev/null +++ b/detectors/unsafe-map-get/src/lib.rs @@ -0,0 +1,88 @@ +#![feature(rustc_private)] + +extern crate rustc_errors; +extern crate rustc_hir; +extern crate rustc_span; + +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{ + intravisit::{walk_expr, FnKind, Visitor}, + Body, Expr, ExprKind, FnDecl, +}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_span::{def_id::LocalDefId, Span}; +use scout_audit_clippy_utils::diagnostics::span_lint_and_sugg; +use utils::{get_receiver_ident_name, is_soroban_map}; + +const LINT_MESSAGE: &str = "Unsafe access on Map, method could panic."; +const UNSAFE_GET_METHODS: [&str; 3] = ["get", "get_unchecked", "try_get_unchecked"]; + +dylint_linting::declare_late_lint! { + pub UNSAFE_MAP_GET, + Warn, + LINT_MESSAGE, + { + name: "Unsafe Map Get", + long_message: "This vulnerability class pertains to the inappropriate usage of the get method for Map in soroban", + severity: "Medium", + help: "https://github.com/CoinFabrik/scout-soroban/tree/main/detectors/unsafe-map-get", + vulnerability_class: "Validations and error handling", + } +} + +struct UnsafeMapGetVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, +} + +impl UnsafeMapGetVisitor<'_, '_> { + fn get_first_arg_str(&self, arg: Option<&Expr<'_>>) -> String { + arg.and_then(|arg| self.cx.sess().source_map().span_to_snippet(arg.span).ok()) + .unwrap_or_default() + } +} + +impl<'a, 'tcx> Visitor<'tcx> for UnsafeMapGetVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if_chain! { + if let ExprKind::MethodCall(path_segment, receiver, args, _) = &expr.kind; + if UNSAFE_GET_METHODS.contains(&path_segment.ident.as_str()); + if is_soroban_map(self.cx, self.cx.typeck_results().node_type(receiver.hir_id)); + then { + let receiver_ident_name = get_receiver_ident_name(receiver); + let first_arg_str = self.get_first_arg_str(args.first()); + span_lint_and_sugg( + self.cx, + UNSAFE_MAP_GET, + expr.span, + LINT_MESSAGE, + &format!("Using `{}` on a Map is unsafe as it could panic, please use", path_segment.ident), + format!("{}.try_get({})", receiver_ident_name, first_arg_str), + Applicability::MaybeIncorrect, + ); + } + } + walk_expr(self, expr); + } +} + +impl<'tcx> LateLintPass<'tcx> for UnsafeMapGet { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: FnKind<'tcx>, + _: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + span: Span, + _: LocalDefId, + ) { + // If the function comes from a macro expansion, we don't want to analyze it. + if span.from_expansion() { + return; + } + + let mut visitor = UnsafeMapGetVisitor { cx }; + + walk_expr(&mut visitor, body.value); + } +} diff --git a/utils/src/lib.rs b/utils/src/lib.rs index 0fa3186c..352bef89 100644 --- a/utils/src/lib.rs +++ b/utils/src/lib.rs @@ -5,3 +5,6 @@ pub use function_call_visitor::FunctionCallVisitor; mod soroban_utils; pub use soroban_utils::*; + +mod lint_utils; +pub use lint_utils::*; diff --git a/utils/src/lint_utils/mod.rs b/utils/src/lint_utils/mod.rs new file mode 100644 index 00000000..30918b33 --- /dev/null +++ b/utils/src/lint_utils/mod.rs @@ -0,0 +1,13 @@ +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_span::Symbol; + +pub fn get_receiver_ident_name(receiver: &Expr) -> Symbol { + if let ExprKind::Path(QPath::Resolved(_, path)) = &receiver.kind { + let name = path.segments.first().map(|segment| segment.ident.name); + return name.unwrap_or(Symbol::intern("")); + } + Symbol::intern("") +} diff --git a/utils/src/soroban_utils/mod.rs b/utils/src/soroban_utils/mod.rs index 67c2d053..5816d63e 100644 --- a/utils/src/soroban_utils/mod.rs +++ b/utils/src/soroban_utils/mod.rs @@ -11,6 +11,7 @@ use rustc_span::def_id::DefId; /// Constants defining the fully qualified names of Soroban types. const SOROBAN_ENV: &str = "soroban_sdk::Env"; const SOROBAN_ADDRESS: &str = "soroban_sdk::Address"; +const SOROBAN_MAP: &str = "soroban_sdk::Map"; /// Determines whether a function defined by its `DefId` is part of a Soroban contract implementation. /// @@ -54,20 +55,26 @@ pub fn is_soroban_function( .all(|pattern| checked_functions.contains(pattern)) } -/// Checks if the provided type is a Soroban environment (`soroban_sdk::Env`). -pub fn is_soroban_env(cx: &LateContext<'_>, expr_type: Ty<'_>) -> bool { +// Private helper function to match soroban types +fn is_soroban_type(cx: &LateContext<'_>, expr_type: Ty<'_>, type_str: &str) -> bool { match expr_type.kind() { - TyKind::Adt(adt_def, _) => cx.tcx.def_path_str(adt_def.did()).contains(SOROBAN_ENV), - TyKind::Ref(_, ty, _) => is_soroban_env(cx, *ty), + TyKind::Adt(adt_def, _) => cx.tcx.def_path_str(adt_def.did()).contains(type_str), + TyKind::Ref(_, ty, _) => is_soroban_type(cx, *ty, type_str), _ => false, } } +/// Checks if the provided type is a Soroban environment (`soroban_sdk::Env`). +pub fn is_soroban_env(cx: &LateContext<'_>, expr_type: Ty<'_>) -> bool { + is_soroban_type(cx, expr_type, SOROBAN_ENV) +} + /// Checks if the provided type is a Soroban Address (`soroban_sdk::Address`). pub fn is_soroban_address(cx: &LateContext<'_>, expr_type: Ty<'_>) -> bool { - match expr_type.kind() { - TyKind::Adt(adt_def, _) => cx.tcx.def_path_str(adt_def.did()).contains(SOROBAN_ADDRESS), - TyKind::Ref(_, ty, _) => is_soroban_address(cx, *ty), - _ => false, - } + is_soroban_type(cx, expr_type, SOROBAN_ADDRESS) +} + +/// Checks if the provided type is a Soroban Map (`soroban_sdk::Map`). +pub fn is_soroban_map(cx: &LateContext<'_>, expr_type: Ty<'_>) -> bool { + is_soroban_type(cx, expr_type, SOROBAN_MAP) } From 35ae18b844b6ee54fb06b4a3c094015d316afb6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Garc=C3=ADa=20Crosta?= Date: Sat, 20 Apr 2024 14:17:05 -0300 Subject: [PATCH 3/5] Edit error on unsafe-expect --- detectors/unsafe-expect/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/detectors/unsafe-expect/src/lib.rs b/detectors/unsafe-expect/src/lib.rs index 9eccba05..e331b61a 100644 --- a/detectors/unsafe-expect/src/lib.rs +++ b/detectors/unsafe-expect/src/lib.rs @@ -4,7 +4,6 @@ extern crate rustc_hir; extern crate rustc_span; -use std::thread::current; use std::{collections::HashSet, hash::Hash}; use if_chain::if_chain; From c272080c6b9272e38ac5a14f6b0fad32d204496f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Garc=C3=ADa=20Crosta?= Date: Sat, 20 Apr 2024 14:18:35 -0300 Subject: [PATCH 4/5] Update remediated test --- .../unsafe-map-get-1/remediated-example/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/src/lib.rs b/test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/src/lib.rs index 4f0f41b8..bf60e514 100644 --- a/test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/src/lib.rs +++ b/test-cases/unsafe-map-get/unsafe-map-get-1/remediated-example/src/lib.rs @@ -1,4 +1,4 @@ -// #![no_std] +#![no_std] use soroban_sdk::{contract, contractimpl, map, Env, Error, IntoVal, Map, TryIntoVal, Val}; From 927106ffa004fbd5bfec1eee382be0362bf7f937 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Sat, 20 Apr 2024 14:40:53 -0300 Subject: [PATCH 5/5] Fix std error --- test-cases/unsafe-map-get/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-cases/unsafe-map-get/Cargo.toml b/test-cases/unsafe-map-get/Cargo.toml index 1bc3e364..efc07045 100644 --- a/test-cases/unsafe-map-get/Cargo.toml +++ b/test-cases/unsafe-map-get/Cargo.toml @@ -4,7 +4,7 @@ members = ["unsafe-map-get-*/*"] resolver = "2" [workspace.dependencies] -soroban-sdk = { version = "20.5.0" } +soroban-sdk = { version = "=20.0.0" } [profile.release] codegen-units = 1