Function sig param sanitization (#389)

src/cwe_checker_lib/src/analysis/function_signature/mod.rs

@@ -41,6 +41,7 @@ use std::collections::HashMap;
 mod context;
 use context::*;
 mod state;
+use itertools::Itertools;
 use state::State;
 mod access_pattern;
 pub use access_pattern::AccessPattern;
@@ -137,7 +138,7 @@ fn extract_fn_signatures_from_fixpoint<'a>(
 /// Compute the function signatures for all functions in the project.
 ///
 /// Returns a map from the function TIDs to their signatures,
-/// and a list of log messages recorded during the computation of the signatures.
+/// and a list of log and debug messages recorded during the computation of the signatures.
 ///
 /// For more information on the used algorithm see the module-level documentation.
 pub fn compute_function_signatures<'a>(
@@ -150,12 +151,20 @@ pub fn compute_function_signatures<'a>(
     // Sanitize the parameters
     let mut logs = Vec::new();
     for (fn_tid, fn_sig) in fn_sig_map.iter_mut() {
-        if fn_sig.sanitize(project).is_err() {
+        let (info_log, debug_log) = fn_sig.sanitize(project);
+        for log in info_log {
             logs.push(
-                LogMessage::new_error("Function parameters are not properly sanitized")
+                LogMessage::new_info(log)
                     .location(fn_tid.clone())
                     .source("Function Signature Analysis"),
-            );
+            )
+        }
+        for log in debug_log {
+            logs.push(
+                LogMessage::new_debug(log)
+                    .location(fn_tid.clone())
+                    .source("Function Signature Analysis"),
+            )
         }
     }
     // Propagate globals in bottom-up direction in the call graph
@@ -230,10 +239,11 @@ impl FunctionSignature {
     /// Sanitize the function signature:
     /// * Remove the return address from the list of stack parameters for x86-based architectures.
     /// * Check for unaligned stack parameters or stack parameters that are not pointer-sized
-    ///   and return an error message if one is found.
+    ///   and return an info message if one is found.
     ///   This may indicate an error in the analysis
     ///   as no proper sanitation pass is implemented for such cases yet.
-    fn sanitize(&mut self, project: &Project) -> Result<(), Error> {
+    /// * Merge intersecting stack parameters
+    fn sanitize(&mut self, project: &Project) -> (Vec<String>, Vec<String>) {
         match project.cpu_architecture.as_str() {
             "x86" | "x86_32" | "x86_64" => {
                 let return_addr_expr = Expression::Var(project.stack_pointer_register.clone());
@@ -246,63 +256,160 @@ impl FunctionSignature {
             }
             _ => (),
         }
-        self.check_for_unaligned_stack_params(&project.stack_pointer_register)
+        let debug_messages = self.merge_intersecting_stack_parameters();
+        let info_messages = self.check_for_unaligned_stack_params(&project.stack_pointer_register);
+
+        (info_messages, debug_messages)
     }
 
-    /// Return an error if an unaligned stack parameter
+    /// Return a log message for every unaligned stack parameter
     /// or a stack parameter of different size than the generic pointer size is found.
-    fn check_for_unaligned_stack_params(&self, stack_register: &Variable) -> Result<(), Error> {
+    fn check_for_unaligned_stack_params(&self, stack_register: &Variable) -> Vec<String> {
+        let mut log_messages: Vec<String> = vec![];
         for arg in self.parameters.keys() {
             if let Arg::Stack { size, .. } = arg {
                 if *size != stack_register.size {
-                    return Err(anyhow!("Unexpected stack parameter size"));
+                    log_messages.push("Unexpected stack parameter size".into());
                 }
                 if let Ok(offset) = arg.eval_stack_offset() {
-                    if offset.try_to_u64()? % u64::from(stack_register.size) != 0 {
-                        return Err(anyhow!("Unexpected stack parameter alignment"));
+                    if offset.try_to_u64().unwrap_or(0) % u64::from(stack_register.size) != 0 {
+                        log_messages.push("Unexpected stack parameter alignment".into());
                     }
                 }
             }
         }
-        Ok(())
+        log_messages
     }
-}
+    /// Merges two intersecting stack parameters by joining them into one stack parameter.
+    ///
+    /// Two [Arg](crate::intermediate_representation::Arg) are merged if *all* of the following applies:
+    /// * parameters return `Ok` on `Arg::eval_stack_offset()`
+    /// * parameters intersect
+    fn merge_intersecting_stack_parameters(&mut self) -> Vec<String> {
+        let mut stack_parms = self
+            .parameters
+            .clone()
+            .into_iter()
+            .filter(|x| x.0.eval_stack_offset().is_ok())
+            .sorted_by(|a, b| {
+                match a
+                    .0
+                    .eval_stack_offset()
+                    .unwrap()
+                    .checked_sgt(&b.0.eval_stack_offset().unwrap())
+                    .unwrap()
+                {
+                    true => std::cmp::Ordering::Greater,
+                    false => std::cmp::Ordering::Less,
+                }
+            })
+            .collect_vec();
+        let mut logs = vec![];
 
-impl Default for FunctionSignature {
-    fn default() -> Self {
-        Self::new()
+        if !stack_parms.is_empty() {
+            let mut i = 0;
+            while i < stack_parms.len() - 1 {
+                if let Ok((merged_arg, log)) =
+                    get_bounds_intersecting_stack_arg(&stack_parms[i].0, &stack_parms[i + 1].0)
+                {
+                    self.parameters.remove(&stack_parms[i].0);
+                    self.parameters.remove(&stack_parms[i + 1].0);
+                    self.parameters.insert(
+                        merged_arg.clone(),
+                        stack_parms[i].1.merge(&stack_parms[i + 1].1),
+                    );
+
+                    stack_parms.insert(
+                        i,
+                        (merged_arg, stack_parms[i].1.merge(&stack_parms[i + 1].1)),
+                    );
+                    stack_parms.remove(i + 1);
+                    stack_parms.remove(i + 1);
+
+                    logs.extend(log);
+                } else {
+                    i += 1;
+                }
+            }
+        }
+
+        logs
     }
 }
 
-#[cfg(test)]
-pub mod tests {
-    use super::*;
-    use crate::variable;
+/// Merges two stack parameters and returns the merged [Arg](crate::intermediate_representation::Arg).
+/// Also returns a message, if one argument is not a subset of the other one.
+///
+/// Assumes the provided `Arg` are ordered by equal or increasing stack offset.
+///
+/// Returns `Err` if `first_arg` or `second_arg`:
+/// * are not `Arg::Stack` types
+/// * return `Err` on `Arg::eval_stack_offset()`
+/// * do not intersect
+fn get_bounds_intersecting_stack_arg(
+    first_arg: &Arg,
+    second_arg: &Arg,
+) -> Result<(Arg, Vec<String>), Error> {
+    if let (
+        Arg::Stack {
+            data_type: _,
+            size: first_size,
+            address: first_address,
+        },
+        Arg::Stack {
+            data_type: _,
+            size: second_size,
+            ..
+        },
+    ) = (first_arg, second_arg)
+    {
+        let first_arg_offset = first_arg.eval_stack_offset()?.try_to_u64()?;
+        let first_arg_size = u64::from(*first_size);
+        let second_arg_offset = second_arg.eval_stack_offset()?.try_to_u64()?;
+        let second_arg_size = u64::from(*second_size);
+
+        let mut logs = vec![];
+        let first_arg_upper_bound = first_arg_offset + first_arg_size;
 
-    impl FunctionSignature {
-        /// Create a mock x64 function signature with 2 parameters, one of which is accessed mutably,
-        /// one mutably accessed global variable at address 0x2000
-        /// and one immutably accessed global variable at address 0x3000.
-        pub fn mock_x64() -> FunctionSignature {
-            let mut write_access_pattern = AccessPattern::new();
-            write_access_pattern.set_unknown_access_flags();
-            let parameters = HashMap::from_iter([
-                (
-                    Arg::from_var(variable!("RDI:8"), None),
-                    AccessPattern::new(),
-                ),
-                (
-                    Arg::from_var(variable!("RSI:8"), None),
-                    write_access_pattern,
-                ),
-            ]);
-            FunctionSignature {
-                parameters,
-                global_parameters: HashMap::from([
-                    (0x2000, AccessPattern::new_unknown_access()),
-                    (0x3000, AccessPattern::new().with_dereference_flag()),
-                ]),
+        // Check if they intersect
+        if first_arg_upper_bound > second_arg_offset {
+            let second_arg_upper_bound = second_arg_offset + second_arg_size;
+
+            // Check if subset
+            if second_arg_upper_bound <= first_arg_upper_bound
+                && second_arg_offset >= first_arg_offset
+            {
+                // second arg is a subset, we just keep first_arg
+                return Ok((first_arg.clone(), logs));
+            }
+            if first_arg_upper_bound <= second_arg_upper_bound
+                && first_arg_offset >= second_arg_offset
+            {
+                // first arg is a subset, we just keep second_arg
+                return Ok((second_arg.clone(), logs));
             }
+            logs.push(
+                "Merged a stack parameter, that intersect another but is no subset".to_string(),
+            );
+
+            let merged_arg = Arg::Stack {
+                address: first_address.clone(),
+                size: (second_arg_upper_bound - first_arg_offset).into(),
+                data_type: None,
+            };
+            return Ok((merged_arg, logs));
+        } else {
+            return Err(anyhow!("Args do not intersect"));
         }
     }
+    Err(anyhow!("Args are no stack arguments"))
 }
+
+impl Default for FunctionSignature {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
+#[cfg(test)]
+mod tests;

src/cwe_checker_lib/src/analysis/function_signature/tests.rs

+use super::*;
+use crate::{expr, variable};
+impl FunctionSignature {
+    /// Create a mock x64 function signature with 2 parameters, one of which is accessed mutably,
+    /// one mutably accessed global variable at address 0x2000
+    /// and one immutably accessed global variable at address 0x3000.
+    pub fn mock_x64() -> FunctionSignature {
+        let mut write_access_pattern = AccessPattern::new();
+        write_access_pattern.set_unknown_access_flags();
+        let parameters = HashMap::from_iter([
+            (
+                Arg::from_var(variable!("RDI:8"), None),
+                AccessPattern::new(),
+            ),
+            (
+                Arg::from_var(variable!("RSI:8"), None),
+                write_access_pattern,
+            ),
+        ]);
+        FunctionSignature {
+            parameters,
+            global_parameters: HashMap::from([
+                (0x2000, AccessPattern::new_unknown_access()),
+                (0x3000, AccessPattern::new().with_dereference_flag()),
+            ]),
+        }
+    }
+}
+
+fn mock_stack_arg(address: Expression, size: u64) -> Arg {
+    Arg::Stack {
+        address,
+        size: size.into(),
+        data_type: None,
+    }
+}
+
+#[test]
+fn test_two_parameter_overlapping_merging() {
+    let proj = Project::mock_x64();
+    let mut func_sig = FunctionSignature::mock_x64();
+    let stack_parm_1 = mock_stack_arg(expr!("RSP:8 + 0x1000:8"), 8);
+    let stack_parm_2 = mock_stack_arg(expr!("RSP:8 + 0x1004:8"), 8);
+
+    func_sig
+        .parameters
+        .insert(stack_parm_1, AccessPattern::new());
+    func_sig
+        .parameters
+        .insert(stack_parm_2, AccessPattern::new());
+
+    assert_eq!(
+        func_sig.sanitize(&proj),
+        (
+            vec!["Unexpected stack parameter size".to_string()],
+            vec!["Merged a stack parameter, that intersect another but is no subset".to_string()]
+        )
+    );
+    let mut expected_function_sig = FunctionSignature::mock_x64();
+    let expected_stack_arg = mock_stack_arg(expr!("RSP:8 + 0x1000:8"), 12);
+
+    expected_function_sig
+        .parameters
+        .insert(expected_stack_arg, AccessPattern::new());
+    assert_eq!(func_sig, expected_function_sig);
+}
+
+#[test]
+fn test_merging_multiple_parameters() {
+    let proj = Project::mock_x64();
+    let mut func_sig = FunctionSignature::mock_x64();
+    let stack_parm_1 = mock_stack_arg(expr!("RSP:8 + 0x1000:8"), 8);
+    let stack_parm_2 = mock_stack_arg(expr!("RSP:8 + 0x1000:8"), 1);
+    let stack_parm_3 = mock_stack_arg(expr!("RSP:8 + 0x1007:8"), 1);
+    let stack_parm_4 = mock_stack_arg(expr!("RSP:8 + 0x1008:8"), 8);
+
+    func_sig.parameters.extend([
+        (stack_parm_1.clone(), AccessPattern::new()),
+        (stack_parm_2, AccessPattern::new()),
+        (stack_parm_3, AccessPattern::new()),
+        (stack_parm_4.clone(), AccessPattern::new()),
+    ]);
+    assert_eq!((vec![], vec![]), func_sig.sanitize(&proj));
+
+    let mut expected_function_sig = FunctionSignature::mock_x64();
+    expected_function_sig.parameters.extend([
+        (stack_parm_1, AccessPattern::new()),
+        (stack_parm_4, AccessPattern::new()),
+    ]);
+    assert_eq!(func_sig, expected_function_sig);
+}
+#[test]
+fn test_log_messages() {
+    let proj = Project::mock_x64();
+    let mut func_sig = FunctionSignature::mock_x64();
+    let stack_parm_1 = mock_stack_arg(expr!("RSP:8 + 0x1001:8"), 8);
+    let stack_parm_2 = mock_stack_arg(expr!("RSP:8 + 0x1007:8"), 4);
+
+    func_sig.parameters.extend([
+        (stack_parm_1.clone(), AccessPattern::new()),
+        (stack_parm_2, AccessPattern::new()),
+    ]);
+
+    let logs = func_sig.sanitize(&proj);
+    assert_eq!(
+        (
+            vec![
+                "Unexpected stack parameter size".to_string(),
+                "Unexpected stack parameter alignment".to_string()
+            ],
+            vec!["Merged a stack parameter, that intersect another but is no subset".to_string()]
+        ),
+        logs
+    );
+}