diff --git a/src/cwe_checker_lib/src/analysis/fixpoint.rs b/src/cwe_checker_lib/src/analysis/fixpoint.rs index 78bae1ca2..f2f1c8eac 100644 --- a/src/cwe_checker_lib/src/analysis/fixpoint.rs +++ b/src/cwe_checker_lib/src/analysis/fixpoint.rs @@ -234,6 +234,16 @@ impl Computation { &self.node_values } + /// Get a mutable iterator over all node values. + /// Also add all nodes that have values to the worklist, because one can change all their values through the iterator. + pub fn node_values_mut(&mut self) -> impl Iterator { + for node in self.node_values.keys() { + let priority = self.node_priority_list[node.index()]; + self.worklist.insert(priority); + } + self.node_values.values_mut() + } + /// Get a reference to the underlying graph pub fn get_graph(&self) -> &DiGraph { self.fp_context.get_graph() diff --git a/src/cwe_checker_lib/src/analysis/function_signature/context/tests.rs b/src/cwe_checker_lib/src/analysis/function_signature/context/tests.rs index 4bec3677e..cc4807213 100644 --- a/src/cwe_checker_lib/src/analysis/function_signature/context/tests.rs +++ b/src/cwe_checker_lib/src/analysis/function_signature/context/tests.rs @@ -68,6 +68,7 @@ fn test_call_stub_handling() { &Tid::new("func"), &project.stack_pointer_register, project.get_standard_calling_convention().unwrap(), + 2, ); let extern_symbol = ExternSymbol::mock_malloc_symbol_arm(); let call_tid = Tid::new("call_malloc"); @@ -93,6 +94,7 @@ fn test_call_stub_handling() { &Tid::new("func"), &project.stack_pointer_register, project.get_standard_calling_convention().unwrap(), + 2, ); // Set the format string param register to a pointer to the string 'cat %s %s %s %s'. state.set_register(&variable!("r1:4"), bitvec!("0x6000:4").into()); diff --git a/src/cwe_checker_lib/src/analysis/function_signature/mod.rs b/src/cwe_checker_lib/src/analysis/function_signature/mod.rs index 3d74bdbd3..7a399f499 100644 --- a/src/cwe_checker_lib/src/analysis/function_signature/mod.rs +++ b/src/cwe_checker_lib/src/analysis/function_signature/mod.rs @@ -31,6 +31,9 @@ //! Nevertheless, it should result in an overapproximation of parameters and their access patterns in most cases. //! * The nesting depth for tracked nested parameters is limited //! to avoid generating infinitely many parameters for recursive types like linked lists. +//! * For arrays no parameters should be created for the array elements. +//! However, if only a particular element in an array is accessed without iteration over the array, +//! then a parameter might be generated for that element. use crate::abstract_domain::AbstractDomain; use crate::abstract_domain::AbstractLocation; @@ -55,16 +58,12 @@ mod global_var_propagation; use global_var_propagation::propagate_globals; pub mod stubs; -/// The recursion depth limit for abstract locations to be tracked by the function signature analysis, -/// i.e. how many dereference operations an abstract location is allowed to contain -/// before the analysis stops tracking the location. -const POINTER_RECURSION_DEPTH_LIMIT: u64 = 2; - /// Generate the computation object for the fixpoint computation /// and set the node values for all function entry nodes. fn generate_fixpoint_computation<'a>( project: &'a Project, graph: &'a Graph, + pointer_recursion_depth_limit: u64, ) -> Computation>> { let context = Context::new(project, graph); let mut computation = create_computation(context, None); @@ -81,6 +80,7 @@ fn generate_fixpoint_computation<'a>( &sub.tid, &project.stack_pointer_register, calling_convention, + pointer_recursion_depth_limit, ); if project.cpu_architecture.contains("MIPS") { let _ = fn_start_state @@ -152,8 +152,35 @@ pub fn compute_function_signatures<'a>( project: &'a Project, graph: &'a Graph, ) -> (BTreeMap, Vec) { - let mut computation = generate_fixpoint_computation(project, graph); + let max_pointer_recursion_depth_limit: u64 = 2; + // We gradually increase the recursion depth limit used in the fixpoint computation. + // The idea is that for array accesses the offset has time to converge to `Top` before IDs for nested objects are created. + // Otherwise the algorithm would generate an object for the first element of an array + // before it can check that it is an array access that we want to ignore. + let mut computation = generate_fixpoint_computation(project, graph, 0); computation.compute_with_max_steps(100); + for pointer_recursion_limit in 1..=max_pointer_recursion_depth_limit { + for node_value in computation.node_values_mut() { + match node_value { + NodeValue::Value(state) => { + state.set_pointer_recursion_depth_limit(pointer_recursion_limit) + } + NodeValue::CallFlowCombinator { + call_stub, + interprocedural_flow, + } => { + for state in call_stub.iter_mut() { + state.set_pointer_recursion_depth_limit(pointer_recursion_limit); + } + for state in interprocedural_flow.iter_mut() { + state.set_pointer_recursion_depth_limit(pointer_recursion_limit); + } + } + } + } + computation.compute_with_max_steps(100); + } + let mut fn_sig_map = extract_fn_signatures_from_fixpoint(project, graph, computation); // Sanitize the parameters let mut logs = Vec::new(); diff --git a/src/cwe_checker_lib/src/analysis/function_signature/state/memory_handling.rs b/src/cwe_checker_lib/src/analysis/function_signature/state/memory_handling.rs index cf8b494b1..6801f90ce 100644 --- a/src/cwe_checker_lib/src/analysis/function_signature/state/memory_handling.rs +++ b/src/cwe_checker_lib/src/analysis/function_signature/state/memory_handling.rs @@ -1,5 +1,4 @@ use super::State; -use super::POINTER_RECURSION_DEPTH_LIMIT; use crate::abstract_domain::*; use crate::intermediate_representation::*; @@ -48,7 +47,7 @@ impl State { Err(_) => DataDomain::new_top(size), } } else if let (true, Ok(constant_offset)) = ( - id.get_location().recursion_depth() < POINTER_RECURSION_DEPTH_LIMIT, + id.get_location().recursion_depth() < self.pointer_recursion_depth_limit, offset.try_to_offset(), ) { // Extend the abstract location string diff --git a/src/cwe_checker_lib/src/analysis/function_signature/state/mod.rs b/src/cwe_checker_lib/src/analysis/function_signature/state/mod.rs index e54d4d6ed..08d7d7938 100644 --- a/src/cwe_checker_lib/src/analysis/function_signature/state/mod.rs +++ b/src/cwe_checker_lib/src/analysis/function_signature/state/mod.rs @@ -1,5 +1,4 @@ use super::AccessPattern; -use super::POINTER_RECURSION_DEPTH_LIMIT; use crate::abstract_domain::*; use crate::intermediate_representation::*; use crate::prelude::*; @@ -27,6 +26,10 @@ pub struct State { stack: MemRegion>, /// Maps each tracked ID to an [`AccessPattern`], which tracks known access patterns to the object. tracked_ids: DomainMap, + /// The recursion depth limit for abstract locations to be tracked by the function signature analysis, + /// i.e. how many dereference operations an abstract location is allowed to contain + /// before the analysis stops tracking the location. + pointer_recursion_depth_limit: u64, } impl State { @@ -37,6 +40,7 @@ impl State { func_tid: &Tid, stack_register: &Variable, calling_convention: &CallingConvention, + pointer_recursion_depth_limit: u64, ) -> State { let mut register_map = BTreeMap::new(); let mut tracked_ids = BTreeMap::new(); @@ -64,9 +68,18 @@ impl State { stack_id, stack, tracked_ids: DomainMap::from(tracked_ids), + pointer_recursion_depth_limit, } } + /// Set the pointer recursion depth limit to the provided value. + /// + /// Note that one should call this function for all states, + /// because merging two states with different depth limits will panic. + pub fn set_pointer_recursion_depth_limit(&mut self, limit: u64) { + self.pointer_recursion_depth_limit = limit; + } + /// Set the MIPS link register `t9` to the address of the function TID. /// /// According to the System V ABI for MIPS the caller has to save the callee address in register `t9` @@ -219,7 +232,7 @@ impl State { } }; location.extend(mem_location, self.stack_id.bytesize()); - if location.recursion_depth() <= POINTER_RECURSION_DEPTH_LIMIT { + if location.recursion_depth() <= self.pointer_recursion_depth_limit { eval_result = eval_result.merge(&DataDomain::from_target( AbstractIdentifier::new(id.get_tid().clone(), location), Bitvector::zero(target_size.into()).into(), @@ -382,6 +395,10 @@ fn get_pointer_inputs_vars_of_address_expression(expr: &Expression) -> Vec<&Vari impl AbstractDomain for State { /// Merge two states fn merge(&self, other: &Self) -> Self { + assert_eq!( + self.pointer_recursion_depth_limit, + other.pointer_recursion_depth_limit + ); let stack_id = self.stack_id.clone(); let stack = self.stack.merge(&other.stack); State { @@ -389,6 +406,7 @@ impl AbstractDomain for State { stack_id, stack, tracked_ids: self.tracked_ids.merge(&other.tracked_ids), + pointer_recursion_depth_limit: self.pointer_recursion_depth_limit, } } diff --git a/src/cwe_checker_lib/src/analysis/function_signature/state/tests.rs b/src/cwe_checker_lib/src/analysis/function_signature/state/tests.rs index 36749ea10..81eb6e7cd 100644 --- a/src/cwe_checker_lib/src/analysis/function_signature/state/tests.rs +++ b/src/cwe_checker_lib/src/analysis/function_signature/state/tests.rs @@ -1,5 +1,10 @@ use super::*; -use crate::{bitvec, expr, variable}; +use crate::{ + analysis::{ + forward_interprocedural_fixpoint::Context as _, function_signature::context::Context, + }, + bitvec, defs, expr, variable, +}; impl State { /// Generate a mock state for an ARM-32 state. @@ -8,6 +13,7 @@ impl State { &Tid::new("mock_fn"), &variable!("sp:4"), &CallingConvention::mock_arm32(), + 2, ) } @@ -17,6 +23,7 @@ impl State { &Tid::new(tid_name), &variable!("RSP:8"), &CallingConvention::mock_x64(), + 2, ) } } @@ -137,3 +144,52 @@ fn test_substitute_global_mem_address() { DataDomain::from_target(expected_global_id, bitvec!("0x0:4").into()) ); } + +#[test] +fn test_pointer_recursion_depth_limit_handling() { + let project = Project::mock_arm32(); + let graph = crate::analysis::graph::get_program_cfg(&project.program); + let context = Context::new(&project, &graph); + // Test interaction of gradually increasing the pointer recursion depth limit with a loop that + // - iterates over an array + // - recursively dereferences a variable + let mut state = State::mock_arm32(); + let defs = defs![ + "instr_1: Store at r1:4 := r0:4", + "instr_2: r1:4 = r1:4 + 0x1:4", + "instr_3: r3:4 := Load from r3:4" + ]; + let array_elem_location = AbstractLocation::mock("r1:4", &[0], 4); + let array_elem_id = AbstractIdentifier::new( + state.get_current_function_tid().clone(), + array_elem_location, + ); + let recursive_elem_location = AbstractLocation::mock("r3:4", &[0], 4); + let recursive_elem_id = AbstractIdentifier::new( + state.get_current_function_tid().clone(), + recursive_elem_location, + ); + // Iteration with depth limit 0 + state.pointer_recursion_depth_limit = 0; + let prev_state = state.clone(); + for def in &defs { + state = context.update_def(&state, def).unwrap(); + } + state = state.merge(&prev_state); + // No array element ID should have been created. + assert!(state.tracked_ids.get(&array_elem_id).is_none()); + // No recursive access ID should have been created. + assert!(state.tracked_ids.get(&recursive_elem_id).is_none()); + + // Iteration with depth limit 1 + state.pointer_recursion_depth_limit = 1; + let prev_state = state.clone(); + for def in &defs { + state = context.update_def(&state, def).unwrap(); + } + state = state.merge(&prev_state); + // No array element ID should have been created. + assert!(state.tracked_ids.get(&array_elem_id).is_none()); + // But the recursive access ID should now exist. + assert!(state.tracked_ids.get(&recursive_elem_id).is_some()); +}