From 7b4d571a005cacf010bc58961ce53411a5718946 Mon Sep 17 00:00:00 2001 From: Tarun Bansal Date: Wed, 24 Jan 2024 23:18:58 +0530 Subject: [PATCH] Add tx.origin detector (#55) * Add tx.origin detector to detect use of tx.origin in authentication checks * Update for new taint api * Update test * Update README.md * Update snapshot --------- Co-authored-by: Simone --- README.md | 11 +- src/detectors/mod.rs | 2 + src/detectors/tx_origin.rs | 224 ++++++++++++++++++ tests/detectors/tx_origin.cairo | 37 +++ ...tion_tests__detectors@tx_origin.cairo.snap | 19 ++ 5 files changed, 288 insertions(+), 5 deletions(-) create mode 100644 src/detectors/tx_origin.rs create mode 100644 tests/detectors/tx_origin.cairo create mode 100644 tests/snapshots/integration_tests__detectors@tx_origin.cairo.snap diff --git a/README.md b/README.md index 2e42b3e..625d3c1 100644 --- a/README.md +++ b/README.md @@ -92,11 +92,12 @@ Num | Detector | What it Detects | Impact | Confidence | Cairo 6 | `unused-events` | Events defined but not emitted | Medium | Medium | 1 & 2 7 | `unused-return` | Unused return values | Medium | Medium | 1 & 2 8 | `unenforced-view` | Function has view decorator but modifies state | Medium | Medium | 1 -9 | `unused-arguments` | Unused arguments | Low | Medium | 1 & 2 -10 | `reentrancy-benign` | Detect when a storage variable is written after an external call but not read before | Low | Medium | 1 & 2 -11 | `reentrancy-events` | Detect when an event is emitted after an external call leading to out-of-order events | Low | Medium | 1 & 2 -12 | `dead-code` | Private functions never used | Low | Medium | 1 & 2 -13 | `use-after-pop-front` | Detect use of an array or a span after removing element(s) | Low | Medium | 1 & 2 +9 | `tx-origin` | Detect usage of the transaction origin address as access control | Medium | Medium | 2 +10 | `unused-arguments` | Unused arguments | Low | Medium | 1 & 2 +11 | `reentrancy-benign` | Detect when a storage variable is written after an external call but not read before | Low | Medium | 1 & 2 +12 | `reentrancy-events` | Detect when an event is emitted after an external call leading to out-of-order events | Low | Medium | 1 & 2 +13 | `dead-code` | Private functions never used | Low | Medium | 1 & 2 +14 | `use-after-pop-front` | Detect use of an array or a span after removing element(s) | Low | Medium | 1 & 2 The Cairo column represent the compiler version(s) for which the detector is valid. diff --git a/src/detectors/mod.rs b/src/detectors/mod.rs index db3aabd..ef35939 100644 --- a/src/detectors/mod.rs +++ b/src/detectors/mod.rs @@ -8,6 +8,7 @@ pub mod read_only_reentrancy; pub mod reentrancy; pub mod reentrancy_benign; pub mod reentrancy_events; +pub mod tx_origin; pub mod unchecked_l1_handler_from; pub mod unused_arguments; pub mod unused_events; @@ -28,5 +29,6 @@ pub fn get_detectors() -> Vec> { Box::::default(), Box::::default(), Box::::default(), + Box::::default(), ] } diff --git a/src/detectors/tx_origin.rs b/src/detectors/tx_origin.rs new file mode 100644 index 0000000..4134b5f --- /dev/null +++ b/src/detectors/tx_origin.rs @@ -0,0 +1,224 @@ +use super::detector::{Confidence, Detector, Impact, Result}; +use crate::analysis::taint::WrapperVariable; +use crate::core::compilation_unit::CompilationUnit; +use crate::core::core_unit::CoreUnit; +use crate::core::function::Function; +use cairo_lang_sierra::extensions::core::CoreConcreteLibfunc; +use cairo_lang_sierra::extensions::felt252::Felt252Concrete; +use cairo_lang_sierra::extensions::structure::StructConcreteLibfunc; +use cairo_lang_sierra::ids::VarId; +use cairo_lang_sierra::program::{GenStatement, Statement as SierraStatement}; +use fxhash::FxHashSet; +use std::collections::HashSet; + +#[derive(Default)] +pub struct TxOrigin {} + +impl Detector for TxOrigin { + fn name(&self) -> &str { + "tx-origin" + } + + fn description(&self) -> &str { + "Detect usage of the transaction origin address as access control" + } + + fn confidence(&self) -> Confidence { + Confidence::Medium + } + + fn impact(&self) -> Impact { + Impact::Medium + } + + fn run(&self, core: &CoreUnit) -> HashSet { + let mut results: HashSet = HashSet::new(); + let compilation_units = core.get_compilation_units(); + + for compilation_unit in compilation_units.iter() { + for function in compilation_unit.functions_user_defined() { + let tx_origins: FxHashSet = function + .get_statements() + .iter() + .filter_map(|stmt| match stmt { + SierraStatement::Invocation(invoc) => { + let libfunc = compilation_unit + .registry() + .get_libfunc(&invoc.libfunc_id) + .expect("Library function not found in the registry"); + + match libfunc { + CoreConcreteLibfunc::Struct( + StructConcreteLibfunc::Deconstruct(struct_type), + ) => { + let struct_params: Vec = struct_type + .signature + .param_signatures + .iter() + .map(|s| s.ty.to_string()) + .collect(); + match &struct_params[..] { + [maybe_tx_info, ..] + if [ + "core::starknet::info::TxInfo", + "core::starknet::info::v2::TxInfo", + ] + .contains(&maybe_tx_info.as_str()) => + { + Some(WrapperVariable::new( + function.name(), + invoc.branches[0].results[1].id, + )) + } + _ => None, + } + } + _ => None, + } + } + _ => None, + }) + .collect(); + + let mut checked_private_functions = HashSet::new(); + let tx_origin_used = self.is_tx_origin_used_in_conditionals( + compilation_unit, + function, + &tx_origins, + &mut checked_private_functions, + ); + + if tx_origin_used { + let message = format!( + "The transaction origin contract address is used in an access control check in the function {}", + &function.name() + ); + results.insert(Result { + name: self.name().to_string(), + impact: self.impact(), + confidence: self.confidence(), + message, + }); + } + } + } + + results + } +} + +impl TxOrigin { + fn is_tx_origin_used_in_conditionals( + &self, + compilation_unit: &CompilationUnit, + function: &Function, + tx_origin_tainted_args: &FxHashSet, + checked_private_functions: &mut HashSet, + ) -> bool { + let tx_origin_checked = function + .get_statements() + .iter() + .filter_map(|stmt| match stmt { + SierraStatement::Invocation(invoc) => Some(invoc), + _ => None, + }) + .any(|invoc| { + let libfunc = compilation_unit + .registry() + .get_libfunc(&invoc.libfunc_id) + .expect("Library function not found in the registry"); + + match libfunc { + CoreConcreteLibfunc::Felt252(Felt252Concrete::IsZero(_)) => self + .is_felt252_is_zero_arg_taintaed_by_tx_origin( + compilation_unit, + tx_origin_tainted_args, + invoc.args.clone(), + &function.name(), + ), + _ => false, + } + }); + + let tx_origin_checked_in_private_functions = tx_origin_checked + || function.private_functions_calls().any(|s| { + if let GenStatement::Invocation(invoc) = s { + let lib_func = compilation_unit + .registry() + .get_libfunc(&invoc.libfunc_id) + .expect("Library function not found in the registry"); + + if let CoreConcreteLibfunc::FunctionCall(f_called) = lib_func { + let private_function = compilation_unit + .function_by_name(f_called.function.id.debug_name.as_ref().unwrap()) + .unwrap(); + if checked_private_functions.contains(&private_function.name()) { + return false; + } + + let taint = compilation_unit.get_taint(&function.name()).unwrap(); + + let sinks: FxHashSet = invoc + .args + .iter() + .map(|v| WrapperVariable::new(function.name(), v.id)) + .collect(); + + let tx_origin_tainted_args: FxHashSet = + tx_origin_tainted_args + .iter() + .flat_map(|source| taint.taints_any_sinks_variable(source, &sinks)) + .map(|sink| { + let mut var_id = None; + for (i, var) in invoc.args.iter().enumerate() { + if var.id == sink.variable() { + var_id = Some(i); + break; + } + } + if let Some(id) = var_id { + WrapperVariable::new( + private_function.name(), + id.try_into().unwrap(), + ) + } else { + println!( + "tx_origin: Did not find sink id, id could be wrong." + ); + // This is very likely to use a wrong var id + // but we should never enter this branch + WrapperVariable::new( + private_function.name(), + sink.variable() - invoc.args[0].id, + ) + } + }) + .collect(); + + checked_private_functions.insert(private_function.name()); + return self.is_tx_origin_used_in_conditionals( + compilation_unit, + private_function, + &tx_origin_tainted_args, + checked_private_functions, + ); + } + } + false + }); + + tx_origin_checked_in_private_functions + } + + fn is_felt252_is_zero_arg_taintaed_by_tx_origin( + &self, + compilation_unit: &CompilationUnit, + tx_origin_tainted_args: &FxHashSet, + felt252_is_zero_args: Vec, + function_name: &str, + ) -> bool { + let taint = compilation_unit.get_taint(function_name).unwrap(); + let sink = WrapperVariable::new(function_name.to_string(), felt252_is_zero_args[0].id); + taint.taints_any_sources(tx_origin_tainted_args, &sink) + } +} diff --git a/tests/detectors/tx_origin.cairo b/tests/detectors/tx_origin.cairo new file mode 100644 index 0000000..a85bcd1 --- /dev/null +++ b/tests/detectors/tx_origin.cairo @@ -0,0 +1,37 @@ +#[starknet::contract] +mod TxOrigin { + use core::box::BoxTrait; + use core::result::ResultTrait; + use starknet::{ ContractAddress, contract_address_const, get_tx_info }; + + #[storage] + struct Storage {} + + #[external(v0)] + fn bad(self: @ContractState) -> bool { + let tx_info = get_tx_info().unbox(); + let tx_origin = tx_info.account_contract_address; + let owner = contract_address_const::<1>(); + tx_origin == owner + } + + #[external(v0)] + fn bad_indirect(self: @ContractState) -> bool { + let tx_info = get_tx_info().unbox(); + let tx_origin = tx_info.account_contract_address; + _check_tx_origin(tx_origin) + } + + #[external(v0)] + fn good(self: @ContractState) -> ContractAddress { + let tx_info = get_tx_info().unbox(); + tx_info.account_contract_address + } + + + fn _check_tx_origin(tx_origin: ContractAddress) -> bool { + let owner = contract_address_const::<1>(); + tx_origin == owner + } + +} \ No newline at end of file diff --git a/tests/snapshots/integration_tests__detectors@tx_origin.cairo.snap b/tests/snapshots/integration_tests__detectors@tx_origin.cairo.snap new file mode 100644 index 0000000..01e76d7 --- /dev/null +++ b/tests/snapshots/integration_tests__detectors@tx_origin.cairo.snap @@ -0,0 +1,19 @@ +--- +source: tests/integration_tests.rs +expression: results +input_file: tests/detectors/tx_origin.cairo +--- +[ + Result { + impact: Medium, + name: "tx-origin", + confidence: Medium, + message: "The transaction origin contract address is used in an access control check in the function tx_origin::tx_origin::TxOrigin::bad", + }, + Result { + impact: Medium, + name: "tx-origin", + confidence: Medium, + message: "The transaction origin contract address is used in an access control check in the function tx_origin::tx_origin::TxOrigin::bad_indirect", + }, +]