diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index d6461a2e..c92c7289 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -99,6 +99,7 @@ jobs: [ "avoid-core-mem-forget", "avoid-panic-error", + "avoid-unsafe-block", "divide-before-multiply", "dos-unbounded-operation", "insufficiently-random-values", diff --git a/detectors/avoid-unsafe-block/Cargo.toml b/detectors/avoid-unsafe-block/Cargo.toml new file mode 100644 index 00000000..986e2c66 --- /dev/null +++ b/detectors/avoid-unsafe-block/Cargo.toml @@ -0,0 +1,19 @@ +[package] +name = "avoid-unsafe-block" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +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-unsafe-block/src/lib.rs b/detectors/avoid-unsafe-block/src/lib.rs new file mode 100644 index 00000000..92c26a2a --- /dev/null +++ b/detectors/avoid-unsafe-block/src/lib.rs @@ -0,0 +1,98 @@ +#![feature(rustc_private)] + +extern crate rustc_ast; +extern crate rustc_hir; +extern crate rustc_span; + +use rustc_hir::{ + intravisit::{walk_expr, Visitor}, + Expr, ExprKind, +}; +use rustc_lint::LateLintPass; +use rustc_span::Span; +use scout_audit_internal::Detector; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// Checks for usage of `unsafe` blocks. + /// + /// ### Why is this bad? + /// `unsafe` blocks should not be used unless absolutely necessary. + /// + /// ### Example + /// ```rust + ///pub fn unsafe_function(n: u64) -> u64 { + /// unsafe { + /// let mut i = n as f64; + /// let mut y = i.to_bits(); + /// y = 0x5fe6ec85e7de30da - (y >> 1); + /// i = f64::from_bits(y); + /// i *= 1.5 - 0.5 * n as f64 * i * i; + /// i *= 1.5 - 0.5 * n as f64 * i * i; + /// + /// let result_ptr: *mut f64 = &mut i; + /// let result = *result_ptr; + /// + /// result.to_bits() + /// } + ///} + /// Use instead: + /// ```rust + ///pub fn unsafe_function(n: u64) -> u64 { + /// let mut i = n as f64; + /// let mut y = i.to_bits(); + /// y = 0x5fe6ec85e7de30da - (y >> 1); + /// i = f64::from_bits(y); + /// i *= 1.5 - 0.5 * n as f64 * i * i; + /// i *= 1.5 - 0.5 * n as f64 * i * i; + /// result.to_bits() + ///} + /// ``` + pub AVOID_UNSAFE_BLOCK, + Warn, + Detector::AvoidUnsafeBlock.get_lint_message() +} + +impl<'tcx> LateLintPass<'tcx> for AvoidUnsafeBlock { + fn check_fn( + &mut self, + cx: &rustc_lint::LateContext<'tcx>, + _: rustc_hir::intravisit::FnKind<'tcx>, + _: &'tcx rustc_hir::FnDecl<'tcx>, + body: &'tcx rustc_hir::Body<'tcx>, + _: rustc_span::Span, + _: rustc_hir::def_id::LocalDefId, + ) { + struct UnsafeBlockVisitor { + unsafe_blocks: Vec>, + } + + impl<'tcx> Visitor<'tcx> for UnsafeBlockVisitor { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if let ExprKind::Block(block, _) = expr.kind { + if block.rules + == rustc_hir::BlockCheckMode::UnsafeBlock( + rustc_hir::UnsafeSource::UserProvided, + ) + { + self.unsafe_blocks.push(Some(expr.span)); + } + } + + walk_expr(self, expr); + } + } + + let mut visitor = UnsafeBlockVisitor { + unsafe_blocks: Vec::new(), + }; + + walk_expr(&mut visitor, body.value); + + visitor.unsafe_blocks.iter().for_each(|span| { + if let Some(span) = span { + Detector::AvoidUnsafeBlock.span_lint(cx, AVOID_UNSAFE_BLOCK, *span); + } + }); + } +} diff --git a/scout-audit-internal/src/detector.rs b/scout-audit-internal/src/detector.rs index 96c9ba88..b9b7a966 100644 --- a/scout-audit-internal/src/detector.rs +++ b/scout-audit-internal/src/detector.rs @@ -28,6 +28,7 @@ use strum::{Display, EnumIter}; pub enum Detector { AvoidCoreMemForget, AvoidPanicError, + AvoidUnsafeBlock, DivideBeforeMultiply, DosUnboundedOperation, InsufficientlyRandomValues, @@ -54,6 +55,7 @@ impl Detector { Detector::UnprotectedUpdateCurrentContractWasm => { UNPROTECTED_UPDATE_CURRENT_CONTRACT_LINT_MESSAGE } + Detector::AvoidUnsafeBlock => AVOID_UNSAFE_BLOCK_LINT_MESSAGE, Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_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 0e689e90..e4549536 100644 --- a/scout-audit-internal/src/detector/lint_message.rs +++ b/scout-audit-internal/src/detector/lint_message.rs @@ -1,12 +1,14 @@ pub const AVOID_CORE_MEM_FORGET_LINT_MESSAGE: &str = "Use the `let _ = ...` pattern or `.drop()` method to forget the value"; +pub const AVOID_UNSAFE_BLOCK_LINT_MESSAGE: &str = + "Avoid using unsafe blocks as it may lead to undefined behavior"; +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 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 DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE: &str = "Division before multiplication might result in a loss of precision"; pub const DOS_UNBOUNDED_OPERATION_LINT_MESSAGE: &str = "In order to prevent a single transaction from consuming all the gas in a block, unbounded operations must be avoided"; -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 SET_CONTRACT_STORAGE_LINT_MESSAGE:&str = "Abitrary users should not have control over keys because it implies writing any value of left mapping, lazy variable, or the main struct of the contract located in position 0 of the storage"; pub const SOROBAN_VERSION_LINT_MESSAGE: &str = "Use the latest version of Soroban"; diff --git a/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/remediated-example/Cargo.toml b/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..98b83cb8 --- /dev/null +++ b/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/remediated-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "avoid-unsafe-block-1-remediated" +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-unsafe-block/avoid-unsafe-block-1/remediated-example/src/lib.rs b/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..5913352e --- /dev/null +++ b/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/remediated-example/src/lib.rs @@ -0,0 +1,33 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl}; +#[contract] +pub struct AvoidUnsafeBlock; +#[contractimpl] +impl AvoidUnsafeBlock { + pub fn unsafe_function(n: u64) -> u64 { + let mut i = n as f64; + let mut y = i.to_bits(); + y = 0x5fe6ec85e7de30da - (y >> 1); + i = f64::from_bits(y); + i *= 1.5 - 0.5 * n as f64 * i * i; + i *= 1.5 - 0.5 * n as f64 * i * i; + i.to_bits() + } +} +#[cfg(test)] +mod tests { + use crate::AvoidUnsafeBlock; + #[test] + fn test_unsafe_block() { + let test_value = 8; + let result = AvoidUnsafeBlock::unsafe_function(test_value); + let inverse = inverse_square_root_without_unsafe(test_value); + + assert_eq!((inverse - result) / inverse, 0); + assert_eq!((inverse - result) / result, 0); + } + + fn inverse_square_root_without_unsafe(n: u64) -> u64 { + (1.0 / (n as f64).sqrt()).to_bits() + } +} diff --git a/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/vulnerable-example/Cargo.toml b/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..f12e1f70 --- /dev/null +++ b/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/vulnerable-example/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "avoid-unsafe-block-1-vulnerable" +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 \ No newline at end of file diff --git a/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/vulnerable-example/src/lib.rs b/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..87ad34c4 --- /dev/null +++ b/test-cases/avoid-unsafe-block/avoid-unsafe-block-1/vulnerable-example/src/lib.rs @@ -0,0 +1,38 @@ +#![no_std] +use soroban_sdk::{contract, contractimpl}; +#[contract] +pub struct AvoidUnsafeBlock; +#[contractimpl] +impl AvoidUnsafeBlock { + pub fn unsafe_function(n: u64) -> u64 { + unsafe { + let mut i = n as f64; + let mut y = i.to_bits(); + y = 0x5fe6ec85e7de30da - (y >> 1); + i = f64::from_bits(y); + i *= 1.5 - 0.5 * n as f64 * i * i; + i *= 1.5 - 0.5 * n as f64 * i * i; + + let result_ptr: *mut f64 = &mut i; + + (*result_ptr).to_bits() + } + } +} +#[cfg(test)] +mod tests { + use crate::AvoidUnsafeBlock; + #[test] + fn test_unsafe_block() { + let test_value = 8; + let result = AvoidUnsafeBlock::unsafe_function(test_value); + let inverse = inverse_square_root_without_unsafe(test_value); + + assert_eq!((inverse - result) / inverse, 0); + assert_eq!((inverse - result) / result, 0); + } + + fn inverse_square_root_without_unsafe(n: u64) -> u64 { + (1.0 / (n as f64).sqrt()).to_bits() + } +}