Merge pull request #451 from vobst/cwe_252

Check for CWE 252

CHANGES.md

@@ -1,6 +1,9 @@
 0.9-dev
 ===
 
+- Improve taint analysis abstraction to simplify interprocedural bottom-up
+ analysis of memory taint (PR #451)
+- Added check for CWE-252: Unchecked Return Value (PR #451)
 - Implemented experimental support for Linux Kernel Modules (PR #441)
 - Support LZCOUNT P-Code operation (PR #444)
 
@@ -11,7 +14,7 @@
 - Enforce expression complexity limit to reduce RAM usage (PR #428)
 - Implement tracking of nested parameters to improve runtime and analysis quality (PR #432)
 - Reduce amount of false positives generated by CWE-416 check (PR #433)
-- Added check for CWE-337: Predictable Seed in Pseudo-Random Number Generator (PR #439) 
+- Added check for CWE-337: Predictable Seed in Pseudo-Random Number Generator (PR #439)
 
 0.7 (2023-06)
 ====

src/caller/src/main.rs

@@ -142,7 +142,7 @@ fn run_with_ghidra(args: &CmdlineArgs) -> Result<(), Error> {
 
  let modules_depending_on_string_abstraction = BTreeSet::from_iter(["CWE78"]);
  let modules_depending_on_pointer_inference = BTreeSet::from_iter([
- "CWE119", "CWE134", "CWE190", "CWE337", "CWE416", "CWE476", "CWE789", "Memory",
+ "CWE119", "CWE134", "CWE190", "CWE252", "CWE337", "CWE416", "CWE476", "CWE789", "Memory",
  ]);
 
  let string_abstraction_needed = modules

src/config.json

@@ -73,6 +73,149 @@
  "CWE248": {
  "symbols": []
  },
+ "CWE252": {
+ "_comment": "External symbols whose return value must be used.",
+ "_comment": "Every exported function that is annotated with warn_unused_result in glibc.",
+ "_comment": "Strict mode activates the check for symbols that even very mature projects ignore.",
+ "strict_mode": false,
+ "strict_symbols": [
+ "fileno",
+ "setpgid"
+ ],
+ "symbols": [
+ "__asprintf",
+ "__getdelim",
+ "__getrandom",
+ "__isoc23_fscanf",
+ "__isoc23_scanf",
+ "__isoc23_sscanf",
+ "__isoc23_vfscanf",
+ "__isoc23_vscanf",
+ "__isoc99_fscanf",
+ "__isoc99_scanf",
+ "__isoc99_sscanf",
+ "__isoc99_vfscanf",
+ "__isoc99_vscanf",
+ "access",
+ "arc4random_uniform",
+ "asprintf",
+ "atof",
+ "atoi",
+ "atol",
+ "atoll",
+ "brk",
+ "chdir",
+ "chown",
+ "chroot",
+ "confstr",
+ "daemon",
+ "dup",
+ "faccessat",
+ "fchdir",
+ "fchmodat",
+ "fchown",
+ "fchownat",
+ "feof",
+ "feof_unlocked",
+ "ferror_unlocked",
+ "fgets",
+ "fgets_unlocked",
+ "fgetws",
+ "fgetws_unlocked",
+ "fileno_unlocked",
+ "fputc",
+ "fread",
+ "fread_unlocked",
+ "fscanf",
+ "ftell",
+ "ftello",
+ "ftello64",
+ "ftruncate",
+ "ftruncate64",
+ "ftrylockfile",
+ "getdelim",
+ "getdomainname",
+ "getentropy",
+ "getgroups",
+ "getline",
+ "getrandom",
+ "gets",
+ "getsubopt",
+ "lchown",
+ "link",
+ "linkat",
+ "lockf",
+ "lockf64",
+ "mkostemp",
+ "mkostemp64",
+ "mkostemps",
+ "mkostemps64",
+ "mkstemp",
+ "mkstemps",
+ "mkstemps64",
+ "mmap",
+ "nice",
+ "pipe",
+ "pipe2",
+ "posix_memalign",
+ "posix_openpt",
+ "prctl",
+ "pread",
+ "preadv",
+ "preadv2",
+ "preadv64",
+ "preadv64v2",
+ "pthread_cond_timedwait",
+ "pthread_cond_wait",
+ "pthread_mutex_lock",
+ "pthread_mutex_trylock",
+ "pthread_spin_lock",
+ "pthread_spin_trylock",
+ "pwritev",
+ "pwritev2",
+ "pwritev64v2",
+ "read",
+ "readlink",
+ "readlinkat",
+ "readv",
+ "revoke",
+ "rpmatch",
+ "setdomainname",
+ "setegid",
+ "seteuid",
+ "setgid",
+ "sethostid",
+ "sethostname",
+ "setregid",
+ "setresgid",
+ "setresuid",
+ "setreuid",
+ "setrlimit",
+ "setsid",
+ "setuid",
+ "sscanf",
+ "stat",
+ "strtod",
+ "strtof",
+ "strtol",
+ "strtold",
+ "strtoll",
+ "strtoul",
+ "strtoull",
+ "symlink",
+ "symlinkat",
+ "system",
+ "truncate",
+ "truncate64",
+ "ttyname_r",
+ "vasprintf",
+ "vfscanf",
+ "vscanf",
+ "wcrtomb",
+ "write",
+ "writev"
+ ]
+ },
  "CWE332": {
  "pairs": [
  [

src/cwe_checker_lib/src/abstract_domain/domain_map.rs

@@ -98,11 +98,22 @@ where
  if self == other {
  self.clone()
  } else {
- DomainMap {
- inner: Arc::new(S::merge_map(&self.inner, &other.inner)),
- phantom: PhantomData,
- }
+ let mut new_map = self.clone();
+
+ new_map.merge_with(other);
+
+ new_map
+ }
+ }
+
+ fn merge_with(&mut self, other: &Self) -> &mut Self {
+ if self != other {
+ let mut_map = Arc::make_mut(&mut self.inner);
+
+ S::merge_map_with(mut_map, &other.inner);
  }
+
+ self
  }
 
  /// A `DomainMap` is considered to be a `Top` element if it is empty.
@@ -111,15 +122,57 @@ where
  }
 }
 
+impl<K, V, S> Default for DomainMap<K, V, S>
+where
+ K: PartialOrd + Ord + Clone,
+ V: AbstractDomain,
+ S: MapMergeStrategy<K, V> + Clone + Eq,
+{
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl<K, V, S> DomainMap<K, V, S>
+where
+ K: PartialOrd + Ord + Clone,
+ V: AbstractDomain,
+ S: MapMergeStrategy<K, V> + Clone + Eq,
+{
+ /// Returns a new, empty map into the abstract domain `V`.
+ ///
+ /// The semantics of an empty map depend on the use case. Oftentimes
+ /// non-existent keys will be mapped to the Top, Bottom, or some default
+ /// element in the target domain.
+ pub fn new() -> Self {
+ BTreeMap::new().into()
+ }
+}
+
 /// A `MapMergeStrategy` determines how the merge-method for a [`DomainMap`] works.
 ///
 /// The possible strategies are:
 /// * [`UnionMergeStrategy`]
 /// * [`IntersectMergeStrategy`]
 /// * [`MergeTopStrategy`]
 pub trait MapMergeStrategy<K: Ord + Clone, V: AbstractDomain> {
- /// This function determines how two [`DomainMap`] instances are merged as abstract domains.
- fn merge_map(map_left: &BTreeMap<K, V>, map_right: &BTreeMap<K, V>) -> BTreeMap<K, V>;
+ /// This function determines how two [`DomainMap`] instances are merged as
+ /// abstract domains.
+ ///
+ /// # Default
+ ///
+ /// Clones the left side and uses [`MapMergeStrategy::merge_map_with`] to
+ /// combine it with the right side.
+ fn merge_map(map_left: &BTreeMap<K, V>, map_right: &BTreeMap<K, V>) -> BTreeMap<K, V> {
+ let mut map = map_left.clone();
+
+ Self::merge_map_with(&mut map, map_right);
+
+ map
+ }
+
+ /// Merges `map` with `other` by modifying `map` in-place.
+ fn merge_map_with(map: &mut BTreeMap<K, V>, other: &BTreeMap<K, V>);
 }
 
 /// A [`MapMergeStrategy`] where key-value pairs whose key is only present in one input map
@@ -135,45 +188,43 @@ pub struct UnionMergeStrategy {
 }
 
 impl<K: Ord + Clone, V: AbstractDomain> MapMergeStrategy<K, V> for UnionMergeStrategy {
- fn merge_map(map_left: &BTreeMap<K, V>, map_right: &BTreeMap<K, V>) -> BTreeMap<K, V> {
- let mut merged_map = map_left.clone();
- for (key, value_right) in map_right.iter() {
- merged_map
- .entry(key.clone())
+ fn merge_map_with(map: &mut BTreeMap<K, V>, other: &BTreeMap<K, V>) {
+ for (key, value_other) in other.iter() {
+ map.entry(key.clone())
  .and_modify(|value| {
- *value = value.merge(value_right);
+ value.merge_with(value_other);
  })
- .or_insert_with(|| value_right.clone());
+ .or_insert_with(|| value_other.clone());
  }
- merged_map
  }
 }
 
-/// A [`MapMergeStrategy`] where the merge function only keeps keys
-/// that are present in both input maps.
-/// Furthermore, keys whose values are merged to the `Top` value are also removed from the merged map.
+/// A [`MapMergeStrategy`] where the merge function only keeps keys that are
+/// present in both input maps.
+///
+/// Furthermore, keys whose values are merged to the `Top` value are also
+/// removed from the merged map.
 ///
-/// The strategy is meant to be used for maps,
-/// where keys not present in the map have an implicit `Top` value associated to them.
-/// The strategy implicitly assumes
-/// that the `Top` value of the value abstract domain is an actual maximal value of the domain.
+/// The strategy is meant to be used for maps, where keys not present in the map
+/// have an implicit `Top` value associated to them. The strategy implicitly
+/// assumes that the `Top` value of the value abstract domain is an actual
+/// maximal value of the domain.
 #[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)]
 pub struct IntersectMergeStrategy {
  _private: (), // Marker to prevent instantiation
 }
 
 impl<K: Ord + Clone, V: AbstractDomain> MapMergeStrategy<K, V> for IntersectMergeStrategy {
- fn merge_map(map_left: &BTreeMap<K, V>, map_right: &BTreeMap<K, V>) -> BTreeMap<K, V> {
- let mut merged_map = BTreeMap::new();
- for (key, value_left) in map_left.iter() {
- if let Some(value_right) = map_right.get(key) {
- let merged_value = value_left.merge(value_right);
- if !merged_value.is_top() {
- merged_map.insert(key.clone(), merged_value);
- }
- }
- }
- merged_map
+ fn merge_map_with(map: &mut BTreeMap<K, V>, other: &BTreeMap<K, V>) {
+ map.retain(|k, value| {
+ let Some(value_other) = other.get(k) else {
+ return false;
+ };
+
+ value.merge_with(value_other);
+
+ !value.is_top()
+ });
  }
 }
 
@@ -190,27 +241,29 @@ pub struct MergeTopStrategy {
 }
 
 impl<K: Ord + Clone, V: AbstractDomain + HasTop> MapMergeStrategy<K, V> for MergeTopStrategy {
- fn merge_map(map_left: &BTreeMap<K, V>, map_right: &BTreeMap<K, V>) -> BTreeMap<K, V> {
- let mut merged_map = BTreeMap::new();
- for (var, value_left) in map_left.iter() {
- let merged_value = if let Some(value_right) = map_right.get(var) {
- value_left.merge(value_right)
+ fn merge_map_with(map: &mut BTreeMap<K, V>, other: &BTreeMap<K, V>) {
+ map.retain(|key, value| {
+ if let Some(value_other) = other.get(key) {
+ value.merge_with(value_other);
  } else {
- value_left.top().merge(value_left)
+ let top = value.top();
+
+ value.merge_with(&top);
  };
- if !merged_value.is_top() {
- merged_map.insert(var.clone(), merged_value);
- }
- }
- for (var, value_right) in map_right.iter() {
- if map_left.get(var).is_none() {
- let merged_value = value_right.top().merge(value_right);
+
+ !value.is_top()
+ });
+ for (k, value_other) in other.iter() {
+ if map.get(k).is_none() {
+ let mut merged_value = value_other.top();
+
+ merged_value.merge_with(value_other);
+
  if !merged_value.is_top() {
- merged_map.insert(var.clone(), merged_value);
+ map.insert(k.clone(), merged_value);
  }
  }
  }
- merged_map
  }
 }