Merge pull request #152 from CoinFabrik/62-implement-try_get-detector…

…-for-mappings

62 implement try get detector for mappings

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" }

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

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);
+    }
+}

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.0.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"

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"]

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<Option<i32>, Error> {
+        let map: Map<Val, Val> = 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<i32, i32> = 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<Option<i32>, Error> {
+        let map: Map<Val, Val> = 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<i32, i32> = 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
+            )))
+        );
+    }
+}

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"]

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<i32> {
+        let map: Map<Val, Val> = map![&env, (1i32.into_val(&env), 2i64.into_val(&env))];
+        let map: Val = map.into();
+        let map: Map<i32, i32> = 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
+    }
+}

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::*;

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("")
+}

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)
 }