From 294dcd785dab6b9dfe4ed4aba0808b094d0d0dab Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Tue, 19 Dec 2023 12:37:28 -0300 Subject: [PATCH 1/3] Add test cases --- .../remediated-example/Cargo.toml | 30 ++++++++ .../remediated-example/src/lib.rs | 69 +++++++++++++++++++ .../vulnerable-example/Cargo.toml | 30 ++++++++ .../vulnerable-example/src/lib.rs | 63 +++++++++++++++++ 4 files changed, 192 insertions(+) create mode 100644 test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/Cargo.toml create mode 100644 test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/src/lib.rs create mode 100644 test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/Cargo.toml create mode 100644 test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/src/lib.rs diff --git a/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/Cargo.toml b/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..42dfaf5d --- /dev/null +++ b/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "avoid-panic-error-remediated-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "=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/avoid-panic-error/avoid-panic-error-1/remediated-example/src/lib.rs b/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..871aa92d --- /dev/null +++ b/test-cases/avoid-panic-error/avoid-panic-error-1/remediated-example/src/lib.rs @@ -0,0 +1,69 @@ +#![no_std] +use soroban_sdk::{contract, contracterror, contractimpl, symbol_short, Env, Symbol}; + +const COUNTER: Symbol = symbol_short!("COUNTER"); + +#[contract] +pub struct AvoidPanicError; + +#[contracterror] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +#[repr(u32)] +pub enum Error { + OverflowError = 1, +} + +#[contractimpl] +impl AvoidPanicError { + pub fn add(env: Env, value: u32) -> Result { + let storage = env.storage().instance(); + let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); + match count.checked_add(value) { + Some(value) => count = value, + None => return Err(Error::OverflowError), + } + storage.set(&COUNTER, &count); + storage.extend_ttl(100, 100); + Ok(count) + } +} + +#[cfg(test)] +mod tests { + use soroban_sdk::Env; + + use crate::{AvoidPanicError, AvoidPanicErrorClient, Error}; + + #[test] + fn add() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, AvoidPanicError); + let client = AvoidPanicErrorClient::new(&env, &contract_id); + + // When + let first_increment = client.try_add(&1); + let second_increment = client.try_add(&2); + let third_increment = client.try_add(&3); + + // Then + assert_eq!(first_increment, Ok(Ok(1))); + assert_eq!(second_increment, Ok(Ok(3))); + assert_eq!(third_increment, Ok(Ok(6))); + } + + #[test] + fn overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, AvoidPanicError); + let client = AvoidPanicErrorClient::new(&env, &contract_id); + + // When + let _max_value = client.try_add(&u32::max_value()); + let overflow = client.try_add(&1); + + // Then + assert_eq!(overflow, Err(Ok(Error::OverflowError))); + } +} diff --git a/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/Cargo.toml b/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..b611b66f --- /dev/null +++ b/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "avoid-panic-error-vulnerable-1" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = { version = "=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/avoid-panic-error/avoid-panic-error-1/vulnerable-example/src/lib.rs b/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..2ead32b9 --- /dev/null +++ b/test-cases/avoid-panic-error/avoid-panic-error-1/vulnerable-example/src/lib.rs @@ -0,0 +1,63 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl, symbol_short, Env, Symbol}; + +const COUNTER: Symbol = symbol_short!("COUNTER"); + +#[contract] +pub struct AvoidPanicError; + +#[contractimpl] +impl AvoidPanicError { + pub fn add(env: Env, value: u32) -> u32 { + let storage = env.storage().instance(); + let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); + match count.checked_add(value) { + Some(value) => count = value, + None => panic!("Overflow error"), + } + storage.set(&COUNTER, &count); + storage.extend_ttl(100, 100); + count + } +} + +#[cfg(test)] +mod tests { + use soroban_sdk::Env; + + use crate::{AvoidPanicError, AvoidPanicErrorClient}; + + #[test] + fn add() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, AvoidPanicError); + let client = AvoidPanicErrorClient::new(&env, &contract_id); + + // When + let first_increment = client.add(&1); + let second_increment = client.add(&2); + let third_increment = client.add(&3); + + // Then + assert_eq!(first_increment, 1); + assert_eq!(second_increment, 3); + assert_eq!(third_increment, 6); + } + + #[test] + #[should_panic(expected = "Overflow error")] + fn overflow() { + // Given + let env = Env::default(); + let contract_id = env.register_contract(None, AvoidPanicError); + let client = AvoidPanicErrorClient::new(&env, &contract_id); + + // When + client.add(&u32::max_value()); + client.add(&1); + + // Then + // panic + } +} From 0a515f8799bbf65e05ff24bd57f9be57d1f27ce3 Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Tue, 19 Dec 2023 12:37:54 -0300 Subject: [PATCH 2/3] Add detector and CI --- .github/workflows/test-detectors.yml | 1 + detectors/avoid-panic-error/Cargo.toml | 20 ++ detectors/avoid-panic-error/src/lib.rs | 178 ++++++++++++++++++ scout-audit-internal/src/detector.rs | 3 +- .../src/detector/lint_message.rs | 2 +- 5 files changed, 202 insertions(+), 2 deletions(-) create mode 100644 detectors/avoid-panic-error/Cargo.toml create mode 100644 detectors/avoid-panic-error/src/lib.rs diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index 95deb93c..69871140 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -98,6 +98,7 @@ jobs: test: [ "avoid-core-mem-forget", + "avoid-panic-error", "divide-before-multiply", "insufficiently-random-values", "overflow-check", diff --git a/detectors/avoid-panic-error/Cargo.toml b/detectors/avoid-panic-error/Cargo.toml new file mode 100644 index 00000000..953dcd46 --- /dev/null +++ b/detectors/avoid-panic-error/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "avoid-panic-error" +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/avoid-panic-error/src/lib.rs b/detectors/avoid-panic-error/src/lib.rs new file mode 100644 index 00000000..5bf77017 --- /dev/null +++ b/detectors/avoid-panic-error/src/lib.rs @@ -0,0 +1,178 @@ +#![feature(rustc_private)] +#![warn(unused_extern_crates)] + +extern crate rustc_ast; +extern crate rustc_span; + +use clippy_utils::sym; +use if_chain::if_chain; +use rustc_ast::{ + ptr::P, + token::{LitKind, TokenKind}, + tokenstream::{TokenStream, TokenTree}, + AttrArgs, AttrKind, Expr, ExprKind, Item, MacCall, StmtKind, +}; +use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_span::{sym, Span}; +use scout_audit_internal::Detector; + +dylint_linting::impl_pre_expansion_lint! { + /// ### What it does + /// The panic! macro is used to stop execution when a condition is not met. + /// This is useful for testing and prototyping, but should be avoided in production code + /// + /// ### Why is this bad? + /// The usage of panic! is not recommended because it will stop the execution of the caller contract. + /// + /// ### Known problems + /// While this linter detects explicit calls to panic!, there are some ways to raise a panic such as unwrap() or expect(). + /// + /// ### Example + /// ```rust + /// pub fn add(&mut self, value: u32) { + /// match self.value.checked_add(value) { + /// Some(v) => self.value = v, + /// None => panic!("Overflow error"), + /// }; + /// } + /// ``` + /// Use instead: + /// ```rust + /// pub fn add(&mut self, value: u32) -> Result<(), Error> { + /// match self.value.checked_add(value) { + /// Some(v) => self.value = v, + /// None => return Err(Error::OverflowError), + /// }; + /// Ok(()) + /// } + /// ``` + pub AVOID_PANIC_ERROR, + Warn, + Detector::AvoidPanicError.get_lint_message(), + AvoidPanicError::default() +} + +#[derive(Default)] +pub struct AvoidPanicError { + in_test_span: Option, +} + +impl EarlyLintPass for AvoidPanicError { + fn check_item(&mut self, _cx: &EarlyContext, item: &Item) { + match (is_test_item(item), self.in_test_span) { + (true, None) => self.in_test_span = Some(item.span), + (true, Some(test_span)) => { + if !test_span.contains(item.span) { + self.in_test_span = Some(item.span); + } + } + (false, None) => {} + (false, Some(test_span)) => { + if !test_span.contains(item.span) { + self.in_test_span = None; + } + } + }; + } + + fn check_stmt(&mut self, cx: &EarlyContext<'_>, stmt: &rustc_ast::Stmt) { + if_chain! { + if !self.in_test_item(); + if let StmtKind::MacCall(mac) = &stmt.kind; + then { + check_macro_call(cx, stmt.span, &mac.mac) + } + } + } + + fn check_expr(&mut self, cx: &EarlyContext, expr: &Expr) { + if_chain! { + if !self.in_test_item(); + if let ExprKind::MacCall(mac) = &expr.kind; + then { + check_macro_call(cx, expr.span, mac) + } + } + } +} + +fn check_macro_call(cx: &EarlyContext, span: Span, mac: &P) { + if_chain! { + if mac.path == sym!(panic); + if let [TokenTree::Token(token, _)] = mac + .args + .tokens + .clone() + .trees() + .collect::>() + .as_slice(); + if let TokenKind::Literal(lit) = token.kind; + if lit.kind == LitKind::Str; + then { + Detector::AvoidPanicError.span_lint_and_help( + cx, + AVOID_PANIC_ERROR, + span, + &format!("You could use instead an Error enum and then 'return Err(Error::{})'", capitalize_err_msg(lit.symbol.as_str()).replace(' ', "")), + ); + } + } +} + +fn is_test_item(item: &Item) -> bool { + item.attrs.iter().any(|attr| { + // Find #[cfg(all(test, feature = "e2e-tests"))] + if_chain!( + if let AttrKind::Normal(normal) = &attr.kind; + if let AttrArgs::Delimited(delim_args) = &normal.item.args; + if is_test_token_present(&delim_args.tokens); + then { + return true; + } + ); + + // Find unit or integration tests + if attr.has_name(sym::test) { + return true; + } + + 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 { + return true; + } + } + + false + }) +} + +impl AvoidPanicError { + fn in_test_item(&self) -> bool { + self.in_test_span.is_some() + } +} + +fn capitalize_err_msg(s: &str) -> String { + s.split_whitespace() + .map(|word| { + let mut chars = word.chars(); + match chars.next() { + None => String::new(), + Some(f) => f.to_uppercase().collect::() + chars.as_str(), + } + }) + .collect::>() + .join(" ") +} + +fn is_test_token_present(token_stream: &TokenStream) -> bool { + token_stream.trees().any(|tree| match tree { + TokenTree::Token(token, _) => token.is_ident_named(sym::test), + TokenTree::Delimited(_, _, token_stream) => is_test_token_present(token_stream), + }) +} diff --git a/scout-audit-internal/src/detector.rs b/scout-audit-internal/src/detector.rs index 9ba0cd89..48aa592c 100644 --- a/scout-audit-internal/src/detector.rs +++ b/scout-audit-internal/src/detector.rs @@ -27,6 +27,7 @@ use strum::{Display, EnumIter}; #[strum(serialize_all = "kebab-case")] pub enum Detector { AvoidCoreMemForget, + AvoidPanicError, DivideBeforeMultiply, InsufficientlyRandomValues, OverflowCheck, @@ -41,8 +42,8 @@ impl Detector { pub const fn get_lint_message(&self) -> &'static str { match self { Detector::AvoidCoreMemForget => AVOID_CORE_MEM_FORGET_LINT_MESSAGE, + Detector::AvoidPanicError => AVOID_PANIC_ERROR_LINT_MESSAGE, Detector::InsufficientlyRandomValues => INSUFFICIENTLY_RANDOM_VALUES_LINT_MESSAGE, - Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE, Detector::OverflowCheck => OVERFLOW_CHECK_LINT_MESSAGE, Detector::SetContractStorage => SET_CONTRACT_STORAGE_LINT_MESSAGE, diff --git a/scout-audit-internal/src/detector/lint_message.rs b/scout-audit-internal/src/detector/lint_message.rs index 23df1253..08d5a514 100644 --- a/scout-audit-internal/src/detector/lint_message.rs +++ b/scout-audit-internal/src/detector/lint_message.rs @@ -1,8 +1,8 @@ pub const AVOID_CORE_MEM_FORGET_LINT_MESSAGE: &str = "Use the `let _ = ...` pattern or `.drop()` method to forget the value"; +pub const AVOID_PANIC_ERROR_LINT_MESSAGE: &str = "The panic! macro is used to stop execution when a condition is not met. Even when this does not break the execution of the contract, it is recommended to use Result instead of panic! because it will stop the execution of the caller contract"; 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 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"; From 61dbca5368129fec00abb186beb1b215fdf2412c Mon Sep 17 00:00:00 2001 From: Jose Garcia Crosta Date: Tue, 19 Dec 2023 14:05:42 -0300 Subject: [PATCH 3/3] Update detector description --- detectors/avoid-panic-error/src/lib.rs | 29 +++++++++++++++++--------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/detectors/avoid-panic-error/src/lib.rs b/detectors/avoid-panic-error/src/lib.rs index 5bf77017..a6350bcf 100644 --- a/detectors/avoid-panic-error/src/lib.rs +++ b/detectors/avoid-panic-error/src/lib.rs @@ -29,21 +29,30 @@ dylint_linting::impl_pre_expansion_lint! { /// /// ### Example /// ```rust - /// pub fn add(&mut self, value: u32) { - /// match self.value.checked_add(value) { - /// Some(v) => self.value = v, - /// None => panic!("Overflow error"), - /// }; + /// pub fn add(env: Env, value: u32) -> u32 { + /// let storage = env.storage().instance(); + /// let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); + /// match count.checked_add(value) { + /// Some(value) => count = value, + /// None => panic!("Overflow error"), + /// } + /// storage.set(&COUNTER, &count); + /// storage.extend_ttl(100, 100); + /// count /// } /// ``` /// Use instead: /// ```rust - /// pub fn add(&mut self, value: u32) -> Result<(), Error> { - /// match self.value.checked_add(value) { - /// Some(v) => self.value = v, + /// pub fn add(env: Env, value: u32) -> Result { + /// let storage = env.storage().instance(); + /// let mut count: u32 = storage.get(&COUNTER).unwrap_or(0); + /// match count.checked_add(value) { + /// Some(value) => count = value, /// None => return Err(Error::OverflowError), - /// }; - /// Ok(()) + /// } + /// storage.set(&COUNTER, &count); + /// storage.extend_ttl(100, 100); + /// Ok(count) /// } /// ``` pub AVOID_PANIC_ERROR,