diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index b91bb9bf..98dd3f85 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -97,6 +97,7 @@ jobs: - macos-latest test: [ + "core-mem-forget", "divide-before-multiply", "insufficiently-random-values", "overflow-check", diff --git a/detectors/core-mem-forget/Cargo.toml b/detectors/core-mem-forget/Cargo.toml new file mode 100644 index 00000000..fc07fb1e --- /dev/null +++ b/detectors/core-mem-forget/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "core-mem-forget" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +clippy_utils = { workspace = true } +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/core-mem-forget/src/lib.rs b/detectors/core-mem-forget/src/lib.rs new file mode 100644 index 00000000..8d2f9c30 --- /dev/null +++ b/detectors/core-mem-forget/src/lib.rs @@ -0,0 +1,98 @@ +#![feature(rustc_private)] + +extern crate rustc_ast; +extern crate rustc_hir; +extern crate rustc_span; + +use if_chain::if_chain; +use rustc_ast::{Expr, ExprKind, Item, NodeId}; +use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_span::sym; +use scout_audit_internal::Detector; + +dylint_linting::impl_pre_expansion_lint! { + /// ### What it does + /// Checks for `core::mem::forget` usage. + /// ### Why is this bad? + /// This is a bad practice because it can lead to memory leaks, resource leaks and logic errors. + /// ### Example + /// ```rust + /// pub fn forget_something(n: WithoutCopy) -> u64 { + /// core::mem::forget(n); + /// 0 + /// } + /// ``` + /// + /// Use instead: + /// ```rust + /// pub fn forget_something(n: WithoutCopy) -> u64 { + /// let _ = n; + /// 0 + /// } + /// ``` + + pub CORE_MEM_FORGET, + Warn, + Detector::CoreMemForget.get_lint_message(), + CoreMemForget::default() +} + +#[derive(Default)] +pub struct CoreMemForget { + stack: Vec, +} + +impl EarlyLintPass for CoreMemForget { + fn check_item(&mut self, _cx: &EarlyContext, item: &Item) { + if self.in_test_item() || is_test_item(item) { + self.stack.push(item.id); + } + } + + fn check_expr(&mut self, cx: &EarlyContext, expr: &Expr) { + if_chain! { + if !self.in_test_item(); + if let ExprKind::Call(a, _) = &expr.kind; + if let ExprKind::Path(_, path) = &a.kind; + if path.segments.len() == 3; + if path.segments[0].ident.name.to_string() == "core"; + if path.segments[1].ident.name.to_string() == "mem"; + if path.segments[2].ident.name.to_string() == "forget"; + then { + Detector::CoreMemForget.span_lint_and_help( + cx, + CORE_MEM_FORGET, + expr.span, + "Instead, use the `let _ = ...` pattern or `.drop` method to forget the value.", + ); + } + } + } +} + +fn is_test_item(item: &Item) -> bool { + item.attrs.iter().any(|attr| { + if attr.has_name(sym::test) { + true + } else { + if_chain! { + if attr.has_name(sym::cfg); + if let Some(items) = attr.meta_item_list(); + if let [item] = items.as_slice(); + if let Some(feature_item) = item.meta_item(); + if feature_item.has_name(sym::test); + then { + true + } else { + false + } + } + } + }) +} + +impl CoreMemForget { + fn in_test_item(&self) -> bool { + !self.stack.is_empty() + } +} diff --git a/scout-audit-internal/src/detector.rs b/scout-audit-internal/src/detector.rs index f6c58dab..5e523e1c 100644 --- a/scout-audit-internal/src/detector.rs +++ b/scout-audit-internal/src/detector.rs @@ -26,6 +26,7 @@ use strum::{Display, EnumIter}; #[derive(Debug, Display, Clone, EnumIter, PartialEq, Eq, Hash)] #[strum(serialize_all = "kebab-case")] pub enum Detector { + CoreMemForget, DivideBeforeMultiply, InsufficientlyRandomValues, OverflowCheck, @@ -38,14 +39,15 @@ impl Detector { /// Returns the lint message for the detector. pub const fn get_lint_message(&self) -> &'static str { match self { - Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE, + Detector::CoreMemForget => CORE_MEM_FORGET_LINT_MESSAGE, Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE, + Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE, Detector::OverflowCheck => OVERFLOW_CHECK_LINT_MESSAGE, Detector::UnprotectedUpdateCurrentContractWasm => { - UNPROTECTED_UPDATE_CURRENT_CONTRACT_MESSAGE + UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE } Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE, - Detector::UnsafeUnwrap => UNSAFE_UNWRAP_MESSAGE, + Detector::UnsafeUnwrap => UNSAFE_UNWRAP_LINT_MESSAGE, } } diff --git a/scout-audit-internal/src/detector/lint_message.rs b/scout-audit-internal/src/detector/lint_message.rs index 0b32f51e..bac80143 100644 --- a/scout-audit-internal/src/detector/lint_message.rs +++ b/scout-audit-internal/src/detector/lint_message.rs @@ -1,8 +1,10 @@ -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 CORE_MEM_FORGET_LINT_MESSAGE: &str = + "Use the `let _ = ...` pattern or `.drop()` method to forget the value"; pub const DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE: &str = "Division before multiplication might result in a loss of precision"; +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 UNPROTECTED_UPDATE_CURRENT_CONTRACT_MESSAGE: &str = +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`"; -pub const UNSAFE_UNWRAP_MESSAGE: &str = "Unsafe usage of `unwrap`"; +pub const UNSAFE_UNWRAP_LINT_MESSAGE: &str = "Unsafe usage of `unwrap`"; diff --git a/test-cases/core-mem-forget/core-mem-forget-1/remediated-example/Cargo.toml b/test-cases/core-mem-forget/core-mem-forget-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..9d6db30e --- /dev/null +++ b/test-cases/core-mem-forget/core-mem-forget-1/remediated-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "core-mem-forget-1-remediated" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0-rc2" } + +[dev_dependencies] +soroban-sdk = { version = "20.0.0-rc2", 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/core-mem-forget/core-mem-forget-1/remediated-example/src/lib.rs b/test-cases/core-mem-forget/core-mem-forget-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..0e6fab6b --- /dev/null +++ b/test-cases/core-mem-forget/core-mem-forget-1/remediated-example/src/lib.rs @@ -0,0 +1,37 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, contracttype}; + +#[contract] +pub struct CoreMemForget; + +#[contracttype] +#[derive(Eq, PartialEq)] +pub struct WithoutCopy { + pub a: u64, + pub b: u64, +} + +#[contractimpl] +impl CoreMemForget { + pub fn forget_something(n: WithoutCopy) -> u64 { + let _ = n; + 0 + } +} + +#[cfg(test)] +mod tests { + use crate::*; + + #[test] + fn test_forget_something() { + // Given + let test_value: WithoutCopy = WithoutCopy { a: 80, b: 60 }; + + // When + let result = CoreMemForget::forget_something(test_value); + + // Then + assert_eq!(result, 0); + } +} diff --git a/test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/Cargo.toml b/test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..9b848f19 --- /dev/null +++ b/test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "core-mem-forget-1-vulnerable" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "20.0.0-rc2" } + +[dev_dependencies] +soroban-sdk = { version = "20.0.0-rc2", 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/core-mem-forget/core-mem-forget-1/vulnerable-example/src/lib.rs b/test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..6d993d95 --- /dev/null +++ b/test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/src/lib.rs @@ -0,0 +1,37 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, contracttype}; + +#[contract] +pub struct CoreMemForget; + +#[contracttype] +#[derive(Eq, PartialEq)] +pub struct WithoutCopy { + pub a: u64, + pub b: u64, +} + +#[contractimpl] +impl CoreMemForget { + pub fn forget_something(n: WithoutCopy) -> u64 { + core::mem::forget(n); + 0 + } +} + +#[cfg(test)] +mod tests { + use crate::*; + + #[test] + fn test_forget_something() { + // Given + let test_value: WithoutCopy = WithoutCopy { a: 80, b: 60 }; + + // When + let result = CoreMemForget::forget_something(test_value); + + // Then + assert_eq!(result, 0); + } +}