Correctly classify some buffer overflows as NULL dereferences (#226)

src/cwe_checker_lib/src/abstract_domain/data.rs

@@ -206,8 +206,8 @@ impl<T: RegisterDomain + Display> DataDomain<T> {
         }
         if self.contains_top_values {
             values.push(serde_json::Value::String(format!(
-                "Top:{}",
-                self.bytesize()
+                "Top:i{}",
+                self.bytesize().as_bit_length()
             )));
         }
         match values.len() {

src/cwe_checker_lib/src/analysis/pointer_inference/context/mod.rs

@@ -516,6 +516,23 @@ impl<'a> Context<'a> {
         }
         ValueDomain::new_top(self.project.stack_pointer_register.size)
     }
+
+    /// Report a NULL dereference CWE at the address of the given TID.
+    fn report_null_deref(&self, tid: &Tid) {
+        let warning = CweWarning {
+            name: "CWE476".to_string(),
+            version: VERSION.to_string(),
+            addresses: vec![tid.address.clone()],
+            tids: vec![format!("{}", tid)],
+            symbols: Vec::new(),
+            other: Vec::new(),
+            description: format!(
+                "(NULL Pointer Dereference) Memory access at {} may result in a NULL dereference",
+                tid.address
+            ),
+        };
+        let _ = self.log_collector.send(LogThreadMsg::Cwe(warning));
+    }
 }
 
 #[cfg(test)]

src/cwe_checker_lib/src/analysis/pointer_inference/context/trait_impls.rs

@@ -32,8 +32,17 @@ impl<'a> crate::analysis::forward_interprocedural_fixpoint::Context<'a> for Cont
             };
             let _ = self.log_collector.send(LogThreadMsg::Cwe(warning));
         }
+        // check for null dereferences
+        match new_state.check_def_for_null_dereferences(def) {
+            Err(_) => {
+                self.report_null_deref(&def.tid);
+                return None;
+            }
+            Ok(true) => self.report_null_deref(&def.tid),
+            Ok(false) => (), // no null dereference detected
+        }
         // check for out-of-bounds memory access
-        if state.contains_out_of_bounds_mem_access(&def.term, self.runtime_memory_image) {
+        if new_state.contains_out_of_bounds_mem_access(&def.term, self.runtime_memory_image) {
             let (warning_name, warning_description) = match def.term {
                 Def::Load { .. } => (
                     "CWE125",

src/cwe_checker_lib/src/analysis/pointer_inference/mod.rs

@@ -15,6 +15,7 @@
 //! - [CWE-125](https://cwe.mitre.org/data/definitions/125.html) Buffer Overflow: Out-of-bounds Read
 //! - [CWE-415](https://cwe.mitre.org/data/definitions/415.html): Double Free
 //! - [CWE-416](https://cwe.mitre.org/data/definitions/416.html): Use After Free
+//! - [CWE-476](https://cwe.mitre.org/data/definitions/476.html): NULL Pointer Dereference
 //! - [CWE-787](https://cwe.mitre.org/data/definitions/787.html): Buffer Overflow: Out-of-bounds Write
 //!
 //! The analysis operates on a best-effort basis.

src/cwe_checker_lib/src/analysis/pointer_inference/state/access_handling.rs

@@ -337,4 +337,51 @@ impl State {
         }
         None
     }
+
+    /// Check whether the given `def` could result in a memory access through a NULL pointer.
+    ///
+    /// If no NULL pointer dereference is detected then `Ok(false)` is returned.
+    /// If a NULL pointer dereference is detected,
+    /// try to specialize the state so that `address_expr` cannot result in a NULL pointer anymore.
+    /// If that succeeds, `Ok(true)` is returned.
+    /// If that would result in an unsatisfiable state, an error is returned.
+    pub fn check_def_for_null_dereferences(&mut self, def: &Term<Def>) -> Result<bool, Error> {
+        let address_expr = match &def.term {
+            Def::Load { address, .. } | Def::Store { address, .. } => address,
+            Def::Assign { .. } => return Ok(false),
+        };
+        let mut address_val = self.eval(address_expr);
+        if let Some((start_index, end_index)) = address_val
+            .get_absolute_value()
+            .map(|val| val.try_to_offset_interval().ok())
+            .flatten()
+        {
+            if (start_index > -1024 && start_index < 1024)
+                || (end_index > -1024 && end_index < 1024)
+            {
+                // Interval starts or ends with a null pointer
+                let absolute_val = address_val.get_absolute_value().unwrap().clone();
+                let new_absolute_val = if start_index > -1024 && start_index < 1024 {
+                    absolute_val
+                        .add_signed_greater_equal_bound(
+                            &Bitvector::from_i16(1024).into_resize_signed(address_val.bytesize()),
+                        )
+                        .ok()
+                } else {
+                    absolute_val
+                        .add_signed_less_equal_bound(
+                            &Bitvector::from_i16(-1024).into_resize_signed(address_val.bytesize()),
+                        )
+                        .ok()
+                };
+                address_val.set_absolute_value(new_absolute_val);
+                if address_val.is_empty() {
+                    return Err(anyhow!("Unsatisfiable state"));
+                }
+                self.specialize_by_expression_result(address_expr, address_val)?;
+                return Ok(true);
+            }
+        }
+        Ok(false)
+    }
 }

src/cwe_checker_lib/src/analysis/pointer_inference/state/tests.rs

@@ -1085,3 +1085,35 @@ fn specialize_pointer_comparison() {
         .is_ok());
     assert_eq!(state.get_register(&register("RAX")), specialized_pointer);
 }
+
+#[test]
+fn test_check_def_for_null_dereferences() {
+    let mut state = State::new(&register("RSP"), Tid::new("func_tid"));
+    let var_rax = Variable::mock("RAX", 8);
+    let def = Def::load(
+        "load_def",
+        Variable::mock("RBX", 8),
+        Expression::Var(var_rax.clone()),
+    );
+    state.set_register(&var_rax, Bitvector::from_i64(0).into());
+    assert!(state.check_def_for_null_dereferences(&def).is_err());
+
+    state.set_register(&var_rax, Bitvector::from_i64(12345).into());
+    assert_eq!(
+        state.check_def_for_null_dereferences(&def).ok(),
+        Some(false)
+    );
+
+    state.set_register(&var_rax, IntervalDomain::mock(-2000, 5).into());
+    assert_eq!(state.check_def_for_null_dereferences(&def).ok(), Some(true));
+    assert_eq!(
+        state.get_register(&var_rax),
+        IntervalDomain::mock(-2000, -1024).into()
+    );
+
+    let mut address = state.get_register(&register("RSP"));
+    address.set_contains_top_flag();
+    address.set_absolute_value(Some(IntervalDomain::mock(0, 0xffff)));
+    state.set_register(&var_rax, address);
+    assert_eq!(state.check_def_for_null_dereferences(&def).ok(), Some(true));
+}

test/src/lib.rs

@@ -184,7 +184,7 @@ mod tests {
         let num_cwes = String::from_utf8(output.stdout)
             .unwrap()
             .lines()
-            .filter(|line| line.starts_with("[CWE125]"))
+            .filter(|line| line.starts_with("[CWE476]"))
             .count();
         // We check the number of found CWEs only approximately
         // so that this check does not fail on minor result changes.