From baaa4a388feb49e60714cbd58870ee1fe34477a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Agust=C3=ADn=20Losiggio?= Date: Thu, 30 Nov 2023 15:12:06 -0300 Subject: [PATCH] 10 Unprotected update current contract wasm (#36) * first version of unprotected-update-current-contract detector * detector added to ci workflow * run cargo fmt * names fix --- .github/workflows/test-detectors.yml | 1 + .../Cargo.toml | 21 ++ .../src/lib.rs | 192 ++++++++++++++++++ scout-audit-internal/src/detector.rs | 4 + .../src/detector/lint_message.rs | 2 + .../remediated-example/Cargo.toml | 31 +++ .../remediated-example/src/lib.rs | 30 +++ .../vulnerable-example/Cargo.toml | 31 +++ .../vulnerable-example/src/lib.rs | 29 +++ 9 files changed, 341 insertions(+) create mode 100644 detectors/unprotected-update-current-contract-wasm/Cargo.toml create mode 100644 detectors/unprotected-update-current-contract-wasm/src/lib.rs create mode 100644 test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example/Cargo.toml create mode 100644 test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example/src/lib.rs create mode 100644 test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example/Cargo.toml create mode 100644 test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example/src/lib.rs diff --git a/.github/workflows/test-detectors.yml b/.github/workflows/test-detectors.yml index ed9cf030..edad1258 100644 --- a/.github/workflows/test-detectors.yml +++ b/.github/workflows/test-detectors.yml @@ -99,6 +99,7 @@ jobs: [ "divide-before-multiply", "overflow-check", + "unprotected-update-current-contract-wasm", "unsafe-expect", "unsafe-unwrap", ] diff --git a/detectors/unprotected-update-current-contract-wasm/Cargo.toml b/detectors/unprotected-update-current-contract-wasm/Cargo.toml new file mode 100644 index 00000000..485595bc --- /dev/null +++ b/detectors/unprotected-update-current-contract-wasm/Cargo.toml @@ -0,0 +1,21 @@ +[package] +name = "unprotected-update-current-contract-wasm" +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/unprotected-update-current-contract-wasm/src/lib.rs b/detectors/unprotected-update-current-contract-wasm/src/lib.rs new file mode 100644 index 00000000..a291c1d7 --- /dev/null +++ b/detectors/unprotected-update-current-contract-wasm/src/lib.rs @@ -0,0 +1,192 @@ +#![feature(rustc_private)] +#![feature(let_chains)] + +extern crate rustc_ast; +extern crate rustc_hir; +extern crate rustc_middle; +extern crate rustc_span; + +use rustc_hir::{ + intravisit::{walk_expr, Visitor}, + Expr, ExprKind, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::mir::{BasicBlock, BasicBlocks, Const, Operand, TerminatorKind}; +use rustc_middle::ty::TyKind; +use rustc_span::def_id::DefId; +use rustc_span::Span; +use scout_audit_internal::Detector; + +dylint_linting::impl_late_lint! { + pub UNPROTECTED_UPDATE_CURRENT_CONTRACT_WASM, + Warn, + Detector::UnprotectedUpdateCurrentContractWasm.get_lint_message(), + UnprotectedUpdateCurrentContractWasm::default() +} + +#[derive(Default)] +pub struct UnprotectedUpdateCurrentContractWasm {} +impl UnprotectedUpdateCurrentContractWasm { + pub fn new() -> Self { + Self {} + } +} + +impl<'tcx> LateLintPass<'tcx> for UnprotectedUpdateCurrentContractWasm { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: rustc_hir::intravisit::FnKind<'tcx>, + _: &'tcx rustc_hir::FnDecl<'tcx>, + body: &'tcx rustc_hir::Body<'tcx>, + _: Span, + localdef: rustc_span::def_id::LocalDefId, + ) { + struct UnprotectedUpdateFinder<'tcx, 'tcx_ref> { + cx: &'tcx_ref LateContext<'tcx>, + require_auth_def_id: Option, + update_contract_def_id: Option, + } + + impl<'tcx> Visitor<'tcx> for UnprotectedUpdateFinder<'tcx, '_> { + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if let ExprKind::MethodCall(path, receiver, ..) = expr.kind { + if path.ident.name.to_string() == "require_auth" { + self.require_auth_def_id = + self.cx.typeck_results().type_dependent_def_id(expr.hir_id); + } else if path.ident.name.to_string() == "update_current_contract_wasm" + && let ExprKind::MethodCall(path2, ..) = receiver.kind + && path2.ident.name.to_string() == "deployer" + { + self.update_contract_def_id = + self.cx.typeck_results().type_dependent_def_id(expr.hir_id); + } + } + + walk_expr(self, expr); + } + } + + let mut uuf_storage = UnprotectedUpdateFinder { + cx, + require_auth_def_id: None, + update_contract_def_id: None, + }; + + walk_expr(&mut uuf_storage, body.value); + + let mir_body = cx.tcx.optimized_mir(localdef); + + let spans = navigate_trough_basicblocks( + &mir_body.basic_blocks, + BasicBlock::from_u32(0), + false, + &uuf_storage, + ); + + for span in spans { + Detector::UnprotectedUpdateCurrentContractWasm.span_lint( + cx, + UNPROTECTED_UPDATE_CURRENT_CONTRACT_WASM, + span, + ) + } + + fn navigate_trough_basicblocks<'tcx>( + bbs: &'tcx BasicBlocks<'tcx>, + bb: BasicBlock, + auth_checked: bool, + uuf_storage: &UnprotectedUpdateFinder, + ) -> Vec { + let mut ret_vec: Vec = Vec::::new(); + if bbs[bb].terminator.is_none() { + return ret_vec; + } + let mut checked = auth_checked; + match &bbs[bb].terminator().kind { + TerminatorKind::Call { + func, + target, + fn_span, + .. + } => { + if let Operand::Constant(fn_const) = func + && let Const::Val(_const, ty) = fn_const.const_ + && let TyKind::FnDef(def, _) = ty.kind() + { + if uuf_storage.require_auth_def_id.is_some_and(|f| f == *def) { + checked = true; + } else if uuf_storage + .update_contract_def_id + .is_some_and(|f| f == *def) + && !checked + { + ret_vec.push(*fn_span); + } + } + if let Some(utarget) = target { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + *utarget, + checked, + uuf_storage, + )); + } + } + TerminatorKind::SwitchInt { targets, .. } => { + for target in targets.all_targets() { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + *target, + checked, + uuf_storage, + )); + } + } + TerminatorKind::Assert { target, .. } + | TerminatorKind::Goto { target, .. } + | TerminatorKind::Drop { target, .. } => { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + *target, + checked, + uuf_storage, + )); + } + TerminatorKind::Yield { resume, .. } => { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + *resume, + checked, + uuf_storage, + )); + } + TerminatorKind::FalseEdge { real_target, .. } + | TerminatorKind::FalseUnwind { real_target, .. } => { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + *real_target, + checked, + uuf_storage, + )); + } + TerminatorKind::InlineAsm { destination, .. } => { + if let Some(udestination) = destination { + ret_vec.append(&mut navigate_trough_basicblocks( + bbs, + *udestination, + checked, + uuf_storage, + )); + } + } + TerminatorKind::Return + | TerminatorKind::Unreachable + | TerminatorKind::GeneratorDrop + | TerminatorKind::UnwindResume + | TerminatorKind::UnwindTerminate(_) => {} + } + ret_vec + } + } +} diff --git a/scout-audit-internal/src/detector.rs b/scout-audit-internal/src/detector.rs index 3f6a9935..a6ed3fba 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 { DivideBeforeMultiply, OverflowCheck, + UnprotectedUpdateCurrentContractWasm, UnsafeExpect, UnsafeUnwrap, } @@ -39,6 +40,9 @@ impl Detector { Detector::DivideBeforeMultiply => DIVIDE_BEFORE_MULTIPLY_LINT_MESSAGE, Detector::UnsafeExpect => UNSAFE_EXPECT_LINT_MESSAGE, Detector::OverflowCheck => OVERFLOW_CHECK_LINT_MESSAGE, + Detector::UnprotectedUpdateCurrentContractWasm => { + UNPROTECTED_UPDATE_CURRENT_CONTRACT_MESSAGE + } Detector::UnsafeUnwrap => UNSAFE_UNWRAP_MESSAGE, } } diff --git a/scout-audit-internal/src/detector/lint_message.rs b/scout-audit-internal/src/detector/lint_message.rs index a89f308c..cf433c9e 100644 --- a/scout-audit-internal/src/detector/lint_message.rs +++ b/scout-audit-internal/src/detector/lint_message.rs @@ -1,5 +1,7 @@ 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"; +pub const UNPROTECTED_UPDATE_CURRENT_CONTRACT_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`"; diff --git a/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example/Cargo.toml b/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example/Cargo.toml new file mode 100644 index 00000000..ac03df7c --- /dev/null +++ b/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example/Cargo.toml @@ -0,0 +1,31 @@ + +[package] +name = "unprotected-update-current-contract-wasm" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "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 \ No newline at end of file diff --git a/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example/src/lib.rs b/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example/src/lib.rs new file mode 100644 index 00000000..178ad2f0 --- /dev/null +++ b/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/remediated-example/src/lib.rs @@ -0,0 +1,30 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, contracttype, Address, BytesN, Env}; + +#[contracttype] +#[derive(Clone)] +enum DataKey { + Admin, +} + +#[contract] +pub struct UpgradeableContract; + +#[contractimpl] +impl UpgradeableContract { + pub fn init(e: Env, admin: Address) { + e.storage().instance().set(&DataKey::Admin, &admin); + } + + pub fn version() -> u32 { + 1 + } + + pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) { + let admin: Address = e.storage().instance().get(&DataKey::Admin).unwrap(); + admin.require_auth(); + + e.deployer().update_current_contract_wasm(new_wasm_hash); + } +} diff --git a/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example/Cargo.toml b/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example/Cargo.toml new file mode 100644 index 00000000..ac03df7c --- /dev/null +++ b/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example/Cargo.toml @@ -0,0 +1,31 @@ + +[package] +name = "unprotected-update-current-contract-wasm" +version = "0.1.0" +edition = "2021" + +[lib] +crate-type = ["cdylib"] + +[dependencies] +soroban-sdk = "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 \ No newline at end of file diff --git a/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example/src/lib.rs b/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example/src/lib.rs new file mode 100644 index 00000000..003e0aaa --- /dev/null +++ b/test-cases/unprotected-update-current-contract-wasm/unprotected-update-current-contract-wasm-1/vulnerable-example/src/lib.rs @@ -0,0 +1,29 @@ +#![no_std] + +use soroban_sdk::{contract, contractimpl, contracttype, Address, BytesN, Env}; + +#[contracttype] +#[derive(Clone)] +enum DataKey { + Admin, +} + +#[contract] +pub struct UpgradeableContract; + +#[contractimpl] +impl UpgradeableContract { + pub fn init(e: Env, admin: Address) { + e.storage().instance().set(&DataKey::Admin, &admin); + } + + pub fn version() -> u32 { + 1 + } + + pub fn upgrade(e: Env, new_wasm_hash: BytesN<32>) { + let admin: Address = e.storage().instance().get(&DataKey::Admin).unwrap(); + + e.deployer().update_current_contract_wasm(new_wasm_hash); + } +}