unsafe-block test case and detector (#38)

* Added unsafe-block test cases

* Added unsafe-block detector

---------

Co-authored-by: Agustín Losiggio <[email protected]>

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

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

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

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

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

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

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

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

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