Cwe476: Less false positives via better parameter checks (#115)

cwe_checker_rs/src/abstract_domain/mem_region.rs

@@ -144,6 +144,15 @@ impl<T: AbstractDomain + HasByteSize + RegisterDomain + std::fmt::Debug> MemRegi
         T::new_top(size_in_bytes)
     }
 
+    /// Get the value at the given position regardless of the value size.
+    /// Return `None` if there is no value at that position in the memory region.
+    pub fn get_unsized(&self, position: Bitvector) -> Option<T> {
+        assert_eq!(ByteSize::from(position.width()), self.address_bytesize);
+        let position = Int::from(position).try_to_i64().unwrap();
+
+        self.values.get(&position).cloned()
+    }
+
     /// Remove all elements intersecting the provided interval.
     pub fn remove(&mut self, position: Bitvector, size_in_bytes: Bitvector) {
         assert_eq!(ByteSize::from(position.width()), self.address_bytesize);

cwe_checker_rs/src/analysis/pointer_inference/mod.rs

@@ -25,7 +25,7 @@ use petgraph::Direction;
 use std::collections::HashMap;
 
 mod context;
-mod object;
+pub mod object;
 mod object_list;
 mod state;
 

cwe_checker_rs/src/analysis/pointer_inference/object.rs

@@ -171,6 +171,11 @@ impl AbstractObjectInfo {
         self.state
     }
 
+    /// Get the type of the memory object.
+    pub fn get_object_type(&self) -> Option<ObjectType> {
+        self.type_
+    }
+
     /// Invalidates all memory and adds the `additional_targets` to the pointer targets.
     /// Represents the effect of unknown write instructions to the object
     /// which may include writing pointers to targets from the `additional_targets` set to the object.

cwe_checker_rs/src/analysis/pointer_inference/object_list.rs

@@ -291,6 +291,18 @@ impl AbstractObjectList {
             object.remove_ids(ids_to_remove);
         }
     }
+
+    // Return the object type of a memory object.
+    // Returns an error if no object with the given ID is contained in the object list.
+    pub fn get_object_type(
+        &self,
+        object_id: &AbstractIdentifier,
+    ) -> Result<Option<ObjectType>, ()> {
+        match self.objects.get(object_id) {
+            Some((object, _)) => Ok(object.get_object_type()),
+            None => Err(()),
+        }
+    }
 }
 
 impl AbstractDomain for AbstractObjectList {

cwe_checker_rs/src/checkers/cwe_476/context.rs

@@ -168,50 +168,78 @@ impl<'a> Context<'a> {
         let _ = self.cwe_collector.send(cwe_warning);
     }
 
-    /// Check whether the given function parameter contains taint
-    /// when evaluating it on the given state.
-    ///
-    /// The `node_id` is used to find the correct pointer inference state.
-    pub fn check_parameter_arg_for_taint(
+    /// Check parameters of an extern symbol for taint.
+    /// For pointers as parameters we also check
+    /// whether the pointer points directly to taint if it points to some stack address.
+    /// or whether the pointed to object contains any taint at all if it is not a stack object.
+    pub fn check_parameters_for_taint(
         &self,
-        parameter: &Arg,
         state: &State,
+        extern_symbol: &ExternSymbol,
         node_id: NodeIndex,
-    ) -> Taint {
-        match parameter {
-            Arg::Register(var) => state.eval(&Expression::Var(var.clone())),
-            Arg::Stack { offset, size } => {
-                if let Some(NodeValue::Value(pi_state)) =
-                    self.pointer_inference_results.get_node_value(node_id)
-                {
-                    if let Ok(stack_address) = pi_state.eval(&Expression::BinOp {
-                        op: BinOpType::IntAdd,
-                        lhs: Box::new(Expression::Var(self.project.stack_pointer_register.clone())),
-                        rhs: Box::new(Expression::Const(
-                            Bitvector::from_i64(*offset)
-                                .into_truncate(apint::BitWidth::from(
-                                    self.project.stack_pointer_register.size,
-                                ))
-                                .unwrap(),
-                        )),
-                    }) {
-                        state.load_taint_from_memory(&stack_address, *size)
-                    } else {
-                        Taint::Top(*size)
+    ) -> bool {
+        // First check for taint directly in parameter registers (we don't need a pointer inference state for that)
+        for parameter in extern_symbol.parameters.iter() {
+            if let Arg::Register(var) = parameter {
+                if state.eval(&Expression::Var(var.clone())).is_tainted() {
+                    return true;
+                }
+            }
+        }
+        if let Some(NodeValue::Value(pi_state)) =
+            self.pointer_inference_results.get_node_value(node_id)
+        {
+            // Check stack parameters and collect referenced memory object that need to be checked for taint.
+            for parameter in extern_symbol.parameters.iter() {
+                match parameter {
+                    Arg::Register(var) => {
+                        if let Ok(data) = pi_state.eval(&Expression::Var(var.clone())) {
+                            if state.check_if_address_points_to_taint(data, pi_state) {
+                                return true;
+                            }
+                        }
+                    }
+                    Arg::Stack { offset, size } => {
+                        if let Ok(stack_address) = pi_state.eval(&Expression::BinOp {
+                            op: BinOpType::IntAdd,
+                            lhs: Box::new(Expression::Var(
+                                self.project.stack_pointer_register.clone(),
+                            )),
+                            rhs: Box::new(Expression::Const(
+                                Bitvector::from_i64(*offset)
+                                    .into_truncate(apint::BitWidth::from(
+                                        self.project.stack_pointer_register.size,
+                                    ))
+                                    .unwrap(),
+                            )),
+                        }) {
+                            if state
+                                .load_taint_from_memory(&stack_address, *size)
+                                .is_tainted()
+                            {
+                                return true;
+                            }
+                        }
+                        if let Ok(stack_param) = pi_state
+                            .eval_parameter_arg(parameter, &self.project.stack_pointer_register)
+                        {
+                            if state.check_if_address_points_to_taint(stack_param, pi_state) {
+                                return true;
+                            }
+                        }
                     }
-                } else {
-                    Taint::Top(*size)
                 }
             }
         }
+        false
     }
 
     /// If a possible  parameter register of the call contains taint,
     /// generate a CWE warning and return `None`.
     /// Else remove all taint contained in non-callee-saved registers.
     fn handle_generic_call(&self, state: &State, call_tid: &Tid) -> Option<State> {
-        // TODO: We do not yet check recursively for taint contained in objects pointed to by parameters.
-        if state.check_generic_function_params_for_taint(self.project) {
+        let pi_state_option = self.get_current_pointer_inference_state(state, call_tid);
+        if state.check_generic_function_params_for_taint(self.project, pi_state_option.as_ref()) {
             self.generate_cwe_warning(call_tid);
             return None;
         }
@@ -249,17 +277,16 @@ impl<'a> crate::analysis::interprocedural_fixpoint::Context<'a> for Context<'a>
     /// Generate a CWE warning if taint may be contained in the function parameters.
     /// Always returns `None` so that the analysis stays intraprocedural.
     fn update_call(&self, state: &State, call: &Term<Jmp>, _target: &Node) -> Option<Self::Value> {
-        if state.check_generic_function_params_for_taint(self.project) {
+        let pi_state_option = self.get_current_pointer_inference_state(state, &call.tid);
+        if state.check_generic_function_params_for_taint(self.project, pi_state_option.as_ref()) {
             self.generate_cwe_warning(&call.tid);
         }
-        // TODO: We do not yet check recursively for taint contained in objects pointed to by parameters.
         None
     }
 
     /// If taint may be contained in the function parameters, generate a CWE warning and return None.
     /// Else remove taint from non-callee-saved registers.
     fn update_call_stub(&self, state: &State, call: &Term<Jmp>) -> Option<Self::Value> {
-        // TODO: We do not yet check recursively for taint contained in objects pointed to by parameters.
         if state.is_empty() {
             return None;
         }
@@ -270,14 +297,9 @@ impl<'a> crate::analysis::interprocedural_fixpoint::Context<'a> for Context<'a>
                         .jmp_to_blk_end_node_map
                         .get(&(call.tid.clone(), self.current_sub.unwrap().tid.clone()))
                         .unwrap();
-                    for parameter in extern_symbol.parameters.iter() {
-                        if !self
-                            .check_parameter_arg_for_taint(parameter, state, *blk_end_node_id)
-                            .is_top()
-                        {
-                            self.generate_cwe_warning(&call.tid);
-                            return None;
-                        }
+                    if self.check_parameters_for_taint(state, extern_symbol, *blk_end_node_id) {
+                        self.generate_cwe_warning(&call.tid);
+                        return None;
                     }
                     let mut new_state = state.clone();
                     new_state.remove_non_callee_saved_taint(
@@ -389,8 +411,9 @@ impl<'a> crate::analysis::interprocedural_fixpoint::Context<'a> for Context<'a>
     ) -> Option<State> {
         if let Some(state) = state_before_return {
             // If taint is returned, generate a CWE warning
-            if !state.is_empty() {
-                self.generate_cwe_warning(&return_term.tid)
+            let pi_state_option = self.get_current_pointer_inference_state(state, &return_term.tid);
+            if state.check_return_values_for_taint(self.project, pi_state_option.as_ref()) {
+                self.generate_cwe_warning(&return_term.tid);
             }
             // Do not return early in case `state_before_call` is also set (possible for recursive functions).
         }
@@ -433,15 +456,21 @@ mod tests {
         let project = Project::mock_empty();
         let pi_results = PointerInferenceComputation::mock(&project);
         let context = Context::mock(&project, &pi_results);
-        let (state, _pi_state) = State::mock_with_pi_state();
+        let (mut state, _pi_state) = State::mock_with_pi_state();
 
-        let arg = Arg::Register(Variable::mock("RAX", 8u64));
-        let param_taint = context.check_parameter_arg_for_taint(&arg, &state, NodeIndex::new(0));
-        assert!(param_taint.is_tainted());
+        assert_eq!(
+            context.check_parameters_for_taint(&state, &ExternSymbol::mock(), NodeIndex::new(0)),
+            false
+        );
 
-        let arg = Arg::Register(Variable::mock("RBX", 8u64));
-        let param_taint = context.check_parameter_arg_for_taint(&arg, &state, NodeIndex::new(0));
-        assert!(!param_taint.is_tainted());
+        state.set_register_taint(
+            &Variable::mock("RDI", ByteSize::new(8)),
+            Taint::Tainted(ByteSize::new(8)),
+        );
+        assert_eq!(
+            context.check_parameters_for_taint(&state, &ExternSymbol::mock(), NodeIndex::new(0)),
+            true
+        );
     }
 
     #[test]

cwe_checker_rs/src/checkers/cwe_476/state.rs

@@ -222,25 +222,114 @@ impl State {
         }
     }
 
+    /// Return true if the memory object with the given ID contains a tainted value.
+    pub fn check_mem_id_for_taint(&self, id: &AbstractIdentifier) -> bool {
+        if let Some(mem_object) = self.memory_taint.get(&id) {
+            for elem in mem_object.values() {
+                if elem.is_tainted() {
+                    return true;
+                }
+            }
+        }
+        false
+    }
+
+    /// If the given address points to the stack,
+    /// return true if and only if the value at that stack position is tainted.
+    /// If the given address points to a non-stack memory object,
+    /// return true if the memory object contains any tainted value (at any position).
+    pub fn check_if_address_points_to_taint(
+        &self,
+        address: Data,
+        pi_state: &PointerInferenceState,
+    ) -> bool {
+        use crate::analysis::pointer_inference::object::ObjectType;
+        if let Data::Pointer(pointer) = address {
+            for (target, offset) in pointer.targets() {
+                if let Ok(Some(ObjectType::Stack)) = pi_state.memory.get_object_type(target) {
+                    // Only check if the value at the address is tainted
+                    if let (Some(mem_object), BitvectorDomain::Value(target_offset)) =
+                        (self.memory_taint.get(target), offset)
+                    {
+                        if let Some(taint) = mem_object.get_unsized(target_offset.clone()) {
+                            if taint.is_tainted() {
+                                return true;
+                            }
+                        }
+                    }
+                } else {
+                    // Check whether the memory object contains any taint.
+                    if self.check_mem_id_for_taint(target) {
+                        return true;
+                    }
+                }
+            }
+        }
+        false
+    }
+
+    /// Check whether a register in the given register list contains taint.
+    /// Return `true` if taint was found and `false` if no taint was found.
+    fn check_register_list_for_taint(
+        &self,
+        register_list: &[String],
+        pi_state_option: Option<&PointerInferenceState>,
+    ) -> bool {
+        // Check whether a register contains taint
+        for (register, taint) in &self.register_taint {
+            if register_list
+                .iter()
+                .any(|reg_name| *reg_name == register.name)
+                && !taint.is_top()
+            {
+                return true;
+            }
+        }
+        // Check whether some memory object referenced by a register may contain taint
+        if let Some(pi_state) = pi_state_option {
+            for register_name in register_list {
+                if let Some(register_value) = pi_state.get_register_by_name(register_name) {
+                    if self.check_if_address_points_to_taint(register_value, pi_state) {
+                        return true;
+                    }
+                }
+            }
+        }
+        false
+    }
+
     /// Check whether a generic function call may contain tainted values in its parameters.
     /// Since we don't know the actual calling convention of the call,
     /// we approximate the parameters with all parameter registers of the standard calling convention of the project.
-    pub fn check_generic_function_params_for_taint(&self, project: &Project) -> bool {
+    pub fn check_generic_function_params_for_taint(
+        &self,
+        project: &Project,
+        pi_state_option: Option<&PointerInferenceState>,
+    ) -> bool {
         if let Some(calling_conv) = project.get_standard_calling_convention() {
-            for (register, taint) in &self.register_taint {
-                if calling_conv
-                    .parameter_register
-                    .iter()
-                    .any(|param| *param == register.name)
-                    && !taint.is_top()
-                {
-                    return true;
-                }
-            }
-            false
+            self.check_register_list_for_taint(
+                &calling_conv.parameter_register[..],
+                pi_state_option,
+            )
+        } else {
+            // No standard calling convention found. Assume everything may be parameters or referenced by parameters.
+            !self.is_empty()
+        }
+    }
+
+    /// Check whether the return registers may contain tainted values or point to objects containing tainted values.
+    /// Since we don't know the actual return registers,
+    /// we approximate them by all return registers of the standard calling convention of the project.
+    pub fn check_return_values_for_taint(
+        &self,
+        project: &Project,
+        pi_state_option: Option<&PointerInferenceState>,
+    ) -> bool {
+        if let Some(calling_conv) = project.get_standard_calling_convention() {
+            self.check_register_list_for_taint(&calling_conv.return_register[..], pi_state_option)
         } else {
-            // No standard calling convention found. Assume all registers may be parameters.
-            self.register_taint.values().any(|taint| !taint.is_top())
+            // No standard calling convention found. Assume everything may be return values or referenced by return values.
+            !self.is_empty()
         }
     }
 

cwe_checker_rs/src/intermediate_representation/term.rs

@@ -481,10 +481,30 @@ mod tests {
     impl CallingConvention {
         pub fn mock() -> CallingConvention {
             CallingConvention {
-                name: "__stdcall".to_string(),
-                parameter_register: vec!["RDI".into()],
-                callee_saved_register: vec!["RBP".into()],
-                return_register: vec!["RAX".into()],
+                name: "__stdcall".to_string(), // so that the mock is useable as standard calling convention in tests
+                parameter_register: vec!["RDI".to_string()],
+                return_register: vec!["RAX".to_string()],
+                callee_saved_register: vec!["RBP".to_string()],
+            }
+        }
+    }
+
+    impl Arg {
+        pub fn mock_register(name: impl ToString) -> Arg {
+            Arg::Register(Variable::mock(name.to_string(), ByteSize::new(8)))
+        }
+    }
+
+    impl ExternSymbol {
+        pub fn mock() -> ExternSymbol {
+            ExternSymbol {
+                tid: Tid::new("mock_symbol"),
+                addresses: vec!["UNKNOWN".to_string()],
+                name: "mock_symbol".to_string(),
+                calling_convention: Some("__stdcall".to_string()),
+                parameters: vec![Arg::mock_register("RDI")],
+                return_values: vec![Arg::mock_register("RAX")],
+                no_return: false,
             }
         }
     }