From 8a86f5bdf63b6d3f0d383e869abb2c09b25e66e6 Mon Sep 17 00:00:00 2001 From: Facundo Lerena Date: Thu, 30 Nov 2023 15:14:12 -0300 Subject: [PATCH 1/7] Add core-mem-forget detector and test case --- detectors/core-mem-forget/Cargo.toml | 20 ++++ detectors/core-mem-forget/src/lib.rs | 109 ++++++++++++++++++ scout-audit-internal/src/detector.rs | 2 + .../src/detector/lint_message.rs | 2 + .../remediated-example/Cargo.toml | 30 +++++ .../remediated-example/src/lib.rs | 42 +++++++ .../vulnerable-example/Cargo.toml | 30 +++++ .../vulnerable-example/src/lib.rs | 42 +++++++ 8 files changed, 277 insertions(+) create mode 100644 detectors/core-mem-forget/Cargo.toml create mode 100644 detectors/core-mem-forget/src/lib.rs create mode 100644 test-cases/core-mem-forget/core-mem-forget-1/remediated-example/Cargo.toml create mode 100644 test-cases/core-mem-forget/core-mem-forget-1/remediated-example/src/lib.rs create mode 100644 test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/Cargo.toml create mode 100644 test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/src/lib.rs diff --git a/detectors/core-mem-forget/Cargo.toml b/detectors/core-mem-forget/Cargo.toml new file mode 100644 index 00000000..ac154ea0 --- /dev/null +++ b/detectors/core-mem-forget/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "avoid-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..18101039 --- /dev/null +++ b/detectors/core-mem-forget/src/lib.rs @@ -0,0 +1,109 @@ +#![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_value(&mut self) { + /// let forgotten_value = self.value; + /// self.value = false; + /// core::mem::forget(forgotten_value); + /// } + /// + /// ``` + /// Use instead: + ///```rust + /// pub fn forget_value(&mut self) { + /// let forgotten_value = self.value; + /// self.value = false; + /// let _ = forgotten_value; + /// } + /// + /// // or use drop if droppable + /// + /// pub fn drop_value(&mut self) { + /// let forgotten_value = self.value; + /// self.value = false; + /// forget_value.drop(); + /// } + ///``` + + 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 3f6a9935..8123d5ee 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, OverflowCheck, UnsafeExpect, @@ -36,6 +37,7 @@ impl Detector { /// Returns the lint message for the detector. pub const fn get_lint_message(&self) -> &'static str { match self { + Detector::CoreMemForget => CORE_MEM_FORGET_LINT_MESSAGE, Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE, Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE, Detector::OverflowCheck => OVERFLOW_CHECK_LINT_MESSAGE, diff --git a/scout-audit-internal/src/detector/lint_message.rs b/scout-audit-internal/src/detector/lint_message.rs index a89f308c..cfb41728 100644 --- a/scout-audit-internal/src/detector/lint_message.rs +++ b/scout-audit-internal/src/detector/lint_message.rs @@ -1,3 +1,5 @@ +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 OVERFLOW_CHECK_LINT_MESSAGE: &str = "Use `overflow-checks = true` in Cargo.toml profile"; 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..949fbad6 --- /dev/null +++ b/test-cases/core-mem-forget/core-mem-forget-1/remediated-example/src/lib.rs @@ -0,0 +1,42 @@ +#![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, + }; + + let result = CoreMemForget::forget_something(test_value); + + 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..0608d7d1 --- /dev/null +++ b/test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/src/lib.rs @@ -0,0 +1,42 @@ +#![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, + }; + + let result = CoreMemForget::forget_something(test_value); + + assert_eq!(result, 0); + } + +} From fe54ae58ef6672e8c9745f0f57e233b17ef18e85 Mon Sep 17 00:00:00 2001 From: Facundo Lerena Date: Thu, 30 Nov 2023 15:15:12 -0300 Subject: [PATCH 2/7] Add core-mem-forget to ci --- .github/workflows/test-detectors.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index edad1258..1ab4fcaf 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", "overflow-check", "unprotected-update-current-contract-wasm", From 1e17ef8a3efbf7099bdeb29c8bd48fdeb51c4e2f Mon Sep 17 00:00:00 2001 From: Facundo Lerena Date: Thu, 30 Nov 2023 15:17:06 -0300 Subject: [PATCH 3/7] cargo fmt --- .../core-mem-forget-1/remediated-example/src/lib.rs | 9 +-------- .../core-mem-forget-1/vulnerable-example/src/lib.rs | 9 +-------- 2 files changed, 2 insertions(+), 16 deletions(-) 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 index 949fbad6..a7cf9ad0 100644 --- 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 @@ -4,7 +4,6 @@ use soroban_sdk::{contract, contractimpl, contracttype}; #[contract] pub struct CoreMemForget; - #[contracttype] #[derive(Eq, PartialEq)] pub struct WithoutCopy { @@ -20,8 +19,6 @@ impl CoreMemForget { } } - - #[cfg(test)] mod tests { use crate::*; @@ -29,14 +26,10 @@ mod tests { #[test] fn test_forget_something() { // Given - let test_value: WithoutCopy = WithoutCopy { - a: 80, - b: 60, - }; + let test_value: WithoutCopy = WithoutCopy { a: 80, b: 60 }; let result = CoreMemForget::forget_something(test_value); assert_eq!(result, 0); } - } 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 index 0608d7d1..93f36254 100644 --- 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 @@ -4,7 +4,6 @@ use soroban_sdk::{contract, contractimpl, contracttype}; #[contract] pub struct CoreMemForget; - #[contracttype] #[derive(Eq, PartialEq)] pub struct WithoutCopy { @@ -20,8 +19,6 @@ impl CoreMemForget { } } - - #[cfg(test)] mod tests { use crate::*; @@ -29,14 +26,10 @@ mod tests { #[test] fn test_forget_something() { // Given - let test_value: WithoutCopy = WithoutCopy { - a: 80, - b: 60, - }; + let test_value: WithoutCopy = WithoutCopy { a: 80, b: 60 }; let result = CoreMemForget::forget_something(test_value); assert_eq!(result, 0); } - } From c29be7d8e39080de2689f20365fd71da9551219f Mon Sep 17 00:00:00 2001 From: Facundo Lerena Date: Thu, 30 Nov 2023 15:21:24 -0300 Subject: [PATCH 4/7] why is the ci not running --- .../core-mem-forget-1/remediated-example/src/lib.rs | 1 - .../core-mem-forget-1/vulnerable-example/src/lib.rs | 1 - 2 files changed, 2 deletions(-) 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 index a7cf9ad0..a9a18872 100644 --- 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 @@ -25,7 +25,6 @@ mod tests { #[test] fn test_forget_something() { - // Given let test_value: WithoutCopy = WithoutCopy { a: 80, b: 60 }; let result = CoreMemForget::forget_something(test_value); 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 index 93f36254..2b6409d7 100644 --- 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 @@ -25,7 +25,6 @@ mod tests { #[test] fn test_forget_something() { - // Given let test_value: WithoutCopy = WithoutCopy { a: 80, b: 60 }; let result = CoreMemForget::forget_something(test_value); From 5b656a7a3008fde28d5d64b9730328ebec0c7958 Mon Sep 17 00:00:00 2001 From: Facundo Lerena Date: Thu, 30 Nov 2023 15:23:53 -0300 Subject: [PATCH 5/7] i forgot a , why --- .github/workflows/test-detectors.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index 1ab4fcaf..272548fb 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -97,7 +97,7 @@ jobs: - macos-latest test: [ - "core-mem-forget" + "core-mem-forget", "divide-before-multiply", "overflow-check", "unprotected-update-current-contract-wasm", From 6f6bf23e53491ea7d71f1ada494f24976992aa7a Mon Sep 17 00:00:00 2001 From: Facundo Lerena Date: Thu, 30 Nov 2023 15:25:08 -0300 Subject: [PATCH 6/7] removed avoid from name --- detectors/core-mem-forget/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/detectors/core-mem-forget/Cargo.toml b/detectors/core-mem-forget/Cargo.toml index ac154ea0..fc07fb1e 100644 --- a/detectors/core-mem-forget/Cargo.toml +++ b/detectors/core-mem-forget/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "avoid-core-mem-forget" +name = "core-mem-forget" version = "0.1.0" edition = "2021" From df517557be8a172e9d4b4f0669523ac678e3f426 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Mon, 4 Dec 2023 14:05:11 -0300 Subject: [PATCH 7/7] Update lint description --- detectors/core-mem-forget/src/lib.rs | 33 +++++++------------ .../remediated-example/src/lib.rs | 3 ++ .../vulnerable-example/src/lib.rs | 3 ++ 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/detectors/core-mem-forget/src/lib.rs b/detectors/core-mem-forget/src/lib.rs index 18101039..8d2f9c30 100644 --- a/detectors/core-mem-forget/src/lib.rs +++ b/detectors/core-mem-forget/src/lib.rs @@ -17,29 +17,19 @@ dylint_linting::impl_pre_expansion_lint! { /// This is a bad practice because it can lead to memory leaks, resource leaks and logic errors. /// ### Example /// ```rust - /// pub fn forget_value(&mut self) { - /// let forgotten_value = self.value; - /// self.value = false; - /// core::mem::forget(forgotten_value); - /// } + /// pub fn forget_something(n: WithoutCopy) -> u64 { + /// core::mem::forget(n); + /// 0 + /// } + /// ``` /// - /// ``` /// Use instead: - ///```rust - /// pub fn forget_value(&mut self) { - /// let forgotten_value = self.value; - /// self.value = false; - /// let _ = forgotten_value; - /// } - /// - /// // or use drop if droppable - /// - /// pub fn drop_value(&mut self) { - /// let forgotten_value = self.value; - /// self.value = false; - /// forget_value.drop(); - /// } - ///``` + /// ```rust + /// pub fn forget_something(n: WithoutCopy) -> u64 { + /// let _ = n; + /// 0 + /// } + /// ``` pub CORE_MEM_FORGET, Warn, @@ -69,7 +59,6 @@ impl EarlyLintPass for CoreMemForget { 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, 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 index a9a18872..0e6fab6b 100644 --- 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 @@ -25,10 +25,13 @@ mod tests { #[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/src/lib.rs b/test-cases/core-mem-forget/core-mem-forget-1/vulnerable-example/src/lib.rs index 2b6409d7..6d993d95 100644 --- 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 @@ -25,10 +25,13 @@ mod tests { #[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); } }