Small improvements (#136)

mostly for cleaner code and better documentation

cwe_checker_rs/src/abstract_domain/bitvector.rs

-use super::{AbstractDomain, HasByteSize, HasTop, RegisterDomain};
+use super::{AbstractDomain, HasTop, RegisterDomain, SizedDomain};
 use crate::bil::BitSize;
 use crate::intermediate_representation::*;
 use crate::prelude::*;
@@ -35,7 +35,7 @@ impl HasTop for BitvectorDomain {
     }
 }
 
-impl HasByteSize for BitvectorDomain {
+impl SizedDomain for BitvectorDomain {
     /// Return the bytesize of `self`.
     fn bytesize(&self) -> ByteSize {
         use BitvectorDomain::*;
@@ -121,13 +121,34 @@ impl RegisterDomain for BitvectorDomain {
                         Bitvector::from_u8(0).into()
                     }
                 }
-                IntMult => BitvectorDomain::Value(lhs_bitvec * rhs_bitvec),
-                IntDiv => BitvectorDomain::Value(
+                IntMult => {
+                    // FIXME: Multiplication for bitvectors larger than 8 bytes is not yet implemented in the `apint` crate (version 0.2).
+                    if u64::from(self.bytesize()) > 8 {
+                        BitvectorDomain::Top(self.bytesize())
+                    } else {
+                        BitvectorDomain::Value(lhs_bitvec * rhs_bitvec)
+                    }
+                }
+                IntDiv => {
+                    // FIXME: Division for bitvectors larger than 8 bytes is not yet implemented in the `apint` crate (version 0.2).
+                    if u64::from(self.bytesize()) > 8 {
+                        BitvectorDomain::Top(self.bytesize())
+                    } else {
+                        BitvectorDomain::Value(
                             lhs_bitvec.clone().into_checked_udiv(rhs_bitvec).unwrap(),
-                ),
-                IntSDiv => BitvectorDomain::Value(
+                        )
+                    }
+                }
+                IntSDiv => {
+                    // FIXME: Division for bitvectors larger than 8 bytes is not yet implemented in the `apint` crate (version 0.2).
+                    if u64::from(self.bytesize()) > 8 {
+                        BitvectorDomain::Top(self.bytesize())
+                    } else {
+                        BitvectorDomain::Value(
                             lhs_bitvec.clone().into_checked_sdiv(rhs_bitvec).unwrap(),
-                ),
+                        )
+                    }
+                }
                 IntRem => BitvectorDomain::Value(
                     lhs_bitvec.clone().into_checked_urem(rhs_bitvec).unwrap(),
                 ),

cwe_checker_rs/src/abstract_domain/data.rs

 use super::{
-    AbstractDomain, AbstractIdentifier, HasByteSize, HasTop, PointerDomain, RegisterDomain,
+    AbstractDomain, AbstractIdentifier, HasTop, PointerDomain, RegisterDomain, SizedDomain,
 };
 use crate::intermediate_representation::*;
 use crate::prelude::*;
@@ -62,7 +62,7 @@ impl<T: RegisterDomain> DataDomain<T> {
     }
 }
 
-impl<T: RegisterDomain> HasByteSize for DataDomain<T> {
+impl<T: RegisterDomain> SizedDomain for DataDomain<T> {
     // Return the bitsize of `self`.
     fn bytesize(&self) -> ByteSize {
         use DataDomain::*;

cwe_checker_rs/src/abstract_domain/mem_region.rs

-use super::{AbstractDomain, HasByteSize, HasTop};
+use super::{AbstractDomain, HasTop, SizedDomain};
 use crate::bil::Bitvector;
 use crate::intermediate_representation::ByteSize;
 use apint::{Int, Width};
@@ -22,17 +22,17 @@ use std::sync::Arc;
 /// To allow cheap cloning of a `MemRegion`, the actual data is wrapped inside an `Arc`.
 #[derive(Serialize, Deserialize, Debug, Hash, Clone, PartialEq, Eq, Deref)]
 #[deref(forward)]
-pub struct MemRegion<T: AbstractDomain + HasByteSize + HasTop + std::fmt::Debug>(
+pub struct MemRegion<T: AbstractDomain + SizedDomain + HasTop + std::fmt::Debug>(
     Arc<MemRegionData<T>>,
 );
 
-impl<T: AbstractDomain + HasByteSize + HasTop + std::fmt::Debug> DerefMut for MemRegion<T> {
+impl<T: AbstractDomain + SizedDomain + HasTop + std::fmt::Debug> DerefMut for MemRegion<T> {
     fn deref_mut(&mut self) -> &mut MemRegionData<T> {
         Arc::make_mut(&mut self.0)
     }
 }
 
-impl<T: AbstractDomain + HasByteSize + HasTop + std::fmt::Debug> AbstractDomain for MemRegion<T> {
+impl<T: AbstractDomain + SizedDomain + HasTop + std::fmt::Debug> AbstractDomain for MemRegion<T> {
     /// Short-circuting the `MemRegionData::merge` function if `self==other`,
     /// to prevent unneccessary cloning.
     fn merge(&self, other: &Self) -> Self {
@@ -49,14 +49,14 @@ impl<T: AbstractDomain + HasByteSize + HasTop + std::fmt::Debug> AbstractDomain
     }
 }
 
-impl<T: AbstractDomain + HasByteSize + HasTop + std::fmt::Debug> HasTop for MemRegion<T> {
+impl<T: AbstractDomain + SizedDomain + HasTop + std::fmt::Debug> HasTop for MemRegion<T> {
     /// Return a new, empty memory region with the same address bytesize as `self`, representing the *Top* element of the abstract domain.
     fn top(&self) -> Self {
         Self::new(self.get_address_bytesize())
     }
 }
 
-impl<T: AbstractDomain + HasByteSize + HasTop + std::fmt::Debug> MemRegion<T> {
+impl<T: AbstractDomain + SizedDomain + HasTop + std::fmt::Debug> MemRegion<T> {
     // Create a new, empty memory region.
     pub fn new(address_bytesize: ByteSize) -> Self {
         MemRegion(Arc::new(MemRegionData::new(address_bytesize)))
@@ -65,12 +65,12 @@ impl<T: AbstractDomain + HasByteSize + HasTop + std::fmt::Debug> MemRegion<T> {
 
 /// The internal data of a memory region. See the description of `MemRegion` for more.
 #[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Hash, Clone)]
-pub struct MemRegionData<T: AbstractDomain + HasByteSize + HasTop + std::fmt::Debug> {
+pub struct MemRegionData<T: AbstractDomain + SizedDomain + HasTop + std::fmt::Debug> {
     address_bytesize: ByteSize,
     values: BTreeMap<i64, T>,
 }
 
-impl<T: AbstractDomain + HasByteSize + HasTop + std::fmt::Debug> MemRegionData<T> {
+impl<T: AbstractDomain + SizedDomain + HasTop + std::fmt::Debug> MemRegionData<T> {
     /// create a new, empty MemRegion
     pub fn new(address_bytesize: ByteSize) -> MemRegionData<T> {
         MemRegionData {
@@ -254,7 +254,7 @@ mod tests {
         }
     }
 
-    impl HasByteSize for MockDomain {
+    impl SizedDomain for MockDomain {
         fn bytesize(&self) -> ByteSize {
             self.1
         }

cwe_checker_rs/src/abstract_domain/mod.rs

@@ -34,7 +34,7 @@ pub trait AbstractDomain: Sized + Eq + Clone {
 /// For abstract domains, the bytesize is a parameter of the domain itself,
 /// i.e. you cannot merge values of different bytesizes,
 /// since they lie in different posets (one for each bytesize).
-pub trait HasByteSize {
+pub trait SizedDomain {
     /// Return the size of the represented value in bytes.
     fn bytesize(&self) -> ByteSize;
 
@@ -58,7 +58,7 @@ pub trait HasTop {
 /// The domain implements all general operations used to manipulate register values.
 /// The domain is parametrized by its bytesize (which represents the size of the register).
 /// It has a *Top* element, which is only characterized by its bytesize.
-pub trait RegisterDomain: AbstractDomain + HasByteSize + HasTop {
+pub trait RegisterDomain: AbstractDomain + SizedDomain + HasTop {
     /// Compute the (abstract) result of a binary operation
     fn bin_op(&self, op: BinOpType, rhs: &Self) -> Self;
 

cwe_checker_rs/src/abstract_domain/pointer.rs

-use super::{AbstractDomain, AbstractIdentifier, HasByteSize, RegisterDomain};
+use super::{AbstractDomain, AbstractIdentifier, RegisterDomain, SizedDomain};
 use crate::intermediate_representation::{BinOpType, ByteSize};
 use crate::prelude::*;
 use std::collections::BTreeMap;
@@ -38,7 +38,7 @@ impl<T: RegisterDomain> AbstractDomain for PointerDomain<T> {
     }
 }
 
-impl<T: RegisterDomain> HasByteSize for PointerDomain<T> {
+impl<T: RegisterDomain> SizedDomain for PointerDomain<T> {
     /// Return the bitsize of the pointer.
     /// Should always equal the pointer size of the CPU architecture.
     fn bytesize(&self) -> ByteSize {

cwe_checker_rs/src/analysis/pointer_inference/context/tests.rs

@@ -33,11 +33,7 @@ fn register(name: &str) -> Variable {
 }
 
 fn reg_add_term(name: &str, value: i64, tid_name: &str) -> Term<Def> {
-    let add_expr = Expression::BinOp {
-        op: BinOpType::IntAdd,
-        lhs: Box::new(Expression::Var(register(name))),
-        rhs: Box::new(Expression::Const(Bitvector::from_i64(value))),
-    };
+    let add_expr = Expression::Var(register(name)).plus_const(value);
     Term {
         tid: Tid::new(format!("{}", tid_name)),
         term: Def::Assign {
@@ -118,11 +114,7 @@ fn context_problem_implementation() {
         tid: Tid::new("def"),
         term: Def::Assign {
             var: register("RSP"),
-            value: BinOp {
-                op: BinOpType::IntAdd,
-                lhs: Box::new(Var(register("RSP"))),
-                rhs: Box::new(Const(Bitvector::from_i64(-16))),
-            },
+            value: Var(register("RSP")).plus_const(-16),
         },
     };
     let store_term = Term {

cwe_checker_rs/src/analysis/pointer_inference/object_list.rs

@@ -13,7 +13,12 @@ use std::collections::{BTreeMap, BTreeSet};
 /// to indicate that it may represent more than one actual memory object.
 #[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)]
 pub struct AbstractObjectList {
-    /// The abstract objects
+    /// The abstract objects.
+    ///
+    /// Each abstract object comes with an offset given as a [`BitvectorDomain`].
+    /// This offset determines where the zero offset corresponding to the abstract identifier inside the object is.
+    /// Note that this offset may be a `Top` element
+    /// if the exact offset corresponding to the identifier is unknown.
     objects: BTreeMap<AbstractIdentifier, (AbstractObject, BitvectorDomain)>,
 }
 

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

@@ -250,15 +250,7 @@ impl State {
         match parameter {
             Arg::Register(var) => self.eval(&Expression::Var(var.clone())),
             Arg::Stack { offset, size } => self.load_value(
-                &Expression::BinOp {
-                    op: BinOpType::IntAdd,
-                    lhs: Box::new(Expression::Var(stack_pointer.clone())),
-                    rhs: Box::new(Expression::Const(
-                        Bitvector::from_i64(*offset)
-                            .into_truncate(apint::BitWidth::from(stack_pointer.size))
-                            .unwrap(),
-                    )),
-                },
+                &Expression::Var(stack_pointer.clone()).plus_const(*offset),
                 *size,
                 global_memory,
             ),

cwe_checker_rs/src/analysis/pointer_inference/state/mod.rs

@@ -98,15 +98,8 @@ impl State {
                 Arg::Register(_) => (),
                 Arg::Stack { offset, size } => {
                     let data_top = Data::new_top(*size);
-                    let location_expression = Expression::BinOp {
-                        lhs: Box::new(Expression::Var(stack_pointer_register.clone())),
-                        op: BinOpType::IntAdd,
-                        rhs: Box::new(Expression::Const(
-                            Bitvector::from_i64(*offset)
-                                .into_truncate(apint::BitWidth::from(stack_pointer_register.size))
-                                .unwrap(),
-                        )),
-                    };
+                    let location_expression =
+                        Expression::Var(stack_pointer_register.clone()).plus_const(*offset);
                     if let Err(err) =
                         self.write_to_address(&location_expression, &data_top, global_memory)
                     {

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

@@ -21,11 +21,7 @@ fn register(name: &str) -> Variable {
 }
 
 fn reg_add(name: &str, value: i64) -> Expression {
-    Expression::BinOp {
-        op: BinOpType::IntAdd,
-        lhs: Box::new(Expression::Var(register(name))),
-        rhs: Box::new(Expression::Const(Bitvector::from_i64(value))),
-    }
+    Expression::Var(register(name)).plus_const(value)
 }
 
 fn reg_sub(name: &str, value: i64) -> Expression {

cwe_checker_rs/src/checkers/cwe_476/context.rs

@@ -8,7 +8,6 @@ use crate::analysis::interprocedural_fixpoint_generic::NodeValue;
 use crate::analysis::pointer_inference::PointerInference as PointerInferenceComputation;
 use crate::analysis::pointer_inference::State as PointerInferenceState;
 use crate::intermediate_representation::*;
-use crate::prelude::*;
 use crate::utils::binary::RuntimeMemoryImage;
 use crate::utils::log::CweWarning;
 use petgraph::graph::NodeIndex;
@@ -205,19 +204,10 @@ impl<'a> Context<'a> {
                         }
                     }
                     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 let Ok(stack_address) = pi_state.eval(
+                            &Expression::Var(self.project.stack_pointer_register.clone())
+                                .plus_const(*offset),
+                        ) {
                             if state
                                 .load_taint_from_memory(&stack_address, *size)
                                 .is_tainted()

cwe_checker_rs/src/checkers/cwe_476/state.rs

 use crate::abstract_domain::{
-    AbstractDomain, AbstractIdentifier, BitvectorDomain, HasByteSize, MemRegion, RegisterDomain,
+    AbstractDomain, AbstractIdentifier, BitvectorDomain, MemRegion, RegisterDomain, SizedDomain,
 };
 use crate::analysis::pointer_inference::Data;
 use crate::analysis::pointer_inference::State as PointerInferenceState;
@@ -99,17 +99,8 @@ impl State {
                 }
                 Arg::Stack { offset, size } => {
                     if let Some(pi_state) = pi_state {
-                        let address_exp = Expression::BinOp {
-                            op: BinOpType::IntAdd,
-                            lhs: Box::new(Expression::Var(stack_pointer_register.clone())),
-                            rhs: Box::new(Expression::Const(
-                                Bitvector::from_i64(*offset)
-                                    .into_truncate(apint::BitWidth::from(
-                                        stack_pointer_register.size,
-                                    ))
-                                    .unwrap(),
-                            )),
-                        };
+                        let address_exp =
+                            Expression::Var(stack_pointer_register.clone()).plus_const(*offset);
                         if let Ok(address) = pi_state.eval(&address_exp) {
                             state.save_taint_to_memory(&address, Taint::Tainted(*size));
                         }
@@ -494,11 +485,7 @@ mod tests {
     fn eval_expression() {
         let (state, _pi_state) = State::mock_with_pi_state();
 
-        let expr = Expression::BinOp {
-            lhs: Box::new(Expression::Var(register("RAX"))),
-            op: BinOpType::IntAdd,
-            rhs: Box::new(Expression::Var(register("RBX"))),
-        };
+        let expr = Expression::Var(register("RAX")).plus(Expression::Var(register("RBX")));
         assert!(state.eval(&expr).is_tainted());
 
         let expr = Expression::UnOp {

cwe_checker_rs/src/checkers/cwe_476/taint.rs

-use crate::abstract_domain::{AbstractDomain, HasByteSize, HasTop, RegisterDomain};
+use crate::abstract_domain::{AbstractDomain, HasTop, RegisterDomain, SizedDomain};
 use crate::intermediate_representation::*;
 use crate::prelude::*;
 use std::fmt::Display;
@@ -45,7 +45,7 @@ impl AbstractDomain for Taint {
     }
 }
 
-impl HasByteSize for Taint {
+impl SizedDomain for Taint {
     /// The size in bytes of the `Taint` value.
     fn bytesize(&self) -> ByteSize {
         match self {

cwe_checker_rs/src/checkers/cwe_782.rs

@@ -49,8 +49,7 @@ pub fn generate_cwe_warning(calls: &[(&str, &Tid, &str)]) -> Vec<CweWarning> {
     for (sub_name, jmp_tid, _) in calls.iter() {
         let address: &String = &jmp_tid.address;
         let description = format!(
-            "(Exposed IOCTL with Insufficient Access Control) Program uses ioctl at {} ({}). 
-        Be sure to double check the program and the corresponding driver.",
+            "(Exposed IOCTL with Insufficient Access Control) Program uses ioctl at {} ({}). Be sure to double check the program and the corresponding driver.",
             sub_name, address
         );
         let cwe_warning = CweWarning::new(

cwe_checker_rs/src/intermediate_representation/expression.rs

@@ -4,6 +4,8 @@ use super::Variable;
 use super::{ByteSize, Def};
 use crate::{pcode::RegisterProperties, prelude::*};
 
+mod builder;
+
 /// An expression is a calculation rule
 /// on how to compute a certain value given some variables (register values) as input.
 ///

cwe_checker_rs/src/intermediate_representation/expression/builder.rs

+use super::{BinOpType, Expression};
+use crate::prelude::*;
+
+/// ## Helper functions for building expressions
+impl Expression {
+    /// Shortcut for creating an `IntAdd`-expression
+    pub fn plus(self, rhs: Expression) -> Expression {
+        Expression::BinOp {
+            lhs: Box::new(self),
+            op: BinOpType::IntAdd,
+            rhs: Box::new(rhs),
+        }
+    }
+
+    /// Construct an expression that adds a constant value to the given expression.
+    ///
+    /// The bytesize of the value is automatically adjusted to the bytesize of the given expression.
+    pub fn plus_const(self, value: i64) -> Expression {
+        let bytesize = self.bytesize();
+        let mut value = Bitvector::from_i64(value);
+        match u64::from(bytesize) {
+            size if size > 8 => value.sign_extend(bytesize).unwrap(),
+            size if size < 8 => value.truncate(bytesize).unwrap(),
+            _ => (),
+        }
+        self.plus(Expression::Const(value))
+    }
+}

cwe_checker_rs/src/utils/binary.rs

 //! Utility structs and functions which directly parse the binary file.
 
 use crate::abstract_domain::BitvectorDomain;
-use crate::abstract_domain::HasByteSize;
 use crate::abstract_domain::RegisterDomain;
+use crate::abstract_domain::SizedDomain;
 use crate::intermediate_representation::BinOpType;
 use crate::prelude::*;
 use goblin::elf;

cwe_checker_rs/src/utils/log.rs

@@ -233,7 +233,7 @@ impl LogThread {
     ///
     /// The parameter is the function containing the actual log collection logic.
     /// I.e. the function should receive messages through the given receiver until the channel disconnects
-    /// or until it receives a [`LogThread::Terminate`] message.
+    /// or until it receives a [`LogThreadMsg::Terminate`] message.
     /// After that it should return the logs collected up to that point.
     pub fn spawn<F>(collector_func: F) -> LogThread
     where