From e4839b8539628e712d6ed561f5c7e529e4a83099 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Mon, 18 Mar 2024 19:04:21 +0100 Subject: [PATCH 01/26] lib/analysis/taint: introduce `handle_empty_state_out` callback Add a callback that allows a taint analysis to hook into the fixpoint computation when some transfer function maps its input state(s) to the empty state. For some analyses this event may be a sink, e.g., for CWE252, while for many analyses it does not make sense to propagate empty states further since it is impossible to generate a non-empty state from them; they may use this hook to optimize resource usage. Signed-off-by: Valentin Obst --- src/cwe_checker_lib/src/analysis/taint/mod.rs | 89 +++++++++++++++---- 1 file changed, 73 insertions(+), 16 deletions(-) diff --git a/src/cwe_checker_lib/src/analysis/taint/mod.rs b/src/cwe_checker_lib/src/analysis/taint/mod.rs index 92a34dd0..ce02409d 100644 --- a/src/cwe_checker_lib/src/analysis/taint/mod.rs +++ b/src/cwe_checker_lib/src/analysis/taint/mod.rs @@ -43,6 +43,23 @@ use state::State; /// to many taint analyses. However, you almost certainly want to override some /// of them to implement the custom logic of your analysis. pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef { + /// Called when a transition function mapped the input state to the empty + /// state. + /// + /// This function will be called every time a default transition function + /// maps a (possibly empty) input state to the empty state. Its return value + /// will override the `Some(empty_state)` return value of the transition + /// function. + /// + /// # Default + /// + /// Just returns `None`. This is the desired behavior as long as it is + /// impossible for transition functions to generate taint from an empty + /// state. + fn handle_empty_state_out(&self, _tid: &Tid) -> Option { + None + } + /// Update taint state on a function call without further target information. /// /// # Default @@ -51,7 +68,7 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef fn update_call_generic( &self, state: &State, - _call_tid: &Tid, + call_tid: &Tid, calling_convention_hint: &Option, ) -> Option { let mut new_state = state.clone(); @@ -62,7 +79,11 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef new_state.remove_non_callee_saved_taint(calling_conv); } - Some(new_state) + if new_state.is_empty() { + self.handle_empty_state_out(call_tid) + } else { + Some(new_state) + } } /// Transition function for edges of type [`Call`]. @@ -91,24 +112,50 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef /// Corresponds to inter-program calls, i.e., calls to shared libraries. /// /// [`ExternCallStub`]: crate::analysis::graph::Edge::ExternCallStub - fn update_call_stub(&self, state: &State, call: &Term) -> Option; + /// + /// # Default + /// + /// Remove taint from non-callee-saved registers. + fn update_call_stub(&self, state: &State, call: &Term) -> Option { + match &call.term { + Jmp::Call { target, .. } => { + let project = >::as_ref(self); + let extern_symbol = project + .program + .term + .extern_symbols + .get(target) + .expect("TA: BUG: Unable to find extern symbol for call."); + let mut new_state = state.clone(); + + new_state + .remove_non_callee_saved_taint(project.get_calling_convention(extern_symbol)); + + if new_state.is_empty() { + self.handle_empty_state_out(&call.tid) + } else { + Some(new_state) + } + } + Jmp::CallInd { .. } => self.update_call_generic(state, &call.tid, &None), + _ => panic!("TA: BUG: Malformed control flow graph encountered."), + } + } /// Returns the new taint state after a jump. /// /// # Default /// - /// Clones the state before the jump, or returns `None` if the state is - /// empty. + /// Clones the state before the jump. fn update_jump( &self, state: &State, - _jump: &Term, + jump: &Term, _untaken_conditional: Option<&Term>, _target: &Term, ) -> Option { if state.is_empty() { - // Without taint there is nothing to propagate. - None + self.handle_empty_state_out(&jump.tid) } else { Some(state.clone()) } @@ -190,7 +237,7 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef return_term: &Term, calling_convention: &Option, ) -> Option { - match (state_before_call, state_before_return) { + let new_state = match (state_before_call, state_before_return) { (Some(state_before_call), Some(state_before_return)) => { let state_from_caller = self.update_call_generic(state_before_call, &call_term.tid, calling_convention); @@ -235,6 +282,17 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef ) }), _ => None, + }; + + match new_state { + Some(state) => { + if state.is_empty() { + self.handle_empty_state_out(&return_term.tid) + } else { + Some(state) + } + } + None => None, } } @@ -331,9 +389,13 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef &self, _old_state: &State, new_state: State, - _def: &Term, + def: &Term, ) -> Option { - Some(new_state) + if new_state.is_empty() { + self.handle_empty_state_out(&def.tid) + } else { + Some(new_state) + } } } @@ -383,11 +445,6 @@ impl<'a, T: TaintAnalysis<'a>> forward_interprocedural_fixpoint::Context<'a> for } fn update_def(&self, state: &Self::Value, def: &Term) -> Option { - if state.is_empty() { - // Without taint there is nothing to propagate. - return None; - } - let new_state = match &def.term { Def::Assign { var, value } => self.update_def_assign(state, &def.tid, var, value), Def::Load { var, address } => self.update_def_load(state, &def.tid, var, address), From 30739fe32d7e35248f3ecf6757b79808f314fe7a Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Mon, 18 Mar 2024 19:10:00 +0100 Subject: [PATCH 02/26] lib/analysis/taint: add `get_register_taint` and `has_register_taint` methods to state Signed-off-by: Valentin Obst --- src/cwe_checker_lib/src/analysis/taint/state.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/cwe_checker_lib/src/analysis/taint/state.rs b/src/cwe_checker_lib/src/analysis/taint/state.rs index 0afdf702..d936773f 100644 --- a/src/cwe_checker_lib/src/analysis/taint/state.rs +++ b/src/cwe_checker_lib/src/analysis/taint/state.rs @@ -223,6 +223,14 @@ impl State { } } + /// Returns the taint state of the given register. + pub fn get_register_taint(&self, register: &Variable) -> Taint { + self.register_taint + .get(register) + .copied() + .unwrap_or(Taint::Top(register.size)) + } + /// Returns true if the memory object with the given ID contains a tainted /// value. pub fn check_mem_id_for_taint(&self, id: &AbstractIdentifier) -> bool { @@ -405,7 +413,14 @@ impl State { /// Check whether `self` contains any taint at all. pub fn is_empty(&self) -> bool { - !self.has_memory_taint() && self.register_taint.is_empty() + !self.has_memory_taint() && !self.has_register_taint() + } + + /// Check whether there are any tainted registers in the state. + pub fn has_register_taint(&self) -> bool { + self.register_taint + .iter() + .any(|(_, taint)| matches!(*taint, Taint::Tainted(_))) } /// Check whether there is any tainted memory in the state. From 6b4bd4edb6a90ab69d4adbc8d02b7db10d0b3de5 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Mon, 25 Mar 2024 20:31:41 +0100 Subject: [PATCH 03/26] lib/analysis/taint: introduce `update_extern_call` callback Break the transition function for `ExternCallStub` edges up into two parts. This allows analyses that are only interested in handling calls to library functions to do so in a more convenient way. Reduces boilerplate code and makes sure they can not forget to call `handle_empty_state_out`. Signed-off-by: Valentin Obst --- src/cwe_checker_lib/src/analysis/taint/mod.rs | 37 +++++++++++++++---- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/src/cwe_checker_lib/src/analysis/taint/mod.rs b/src/cwe_checker_lib/src/analysis/taint/mod.rs index ce02409d..346c8940 100644 --- a/src/cwe_checker_lib/src/analysis/taint/mod.rs +++ b/src/cwe_checker_lib/src/analysis/taint/mod.rs @@ -107,10 +107,34 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef None } + /// Transition function for calls to external functions. + /// + /// # Default + /// + /// Removes taint from non-callee-saved registers. + fn update_extern_call( + &self, + state: &State, + _call: &Term, + project: &Project, + extern_symbol: &ExternSymbol, + ) -> Option { + let mut new_state = state.clone(); + + new_state.remove_non_callee_saved_taint(project.get_calling_convention(extern_symbol)); + + Some(new_state) + } + /// Transition function for edges of type [`ExternCallStub`]. /// /// Corresponds to inter-program calls, i.e., calls to shared libraries. + /// Currently, indirect calls also lead to edges of type [`ExternCallStub`]. + /// If you are only interested in handling calls to library functions + /// consider implementing [`update_extern_call`] instead. /// + /// [`update_extern_call`]: TaintAnalysis::update_extern_call + /// [indirect calls]: crate::intermediate_representation::Jmp::CallInd /// [`ExternCallStub`]: crate::analysis::graph::Edge::ExternCallStub /// /// # Default @@ -126,15 +150,12 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef .extern_symbols .get(target) .expect("TA: BUG: Unable to find extern symbol for call."); - let mut new_state = state.clone(); - new_state - .remove_non_callee_saved_taint(project.get_calling_convention(extern_symbol)); - - if new_state.is_empty() { - self.handle_empty_state_out(&call.tid) - } else { - Some(new_state) + match self.update_extern_call(state, call, project, extern_symbol) { + Some(new_state) if new_state.is_empty() => { + self.handle_empty_state_out(&call.tid) + } + new_state_option => new_state_option, } } Jmp::CallInd { .. } => self.update_call_generic(state, &call.tid, &None), From 986ddc557fa7570ccabccb0f07fe618564e31df0 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Thu, 14 Mar 2024 18:12:58 +0100 Subject: [PATCH 04/26] lib/checkers: add initial check for CWE252 Signed-off-by: Valentin Obst --- src/caller/src/main.rs | 2 +- src/config.json | 5 + src/cwe_checker_lib/src/checkers.rs | 1 + src/cwe_checker_lib/src/checkers/cwe_252.rs | 275 +++++++++++++++ .../src/checkers/cwe_252/context.rs | 313 ++++++++++++++++++ .../src/checkers/cwe_252/isolated_returns.rs | 156 +++++++++ src/cwe_checker_lib/src/lib.rs | 1 + src/cwe_checker_lib/src/utils/symbol_utils.rs | 36 +- 8 files changed, 786 insertions(+), 3 deletions(-) create mode 100644 src/cwe_checker_lib/src/checkers/cwe_252.rs create mode 100644 src/cwe_checker_lib/src/checkers/cwe_252/context.rs create mode 100644 src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs diff --git a/src/caller/src/main.rs b/src/caller/src/main.rs index 1e8cf373..d6f34b98 100644 --- a/src/caller/src/main.rs +++ b/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 diff --git a/src/config.json b/src/config.json index 1f0b2d7a..28a756cc 100644 --- a/src/config.json +++ b/src/config.json @@ -73,6 +73,11 @@ "CWE248": { "symbols": [] }, + "CWE252": { + "symbols": [ + "fgets" + ] + }, "CWE332": { "pairs": [ [ diff --git a/src/cwe_checker_lib/src/checkers.rs b/src/cwe_checker_lib/src/checkers.rs index b1ae6437..332a78b3 100644 --- a/src/cwe_checker_lib/src/checkers.rs +++ b/src/cwe_checker_lib/src/checkers.rs @@ -15,6 +15,7 @@ pub mod cwe_134; pub mod cwe_190; pub mod cwe_215; pub mod cwe_243; +pub mod cwe_252; pub mod cwe_332; pub mod cwe_337; pub mod cwe_367; diff --git a/src/cwe_checker_lib/src/checkers/cwe_252.rs b/src/cwe_checker_lib/src/checkers/cwe_252.rs new file mode 100644 index 00000000..7a08f8e8 --- /dev/null +++ b/src/cwe_checker_lib/src/checkers/cwe_252.rs @@ -0,0 +1,275 @@ +//! CWE-252: Unchecked Return Value. +//! +//! It is a common programming pattern that called procedures indicate their +//! success or failure to the caller via their return value. If a caller does +//! not check the return value of a called procedure they can not know if the +//! operation was successful or not. This may lead to bugs with security +//! implications when the program resumes execution under the assumption that +//! the failed call has worked. +//! +//! # Examples +//! +//! See CVE for examples in Linux user-mode programs: +//! +//! - CVE-2007-5191 +//! - CVE-2017-6964 +//! - CVE-2018-16643 +//! - CVE-2019-15900 +//! - CVE-2023-40303 +//! +//! Also see [CWE252 at Mitre]. +//! +//! [CWE252 at Mitre]: https://cwe.mitre.org/data/definitions/252.html +//! +//! # Algorithm +//! +//! We perform a taint analysis where the sources are return values of calls to +//! external functions and the sinks are: +//! +//! - Places where all taint vanishes from the state. Here, the program losses +//! all information about success of failure of the API call; thus, it can not +//! possibly adapt its behavior in the subsequent execution. +//! - Taint reaches a return site of a function without any taint being returned +//! to the caller. Here, the caller of the function cannot know if the API +//! call was successful. +//! +//! Taint propagation is stopped along paths as soon as a conditional control +//! flow transfer depend on a tainted value. +//! +//! # Limitations +//! +//! ## False Positives +//! +//! - For many API functions the necessity to check the return value depends on +//! the context of the caller. +//! - Cases where the result is checked before it is assumed that the operation +//! has worked, but there are paths from the call site that are correct +//! irrespective of the success or failure of the call. +//! - Patterns where the return value is handled by giving it as an argument to +//! another function call. +//! - ... +//! +//! ## False Negatives +//! +//! - Return value is checked but the program does not act accordingly. +//! - The API function is not in the list of checked functions. +//! +//! # Configuration +//! +//! The list of checked external functions can be configured via the +//! `config.json`. By selecting the `strict_mode` additional functions can be +//! included, however, those are more likely to produce false positives. + +use crate::analysis::graph::{Edge, NodeIndex}; +use crate::analysis::pointer_inference::PointerInference; +use crate::intermediate_representation::{ExternSymbol, Jmp, Project, Term}; +use crate::pipeline::AnalysisResults; +use crate::prelude::*; +use crate::utils::log::{CweWarning, LogMessage}; +use crate::utils::{symbol_utils, ToJsonCompact}; +use crate::CweModule; + +use petgraph::visit::EdgeRef; + +use std::collections::{BTreeMap, HashSet, VecDeque}; +use std::sync::Arc; + +mod context; +mod isolated_returns; + +use context::*; +use isolated_returns::*; + +/// CWE-252: Unchecked Return Value. +pub static CWE_MODULE: CweModule = CweModule { + name: "CWE252", + version: "0.1", + run: check_cwe, +}; + +/// Configuration of the check; this is read from the `config.json` file. +#[derive(Deserialize)] +struct Config { + strict_mode: bool, + /// External symbols whose return values must be checked. + symbols: HashSet, + /// Additional symbols that are only checked when we run in strict mode. + strict_symbols: HashSet, +} + +impl Config { + fn into_symbols(mut self) -> HashSet { + if self.strict_mode { + self.symbols.extend(self.strict_symbols); + } + + self.symbols + } +} + +/// Call whose return value must be checked. +#[derive(Clone, Copy)] +struct MustUseCall<'a> { + /// Information about the function that was called. + symbol: &'a ExternSymbol, + /// CFG node where the call will return to. + return_node: NodeIndex, + /// IR instruction of the call. + jmp: &'a Term, +} + +impl MustUseCall<'_> { + /// Returns a copy of the name of the external function that was called. + pub fn get_symbol_name(&self) -> String { + self.symbol.name.clone() + } +} + +/// List of calls to analyze. +struct Worklist<'a> { + /// Remaining calls to analyze. + calls: VecDeque>, +} + +impl<'a> Worklist<'a> { + /// Creates a new worklist of external function calls to analyze. + /// + /// Searches the program for calls to the functions specified in the + /// `config` and gathers them into a new worklist. + fn new(analysis_results: &'a AnalysisResults, symbols: &HashSet) -> Self { + let symbol_map = symbol_utils::get_symbol_map_fast(analysis_results.project, symbols); + let cfg = analysis_results + .pointer_inference + .expect("CWE252: BUG: No pointer inference results.") + .get_graph(); + + Worklist { + calls: cfg + .edge_references() + .filter_map(|edge| { + let Edge::ExternCallStub(jmp) = edge.weight() else { + return None; + }; + let Jmp::Call { target, .. } = &jmp.term else { + return None; + }; + let return_node = edge.target(); + Some(MustUseCall { + symbol: symbol_map.get(target)?, + return_node, + jmp, + }) + }) + .collect(), + } + } +} + +/// Represents the full CWE252 analysis of a given project. +struct CweAnalysis<'a, 'b: 'a> { + /// Remaining calls to external functions that need to be analyzed. + worklist: Worklist<'a>, + isolated_returns: Arc>, + project: &'a Project, + pi_result: &'a PointerInference<'b>, + /// Used to collect CWE warnings sent by the analyses for inidividual calls. + cwe_collector: crossbeam_channel::Receiver, + /// Given to analyses for inidividual calls to send their CWE warnings. + cwe_sender_proto: crossbeam_channel::Sender, +} + +impl<'a, 'b: 'a> CweAnalysis<'a, 'b> { + /// Creates a new CWE252 analysis for the given project and configuration. + fn new(analysis_results: &'a AnalysisResults<'b>, config: Config) -> Self { + let channel = crossbeam_channel::unbounded(); + let cfg = analysis_results.control_flow_graph; + + Self { + worklist: Worklist::new(analysis_results, &config.into_symbols()), + isolated_returns: Arc::new(get_isolated_returns(cfg)), + project: analysis_results.project, + pi_result: analysis_results.pointer_inference.unwrap(), + cwe_collector: channel.1, + cwe_sender_proto: channel.0, + } + } + + /// Pops a call of the worklist and returns the taint analysis definition + /// for it. + fn next_call_ctx(&mut self) -> Option<(IsolatedReturnAnalysis<'_>, TaCompCtx<'_, 'b>)> { + self.worklist.calls.pop_front().map(|call| { + ( + IsolatedReturnAnalysis::new( + call, + Arc::clone(&self.isolated_returns), + self.project, + self.cwe_sender_proto.clone(), + ), + TaCompCtx::new(call, self.project, self.pi_result, &self.cwe_sender_proto), + ) + }) + } + + /// Runs the CWE252 analysis and returns the generated warnings. + fn run(mut self) -> (Vec, Vec) { + while let Some((isolated_returns, ta_comp_ctx)) = self.next_call_ctx() { + let mut ta_comp = ta_comp_ctx.into_computation(); + + ta_comp.compute_with_max_steps(100); + + isolated_returns.analyze(&ta_comp); + } + + ( + Vec::new(), + self.cwe_collector + .try_iter() + .map(|msg| (msg.tids.clone(), msg)) + .collect::>() + .into_values() + .collect(), + ) + } +} + +fn generate_cwe_warning( + sender: &crossbeam_channel::Sender, + call: &MustUseCall<'_>, + warning_location: &Tid, + reason: &str, +) { + let taint_source = call.jmp; + let taint_source_name = call.get_symbol_name(); + let cwe_warning = CweWarning::new( + CWE_MODULE.name, + CWE_MODULE.version, + format!( + "(Unchecked Return Value) There is no check of the return value of {} ({}).", + taint_source.tid.address, taint_source_name + ), + ) + .addresses(vec![ + taint_source.tid.address.clone(), + warning_location.address.clone(), + ]) + .tids(vec![ + format!("{}", taint_source.tid), + format!("{}", warning_location), + ]) + .symbols(vec![taint_source_name]) + .other(vec![vec![format!("reason={}", reason.to_string())]]); + sender + .send(cwe_warning) + .expect("CWE252: failed to send CWE warning"); +} + +/// CWE-252: Unchecked Return Value. +pub fn check_cwe( + analysis_results: &AnalysisResults, + cwe_params: &serde_json::Value, +) -> (Vec, Vec) { + let config: Config = + serde_json::from_value(cwe_params.clone()).expect("CWE252: invalid configuration"); + + CweAnalysis::new(analysis_results, config).run() +} diff --git a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs new file mode 100644 index 00000000..ca497db9 --- /dev/null +++ b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs @@ -0,0 +1,313 @@ +//! Definition of the Taint Analysis for CWE252. +//! +//! Implementation of the [`TaintAnalysis`] for this CWE check. See the module +//! documentation for more details on the algorithm and its limitations. + +use super::MustUseCall; + +use crate::analysis::fixpoint; +use crate::analysis::forward_interprocedural_fixpoint::{ + self, create_computation as fwd_fp_create_computation, +}; +use crate::analysis::graph::{Graph as Cfg, HasCfg}; +use crate::analysis::interprocedural_fixpoint_generic::NodeValue; +use crate::analysis::pointer_inference::{Data as PiData, PointerInference}; +use crate::analysis::taint::{state::State as TaState, TaintAnalysis}; +use crate::analysis::vsa_results::{HasVsaResult, VsaResult}; +use crate::intermediate_representation::{Blk, ExternSymbol, Jmp, Project, Term, Tid}; +use crate::utils::debug::ToJsonCompact; +use crate::utils::log::CweWarning; + +use std::convert::AsRef; + +/// Type of the fixpoint computation of the taint analysis. +pub type FpComputation<'a, 'b> = fixpoint::Computation< + forward_interprocedural_fixpoint::GeneralizedContext<'a, TaCompCtx<'a, 'b>>, +>; + +/// Type that represents the definition of the taint analysis. +/// +/// Values of this type represent the taint analysis for a particular call to an +/// external function. +pub struct TaCompCtx<'a, 'b: 'a> { + /// Extern function call that is analyzed. + call: MustUseCall<'a>, + project: &'a Project, + pi_result: &'a PointerInference<'b>, + /// Used to send generated CWE warnings to the collector. + cwe_sender: crossbeam_channel::Sender, +} + +impl<'a, 'b: 'a> TaCompCtx<'a, 'b> { + /// Creates a new taint analysis context for the given call to an external + /// function. + pub(super) fn new( + call: MustUseCall<'a>, + project: &'a Project, + pi_result: &'a PointerInference<'b>, + cwe_sender: &'a crossbeam_channel::Sender, + ) -> Self { + Self { + call, + project, + pi_result, + cwe_sender: cwe_sender.clone(), + } + } + + /// Converts the taint analysis context into a fixpoint computation. + /// + /// The returned computation can be solved to analyze this particular + /// function call. + pub fn into_computation(self) -> FpComputation<'a, 'b> { + let symbol = self.call.symbol; + let vsa_result = self.vsa_result(); + let return_node = self.call.return_node; + let node_value = NodeValue::Value(TaState::new_return(symbol, vsa_result, return_node)); + + let mut computation = fwd_fp_create_computation(self, None); + + computation.set_node_value(return_node, node_value); + + computation + } + + fn generate_cwe_warning(&self, warning_location: &Tid, reason: &str) { + super::generate_cwe_warning(&self.cwe_sender, &self.call, warning_location, reason) + } +} + +impl<'a> HasCfg<'a> for TaCompCtx<'a, '_> { + fn get_cfg(&self) -> &Cfg<'a> { + self.pi_result.get_graph() + } +} + +impl HasVsaResult for TaCompCtx<'_, '_> { + fn vsa_result(&self) -> &impl VsaResult { + self.pi_result + } +} + +impl AsRef for TaCompCtx<'_, '_> { + fn as_ref(&self) -> &Project { + self.project + } +} + +impl<'a> TaintAnalysis<'a> for TaCompCtx<'a, '_> { + /// Generates a CWE warning when a transition function returns the empty + /// state. + /// + /// If a transition function returns the empty state this implies that there + /// is some time in the program where all information about the return value + /// of a fallible call has been eradicated without a previous check. + /// + /// From this time onwards, the program cannot possibly know if the fallible + /// operation has succeeded or not. This point is reported as a possible bug + /// by the check. + /// + /// For an example where this rule is needed to detect an error consider the + /// following program: + /// + /// ```c + /// x = fallible_call(); + /// + /// if (some_unrelated_condition) { + /// x = 42; + /// } + /// + /// if (x < 0) { + /// return x; + /// } + /// + /// // Do something that assumes that `fallible_call` has worked. + /// ``` + /// + /// A pure forward 'may' data flow analysis would only see a tainted `x` + /// being used in a comparison and consider the fallible call to be checked. + /// With this condition, the analysis would emit a warning for the term + /// `x = 42` since at this point all information about the return value is + /// lost. + fn handle_empty_state_out(&self, tid: &Tid) -> Option { + self.generate_cwe_warning(tid, "empty_state"); + + None + } + + /// Update taint state on call to extern function. + /// + /// We almost always just want to remove the taint from non-callee-saved + /// registers. However, for calls/jumps into nonreturning functions at the + /// end of a procedure we need some logic to suppress false positives. + fn update_extern_call( + &self, + state: &TaState, + _call: &Term, + project: &Project, + extern_symbol: &ExternSymbol, + ) -> Option { + // External symbols that are unconditional termination points. It does + // not make sense to propagate taint through those so we always return + // `None`. + if extern_symbol.no_return { + return None; + } + // External symbols that are effectively no-ops. Here we always return + // the unmodified input state. + // + // FIXME: This is an artifact of the way in which we generate the CFG. + // On x86 and amd64 kernel functions end in a 'jmp retpoline' but we + // generate a 'call extern; return;', where we introduced an artificial + // isolated return node. Another workaround would be to replace the jmp + // with a ret in a normalization pass. + const EXTERN_NOOP: [&str; 1] = ["__x86_return_thunk"]; + + let mut new_state = state.clone(); + + if !EXTERN_NOOP.iter().any(|s| s == &extern_symbol.name) { + new_state.remove_non_callee_saved_taint(project.get_calling_convention(extern_symbol)); + } + + Some(new_state) + } + + /// Stops taint propagation if jump depends on a tainted condition. + /// + /// We assume that any check that depends on tainted values is a check of + /// the return value of the fallible function, and that the program handles + /// all outcomes correctly. + /// + /// A jump can depend on a tainted condition in two ways, either it is + /// executed because the condition evaluated to `true`, or because it + /// evaluated to `false`, both cases must be handled here. + fn update_jump( + &self, + state: &TaState, + jump: &Term, + untaken_conditional: Option<&Term>, + _target: &Term, + ) -> Option { + // If this control flow transfer depends on a condition involving + // a tainted value then we do not propagate any taint information to + // the destination. + match (&jump.term, untaken_conditional) { + // Directly depends on a tainted value. + (Jmp::CBranch { condition, .. }, _) if state.eval(condition).is_tainted() => None, + // Branch is only taken because a condition based on a tainted value + // evaluated to false. + ( + _, + Some(Term { + tid: _, + term: Jmp::CBranch { condition, .. }, + }), + ) if state.eval(condition).is_tainted() => None, + // Does not depend on tainted values. + _ => { + if state.is_empty() { + self.handle_empty_state_out(&jump.tid) + } else { + Some(state.clone()) + } + } + } + } + + /// Propagates taint from callee to caller. + /// + /// The check performs a limited interprocedural taint analysis. The main + /// idea is that: *If a function may propagate the return value of a + /// fallible function call to its caller without checking it, then the + /// function itself becomes "must check".* (In the sense that the caller is + /// now responsible for checking its return value.) + /// + /// Limitations stem from two sources: + /// + /// - It is not possible to propagate memory taint from the callee to the + /// caller. + /// - The calling convention may be unknown. + /// + /// The the source code comments for further information. + /// + /// Furthermore, this callback is responsible for detecting cases where + /// taint may reach the end of a function without being returned. + /// This implies that the called function may have ignored the return + /// value of a fallible function call and there is no way for the caller to + /// know the outcome. The check raises a warning in those cases. + fn update_return_callee( + &self, + state: &TaState, + _call_term: &Term, + return_term: &Term, + calling_convention: &Option, + ) -> Option { + if !state.has_register_taint() { + // No register taint means that we can not propagate any taint to + // the caller (since we are currently unable to propagate memory + // taint from callee to caller). + // + // We consider the information in the tainted memory to be "lost", + // and thus we generate a CWE warning since its presence implies + // that the return value of a fallible function call may have been + // ignored by the called function. + self.generate_cwe_warning(&return_term.tid, "return_no_reg_taint"); + + // We still want to allow for the propagation of taint from the + // call site. + Some(TaState::new_empty()) + } else if let Some(calling_convention) = self + .project + .get_specific_calling_convention(calling_convention) + { + // We have tainted registers and we know the calling convention: + // + // If there is a non-empty overlap between the tainted registers and + // the return registers we propagate the taint to the caller, + // which makes them responsible for checking it. + // + // Else it means that the called function may have ignored the + // return value of a fallible function call and we emit a warning. + let propagated_state = calling_convention.get_all_return_register().iter().fold( + TaState::new_empty(), + |mut propagated_state, return_register| { + propagated_state.set_register_taint( + return_register, + state.get_register_taint(return_register), + ); + + propagated_state + }, + ); + + if propagated_state.has_register_taint() { + Some(propagated_state) + } else { + self.generate_cwe_warning(&return_term.tid, "return_no_taint_returned"); + + // We still want to allow for the propagation of taint from the + // call site. + Some(TaState::new_empty()) + } + } else { + // We have tainted registers but we do not know the calling + // convention. Here we simply return the complete register taint + // of the callee to the caller. This heuristic should in practice + // hopefully be a good approximation to the real calling convention: + // - It is an over approximation so return registers will be + // propagated correctly. + // - There is a chance that callee-saved registers have been + // overwritten with their saved values by the function epilog and + // are thus not tainted. + // - There is a chance that caller saved registers will be restored + // by the caller such that the taint is immediatly eliminated and + // we catch cases where the called function has ignored tainted + // values. + let mut new_state = state.clone(); + + new_state.remove_all_memory_taints(); + + Some(new_state) + } + } +} diff --git a/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs b/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs new file mode 100644 index 00000000..3d7b6364 --- /dev/null +++ b/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs @@ -0,0 +1,156 @@ +//! Analyse Isolated Return Sites. +//! +//! Taint that reaches a return site implies that there is some path from the +//! call to a fallible function to the return instruction where the return value +//! is not checked. If additionally, the taint is not not returned to the caller +//! we have a bug, since with the return of the function all information about +//! success or failure of the function call is lost. +//! +//! For each function that is called at least once, we catch those cases during +//! the FP computation. However, for functions that are never called, e.g., +//! exported functions in shared libraries or functions that are only ever +//! called indirectly, we need an additional pass once we have the result of +//! the FP computation. + +use crate::abstract_domain::AbstractDomain; +use crate::analysis::graph::{Graph, Node, NodeIndex}; +use crate::intermediate_representation::{Jmp, Project, Tid}; +use crate::utils::log::CweWarning; + +use std::collections::HashSet; +use std::sync::Arc; + +use super::context; +use super::{generate_cwe_warning, MustUseCall}; + +/// Represents a return site of a function. +#[derive(Hash, Eq, PartialEq, Debug)] +pub struct ReturnSite<'a> { + /// CFG node of the end of the block containing the return instruction. + ret_node: NodeIndex, + /// Calling convention of the function that returns. + calling_convention: &'a Option, + /// Identifier of the return instruction. + ret_insn_tid: &'a Tid, +} + +impl<'a> ReturnSite<'a> { + /// Create a new `ReturnSite`. + fn new( + ret_node: NodeIndex, + calling_convention: &'a Option, + ret_insn_tid: &'a Tid, + ) -> Self { + Self { + ret_node, + calling_convention, + ret_insn_tid, + } + } +} + +/// The set of all isolated return sites of a binary. +pub type IsolatedReturns<'a> = HashSet>; + +/// Represents the post-fixpoint-computation pass that checks isolated return +/// sites for taint. +pub struct IsolatedReturnAnalysis<'a> { + call: MustUseCall<'a>, + isolated_returns: Arc>, + project: &'a Project, + cwe_sender: crossbeam_channel::Sender, +} + +impl<'a> IsolatedReturnAnalysis<'a> { + /// Create a new `IsolatedReturnAnalysis`. + pub fn new( + call: MustUseCall<'a>, + isolated_returns: Arc>, + project: &'a Project, + cwe_sender: crossbeam_channel::Sender, + ) -> Self { + Self { + call, + isolated_returns, + project, + cwe_sender, + } + } + + /// Checks isolated return sites for taint with the results of the given + /// `computation`. + /// + /// Generates CWE warnings when non-returned taint is found. + pub fn analyze(&self, computation: &context::FpComputation<'_, '_>) { + for (taint_state, calling_convention, ret_insn_tid) in + // Filter isolated returns with a taint state. + self.isolated_returns.iter().filter_map( + |ReturnSite { + ret_node, + calling_convention, + ret_insn_tid, + }| { + computation + .node_values() + .get(ret_node) + .map(|state| (state.unwrap_value(), calling_convention, ret_insn_tid)) + }, + ) + { + if !taint_state.has_register_taint() { + // Emit a warning since we do not consider cases where memory + // taint may be returned. + generate_cwe_warning( + &self.cwe_sender, + &self.call, + ret_insn_tid, + "isolated_returns_no_reg_taint", + ); + } else if let Some(calling_convention) = self + .project + .get_specific_calling_convention(calling_convention) + { + // If no taint is returned we emit a warning. + if calling_convention + .get_all_return_register() + .iter() + .all(|return_register| taint_state.get_register_taint(return_register).is_top()) + { + generate_cwe_warning( + &self.cwe_sender, + &self.call, + ret_insn_tid, + "isolated_returns_no_taint_returned", + ); + } + } + } + } +} + +/// Get the set of all isolated return sites in the given interprocedural CFG. +pub fn get_isolated_returns<'a>(cfg: &Graph<'a>) -> IsolatedReturns<'a> { + cfg.node_indices() + .filter_map(|node| { + if cfg.edges(node).next().is_some() { + // By definition, a node with outgoing edges cannot be an + // isolated return. + None + } else if let Node::BlkEnd(blk, sub) = cfg[node] { + blk.term.jmps.iter().find_map(|jmp| { + if matches!(jmp.term, Jmp::Return(_)) { + Some(ReturnSite::new( + node, + &sub.term.calling_convention, + &jmp.tid, + )) + } else { + None + } + }) + } else { + None + } + }) + .collect() +} diff --git a/src/cwe_checker_lib/src/lib.rs b/src/cwe_checker_lib/src/lib.rs index c3674588..1957b5db 100644 --- a/src/cwe_checker_lib/src/lib.rs +++ b/src/cwe_checker_lib/src/lib.rs @@ -119,6 +119,7 @@ pub fn get_modules() -> Vec<&'static CweModule> { &crate::checkers::cwe_190::CWE_MODULE, &crate::checkers::cwe_215::CWE_MODULE, &crate::checkers::cwe_243::CWE_MODULE, + &crate::checkers::cwe_252::CWE_MODULE, &crate::checkers::cwe_332::CWE_MODULE, &crate::checkers::cwe_337::CWE_MODULE, &crate::checkers::cwe_367::CWE_MODULE, diff --git a/src/cwe_checker_lib/src/utils/symbol_utils.rs b/src/cwe_checker_lib/src/utils/symbol_utils.rs index 5cee474b..5e6d66ed 100644 --- a/src/cwe_checker_lib/src/utils/symbol_utils.rs +++ b/src/cwe_checker_lib/src/utils/symbol_utils.rs @@ -1,7 +1,7 @@ //! Helper functions for common tasks utilizing extern symbols, //! e.g. searching for calls to a specific extern symbol. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use crate::intermediate_representation::*; @@ -42,7 +42,12 @@ pub fn get_calls_to_symbols<'a, 'b>( } /// Get a map from TIDs to the corresponding extern symbol struct. -/// Only symbols with names contained in `symbols_to_find` are contained in the map. +/// +/// Only symbols with names contained in `symbols_to_find` are contained in the +/// map. +/// +/// This is O(|symbols_to_find| x |extern_symbols|), prefer +/// [`get_symbol_map_fast`] if speed matters. pub fn get_symbol_map<'a>( project: &'a Project, symbols_to_find: &[String], @@ -69,6 +74,33 @@ pub fn get_symbol_map<'a>( tid_map } +/// Get a map from TIDs to the corresponding extern symbol struct. +/// +/// Only symbols with names contained in `symbols_to_find` are contained in the +/// map. +/// +/// More efficient than [`get_symbol_map`], prefer this if `symbols_to_find` is +/// huge since this is O(|extern_symbols|) and not +/// O(|symbols_to_find|x|extern_symbols|). +pub fn get_symbol_map_fast<'a>( + project: &'a Project, + symbols_to_find: &HashSet, +) -> HashMap { + project + .program + .term + .extern_symbols + .iter() + .filter_map(|(_tid, symbol)| { + if symbols_to_find.contains(&symbol.name) { + Some((symbol.tid.clone(), symbol)) + } else { + None + } + }) + .collect() +} + /// Find calls to TIDs contained as keys in the given symbol map. /// For each match return the block containing the call, /// the jump term representing the call itself and the symbol corresponding to the TID from the symbol map. From 437a74ad808aa38b33b182f591147b9688d05299 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Mon, 25 Mar 2024 20:24:48 +0100 Subject: [PATCH 05/26] config/cwe252: initial list of checked symbols Seed the check for CWE252 with a list of all libc functions that are annotated with the compiler attribute `warn_unused_result` in glibc. Does not include functions that indicate a failure by returning a NULL pointer since those are handled in the check for CWE476. Signed-off-by: Valentin Obst --- src/config.json | 140 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 139 insertions(+), 1 deletion(-) diff --git a/src/config.json b/src/config.json index 28a756cc..afb2b3aa 100644 --- a/src/config.json +++ b/src/config.json @@ -74,8 +74,146 @@ "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": [ - "fgets" + "__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": { From 517280faa5aa30ea42ee56ea7cf9f826bc9d3c4c Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Mon, 25 Mar 2024 21:34:12 +0100 Subject: [PATCH 06/26] lib/checkers/cwe252: add support for LKMs Enables the CWE252 check for LKMs and seeds it with all functions in the module API that are annotated with the `warn_unused_result` compiler attribute. Signed-off-by: Valentin Obst --- src/cwe_checker_lib/src/checkers.rs | 5 +- src/lkm_config.json | 571 ++++++++++++++++++++++++++++ 2 files changed, 574 insertions(+), 2 deletions(-) diff --git a/src/cwe_checker_lib/src/checkers.rs b/src/cwe_checker_lib/src/checkers.rs index 332a78b3..2bcb57f1 100644 --- a/src/cwe_checker_lib/src/checkers.rs +++ b/src/cwe_checker_lib/src/checkers.rs @@ -6,8 +6,9 @@ //! See there for detailed information about this check. /// Checkers that are supported for Linux kernel modules. -pub const MODULES_LKM: [&str; 9] = [ - "CWE134", "CWE190", "CWE215", "CWE416", "CWE457", "CWE467", "CWE476", "CWE676", "CWE789", +pub const MODULES_LKM: [&str; 10] = [ + "CWE134", "CWE190", "CWE215", "CWE252", "CWE416", "CWE457", "CWE467", "CWE476", "CWE676", + "CWE789", ]; pub mod cwe_119; diff --git a/src/lkm_config.json b/src/lkm_config.json index a223c864..cf12519b 100644 --- a/src/lkm_config.json +++ b/src/lkm_config.json @@ -11,6 +11,577 @@ "CWE215": { "symbols": [] }, + "CWE252": { + "_comment": "All exported functions annotated with warn_unused_result.", + "strict_mode": false, + "strict_symbols": [], + "symbols": [ + "__ab_c_size", + "action", + "add_bind_files", + "add_mtd_device", + "aesgcm_decrypt", + "anybuss_host_common_probe", + "aq_check_approve_fl2", + "aq_check_approve_fvlan", + "aq_check_filter", + "aq_check_rule", + "aq_match_filter", + "aq_rule_already_exists", + "aq_rule_is_approve", + "aq_rule_is_not_correct", + "aq_rule_is_not_support", + "__arch_clear_user", + "__arch_copy_from_user", + "__arch_copy_to_user", + "arch_get_random_longs", + "arch_get_random_seed_longs", + "arch_phys_wc_add", + "arch_phys_wc_index", + "arm_clear_user", + "arm_copy_from_user", + "arm_copy_to_user", + "asix_read_cmd", + "__asm_copy_from_user", + "__asm_copy_to_user", + "ata_pci_device_do_resume", + "__attribute__", + "attribute_container_unregister", + "ax25_create_cb", + "ax25_listen_register", + "ax25_rt_add", + "ax25_uid_ioctl", + "__bch2_btree_iter_traverse", + "bch2_btree_iter_traverse", + "bch2_btree_node_lock_write", + "bch2_btree_path_make_mut", + "__bch2_btree_path_set_pos", + "bch2_btree_path_set_pos", + "bch2_btree_path_traverse", + "bch2_btree_path_traverse_one", + "bch2_trans_update", + "bch2_trans_update_buffered", + "bch2_trans_update_by_path", + "bch2_write_inode", + "bch2_write_inode_size", + "bio_add_folio", + "bio_add_page", + "blk_get_queue", + "bsg_job_get", + "btree_init", + "btree_insert", + "btree_node_lock_nopath", + "bus_create_file", + "bus_register", + "bus_rescan_devices", + "bus_rescan_devices_helper", + "can_get_echo_skb", + "__cdx_driver_register", + "cfg80211_inform_bss", + "cfg80211_inform_bss_frame", + "chacha20poly1305_decrypt", + "check_copy_size", + "check_zeroed_user", + "class_create", + "class_create_file", + "class_create_file_ns", + "class_interface_register", + "class_register", + "__clear_user", + "clear_user", + "__clear_user_std", + "clk_bulk_enable", + "clk_bulk_get", + "clk_bulk_get_all", + "clk_bulk_get_optional", + "clk_bulk_prepare", + "clk_bulk_prepare_enable", + "clk_hw_register", + "copy_from_iter", + "copy_from_iter_full", + "copy_from_iter_full_nocache", + "copy_from_iter_nocache", + "__copy_from_user", + "_copy_from_user", + "copy_from_user", + "__copy_from_user_inatomic", + "__copy_from_user_inatomic_nocache", + "_copy_from_user_key", + "copy_from_user_key", + "copy_mc_fragile", + "copy_mc_generic", + "copy_mc_to_kernel", + "copy_mc_to_user", + "copy_nofault", + "__copy_tofrom_user", + "copy_to_iter", + "__copy_to_user", + "_copy_to_user", + "copy_to_user", + "_copy_to_user_key", + "copy_to_user_key", + "__copy_to_user_std", + "__copy_user_flushcache", + "copy_user_generic", + "csr_seed_long", + "current_clr_polling_and_test", + "current_set_polling_and_test", + "curve25519", + "__deliver_ckc", + "__deliver_cpu_timer", + "__deliver_emergency_signal", + "__deliver_external_call", + "__deliver_io", + "__deliver_machine_check", + "__deliver_pfault_done", + "__deliver_pfault_init", + "__deliver_prog", + "__deliver_restart", + "__deliver_service", + "__deliver_service_ev", + "__deliver_set_prefix", + "__deliver_virtio", + "destroy_hw_fd_uobject", + "destroy_hw_idr_uobject", + "device_add", + "device_add_disk", + "device_add_group", + "device_add_groups", + "device_attach", + "device_bind_driver", + "device_create_bin_file", + "device_driver_attach", + "device_register", + "device_reprobe", + "device_reset", + "devm_anybuss_host_common_probe", + "devm_clk_bulk_get", + "devm_clk_bulk_get_all", + "devm_clk_bulk_get_optional", + "devm_clk_hw_register", + "devm_device_add_group", + "devm_device_add_groups", + "devm_pinctrl_get", + "devm_pinctrl_get_select", + "devm_pinctrl_get_select_default", + "devm_regulator_bulk_get", + "devm_regulator_bulk_get_const", + "devm_regulator_bulk_get_exclusive", + "devm_request_any_context_irq", + "devm_request_irq", + "devm_request_threaded_irq", + "__devm_uio_register_device", + "devres_open_group", + "__die", + "dm_set_target_max_io_len", + "do_mlock", + "do_notify_parent", + "down_interruptible", + "down_killable", + "down_read_interruptible", + "down_read_killable", + "down_read_killable_nested", + "down_timeout", + "down_trylock", + "down_write_killable", + "driver_attach", + "driver_create_file", + "driver_for_each_device", + "driver_register", + "drm_atomic_add_affected_connectors", + "drm_atomic_add_affected_planes", + "drm_atomic_add_encoder_bridges", + "drm_atomic_check_only", + "drm_atomic_commit", + "drm_atomic_get_crtc_state", + "drm_atomic_get_private_obj_state", + "drm_atomic_helper_swap_state", + "drm_atomic_nonblocking_commit", + "drm_atomic_set_crtc_for_connector", + "drm_atomic_set_crtc_for_plane", + "drm_atomic_set_mode_for_crtc", + "drm_atomic_set_mode_prop_for_crtc", + "drm_atomic_state_alloc", + "drm_atomic_state_init", + "drm_dp_atomic_find_time_slots", + "drm_dp_atomic_release_time_slots", + "drm_dp_mst_add_affected_dsc_crtcs", + "drm_dp_mst_atomic_check", + "drm_dp_mst_atomic_check_mgr", + "drm_dp_mst_atomic_setup_commit", + "drm_dp_mst_root_conn_atomic_check", + "drm_dp_mst_topology_mgr_resume", + "__drmm_add_action", + "__drmm_add_action_or_reset", + "drmm_mode_config_init", + "drm_modeset_lock_single_interruptible", + "dvb_usb_generic_rw", + "dvb_usb_generic_write", + "ERR_PTR", + "ffs_do_descs", + "ffs_do_single_desc", + "ffs_epfiles_create", + "fh_fill_both_attrs", + "fh_fill_pre_attrs", + "fieldbus_dev_register", + "file_check_and_advance_wb_err", + "file_fdatawait_range", + "file_write_and_wait_range", + "fsl_create_mc_io", + "fsl_mc_allocate_irqs", + "fsl_mc_device_add", + "__fsl_mc_driver_register", + "fsl_mc_is_allocatable", + "fsl_mc_object_allocate", + "fsl_mc_portal_allocate", + "fsl_mc_resource_allocate", + "__gameport_register_driver", + "guc_mcr_reg_add", + "guc_mmio_reg_add", + "hex2bin", + "hid_hw_open", + "hid_hw_start", + "__hid_register_driver", + "HYPERVISOR_hvm_op", + "i915_active_fence_set", + "i915_gem_evict_for_node", + "i915_gem_evict_something", + "i915_gem_gtt_prepare_pages", + "i915_gem_init", + "i915_gem_object_ggtt_pin", + "i915_gem_object_ggtt_pin_ww", + "i915_gem_object_pin_pages", + "i915_gem_object_set_to_cpu_domain", + "i915_gem_object_set_to_gtt_domain", + "i915_gem_object_set_to_wc_domain", + "i915_gem_ww_ctx_backoff", + "__i915_request_create", + "_i915_vma_move_to_active", + "i915_vma_pin", + "i915_vma_pin_fence", + "i915_vma_pin_ww", + "i915_vma_unbind", + "i915_vma_unbind_async", + "i915_vma_unbind_unlocked", + "i915_vm_lock_objects", + "idr_alloc_u32", + "__idxd_driver_register", + "ieee80211_link_change_bandwidth", + "ieee80211_link_reserve_chanctx", + "ieee80211_link_use_channel", + "ieee80211_link_use_reserved_context", + "igt_vma_move_to_active_unlocked", + "il4965_request_firmware", + "il_send_cmd_pdu", + "il_send_cmd_sync", + "input_allocate_device", + "input_get_new_minor", + "input_register_device", + "input_register_handler", + "intel_gt_init_hw", + "intel_gt_reset_lock_interruptible", + "intel_gt_reset_trylock", + "io_schedule_prepare", + "IS_ERR", + "IS_ERR_OR_NULL", + "ivpu_bo_pin", + "ivpu_rpm_get", + "ivpu_rpm_get_if_active", + "iwl_mvm_send_cmd", + "iwl_mvm_send_cmd_pdu", + "iwl_mvm_send_cmd_pdu_status", + "iwl_mvm_send_cmd_status", + "iwl_op_mode_hw_rf_kill", + "iwl_pci_register_driver", + "iwl_scan_initiate", + "__kasan_init_slab_obj", + "__kasan_kmalloc", + "__kasan_kmalloc_large", + "__kasan_krealloc", + "__kasan_slab_alloc", + "kfence_handle_page_fault", + "__kfifo_int_must_check_helper", + "__kfifo_uint_must_check_helper", + "kmsan_ioremap_page_range", + "kmsan_memblock_free_pages", + "kmsan_vmap_pages_range_noflush", + "kobject_add", + "kobject_create_and_add", + "kobject_get_unless_zero", + "kobject_init_and_add", + "kobject_move", + "kobject_rename", + "krealloc", + "kset_register", + "kstrtobool", + "kstrtobool_from_user", + "kstrtoint", + "kstrtoint_from_user", + "_kstrtol", + "kstrtol_from_user", + "kstrtoll", + "kstrtoll_from_user", + "kstrtos16", + "kstrtos16_from_user", + "kstrtos32", + "kstrtos32_from_user", + "kstrtos64", + "kstrtos64_from_user", + "kstrtos8", + "kstrtos8_from_user", + "kstrtou16", + "kstrtou16_from_user", + "kstrtou32", + "kstrtou32_from_user", + "kstrtou64", + "kstrtou64_from_user", + "kstrtou8", + "kstrtou8_from_user", + "kstrtouint", + "kstrtouint_from_user", + "_kstrtoul", + "kstrtoul_from_user", + "kstrtoull", + "kstrtoull_from_user", + "kvm_s390_deliver_pending_interrupts", + "kvm_s390_inject_vcpu", + "kvm_s390_inject_vm", + "lclear_user", + "may_use_simd", + "__mcb_register_driver", + "mddev_lock", + "md_flush_request", + "__media_device_register", + "media_device_register_entity", + "media_devnode_register", + "media_request_lock_for_access", + "media_request_lock_for_update", + "mhi_ep_check_mhi_state", + "mhi_poll_reg_field", + "mhi_read_reg", + "mhi_read_reg_field", + "mhi_tryset_pm_state", + "mhi_write_reg_field", + "mix_dh", + "mix_precomputed_dh", + "__mmio_reg_add", + "__must_check_fn", + "mutex_lock_interruptible", + "mutex_lock_interruptible_nested", + "mutex_lock_killable", + "mutex_lock_killable_nested", + "mux_control_select", + "mux_control_select_delay", + "mux_control_try_select", + "mux_control_try_select_delay", + "mux_state_select", + "mux_state_select_delay", + "mux_state_try_select", + "mux_state_try_select_delay", + "__nd_driver_register", + "nd_jump_link", + "npe_debug_instr", + "npe_logical_reg_write16", + "npe_logical_reg_write32", + "npe_logical_reg_write8", + "nr_set_mac_address", + "nvhe_check_data_corruption", + "nvif_chan_wait", + "object_type_to_pool_type", + "of_clk_bulk_get", + "of_clk_bulk_get_all", + "of_clk_hw_register", + "of_icc_bulk_get", + "of_regulator_bulk_get_all", + "omap_get_global_state", + "__parport_register_driver", + "pci_assign_resource", + "pcibios_enable_device", + "pci_bus_alloc_resource", + "pci_create_sysfs_dev_files", + "pci_enable_device", + "pci_enable_device_io", + "pci_enable_device_mem", + "pci_map_rom", + "pcim_enable_device", + "pcim_set_mwi", + "pci_reassign_resource", + "pci_reenable_device", + "__pci_register_driver", + "pci_request_config_region_exclusive", + "pci_request_region", + "pci_request_regions", + "pci_request_regions_exclusive", + "pci_resize_resource", + "pci_set_mwi", + "pcmcia_request_irq", + "percpu_ref_init", + "pinctrl_get", + "pinctrl_get_select", + "pinctrl_get_select_default", + "pinctrl_lookup_state", + "prom_getproperty", + "PTR_ERR", + "PTR_ERR_OR_ZERO", + "PUSH_WAIT", + "raw_copy_from_user", + "raw_copy_in_user", + "raw_copy_to_user", + "rcuref_get_slowpath", + "rcuref_put_slowpath", + "rdma_restrack_get", + "rdrand_long", + "rdseed_long", + "__realloc_size", + "__refcount_add_not_zero", + "refcount_dec_and_lock", + "refcount_dec_and_lock_irqsave", + "refcount_dec_and_mutex_lock", + "__refcount_dec_and_test", + "refcount_dec_if_one", + "refcount_dec_not_one", + "__refcount_inc_not_zero", + "__refcount_sub_and_test", + "regulator_bulk_enable", + "regulator_bulk_get", + "regulator_enable", + "reiserfs_write_unlock_nested", + "__releases", + "remove_arg_zero", + "request_any_context_irq", + "request_nmi", + "__request_percpu_irq", + "request_percpu_irq", + "request_percpu_nmi", + "request_threaded_irq", + "rfkill_alloc", + "rfkill_register", + "RING_SPACE", + "rmi_register_function", + "__rmi_register_function_handler", + "rose_add_loopback_node", + "scmi_devres_protocol_instance_get", + "scmi_voltage_info_get", + "scsi_add_host", + "scsi_add_host_with_dma", + "scsi_device_get", + "scsi_device_reprobe", + "__serio_register_driver", + "set_fd_set", + "shpchp_create_ctrl_files", + "__skb_header_pointer", + "skb_header_pointer", + "skb_pointer_if_linear", + "skb_set_owner_sk_safe", + "skb_to_sgvec", + "skb_to_sgvec_nomark", + "skip_spaces", + "__smsc75xx_read_reg", + "smsc75xx_read_reg", + "smsc75xx_read_reg_nopm", + "__smsc75xx_write_reg", + "smsc75xx_write_reg", + "smsc75xx_write_reg_nopm", + "smsc95xx_eeprom_confirm_not_busy", + "smsc95xx_read_reg", + "smsc95xx_wait_eeprom", + "smsc95xx_write_reg", + "smsc95xx_write_reg_async", + "__snd_seq_driver_register", + "spu_acquire_saved", + "ssc_request", + "stack_depot_set_extra_bits", + "__sta_info_destroy", + "__sta_info_destroy_part1", + "strncpy_from_user", + "strnlen_user", + "strstrip", + "sysfs_chmod_file", + "sysfs_create_bin_file", + "sysfs_create_dir_ns", + "sysfs_create_file", + "sysfs_create_file_ns", + "sysfs_create_files", + "sysfs_create_group", + "sysfs_create_groups", + "sysfs_create_link", + "sysfs_create_link_nowarn", + "sysfs_create_mount_point", + "sysfs_init", + "sysfs_move_dir_ns", + "sysfs_rename_dir_ns", + "sysfs_update_groups", + "tcp_queue_rcv", + "tpm_try_get_ops", + "try_get_page", + "try_grab_page", + "tty_ldisc_init", + "typeof", + "__uio_register_device", + "unclaimed_reg_debug_header", + "usb_find_bulk_in_endpoint", + "usb_find_bulk_out_endpoint", + "usb_find_common_endpoints", + "usb_find_common_endpoints_reverse", + "usb_find_int_in_endpoint", + "usb_find_int_out_endpoint", + "usb_find_last_bulk_in_endpoint", + "usb_find_last_bulk_out_endpoint", + "usb_find_last_int_in_endpoint", + "usb_find_last_int_out_endpoint", + "user_access_begin", + "user_event_last_ref", + "user_read_access_begin", + "user_write_access_begin", + "uuid_is_valid", + "v4l2_async_register_subdev_sensor", + "v4l2_device_register", + "v4l2_device_register_subdev", + "__v4l2_device_register_subdev_nodes", + "vb2_queue_init", + "vb2_queue_init_name", + "__video_register_device", + "__vmap_pages_range_noflush", + "vmap_pages_range_noflush", + "vm_brk_flags", + "__vmbus_driver_register", + "vm_mmap", + "vm_mmap_pgoff", + "wl12xx_sdio_raw_read", + "wl12xx_sdio_raw_write", + "wl12xx_spi_raw_read", + "wl12xx_spi_raw_write", + "wl12xx_top_reg_read", + "wl12xx_top_reg_write", + "wl18xx_top_reg_read", + "wl18xx_top_reg_write", + "wlcore_raw_read", + "wlcore_raw_read32", + "wlcore_raw_read_data", + "wlcore_raw_write32", + "wlcore_raw_write_data", + "wlcore_read", + "wlcore_read32", + "wlcore_read_data", + "wlcore_read_hwaddr", + "wlcore_read_reg", + "wlcore_write", + "wlcore_write32", + "wlcore_write_data", + "wlcore_write_reg", + "__wmi_driver_register", + "ww_mutex_lock", + "ww_mutex_lock_interruptible", + "ww_mutex_trylock", + "__xa_alloc", + "__xa_alloc_cyclic", + "__xa_insert", + "xchacha20poly1305_decrypt", + "__xenbus_register_backend", + "__xenbus_register_frontend", + "zswap_pool_get" + ] + }, "CWE416": { "_comment": "Functions that invalidate the pointer passed as the first argument.", "deallocation_symbols": [], From 772d02de99dd94198ddcc1aeb209c20ed54623d8 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Tue, 19 Mar 2024 16:03:24 +0100 Subject: [PATCH 07/26] lib/utils/debug: introduce `ToJsonCompact` trait Many types implement a custom JSON serialization method for internal debugging purposes. Add an abstraction for this pattern in form of the `ToJsonCompact` trait. This enables all types to benefit from the default implementation of a printing method and makes it easier to use generic programming. This commit does not convert any existing types that implement this behavior. They are expected to be converted in an ongoing process. Signed-off-by: Valentin Obst --- .../src/abstract_domain/mem_region.rs | 14 ++++++++++ .../interprocedural_fixpoint_generic.rs | 2 +- src/cwe_checker_lib/src/analysis/taint/mod.rs | 7 +++++ .../src/analysis/taint/state.rs | 26 +++++++++++++++++++ src/cwe_checker_lib/src/checkers/cwe_252.rs | 2 +- .../src/checkers/cwe_252/context.rs | 17 ++++++++++++ src/cwe_checker_lib/src/utils/debug.rs | 21 +++++++++++++++ src/cwe_checker_lib/src/utils/mod.rs | 1 + 8 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 src/cwe_checker_lib/src/utils/debug.rs diff --git a/src/cwe_checker_lib/src/abstract_domain/mem_region.rs b/src/cwe_checker_lib/src/abstract_domain/mem_region.rs index 32691b11..94238394 100644 --- a/src/cwe_checker_lib/src/abstract_domain/mem_region.rs +++ b/src/cwe_checker_lib/src/abstract_domain/mem_region.rs @@ -1,6 +1,7 @@ use super::{AbstractDomain, HasTop, SizedDomain}; use crate::intermediate_representation::ByteSize; use crate::prelude::*; +use crate::utils::debug::ToJsonCompact; use apint::{Int, Width}; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; @@ -66,6 +67,19 @@ impl HasTop for MemR } } +impl ToJsonCompact for MemRegion +where + T: ToJsonCompact + AbstractDomain + SizedDomain + HasTop + std::fmt::Debug, +{ + fn to_json_compact(&self) -> serde_json::Value { + serde_json::Value::Object( + self.iter() + .map(|(offset, val)| (offset.to_string(), val.to_json_compact())) + .collect(), + ) + } +} + impl MemRegion { /// Create a new, empty memory region. pub fn new(address_bytesize: ByteSize) -> MemRegion { diff --git a/src/cwe_checker_lib/src/analysis/interprocedural_fixpoint_generic.rs b/src/cwe_checker_lib/src/analysis/interprocedural_fixpoint_generic.rs index 99e1a5fa..4fb913fe 100644 --- a/src/cwe_checker_lib/src/analysis/interprocedural_fixpoint_generic.rs +++ b/src/cwe_checker_lib/src/analysis/interprocedural_fixpoint_generic.rs @@ -14,7 +14,7 @@ use crate::prelude::*; /// The interprocedural_flow value will either be transferred from the end of the called subroutine /// to the return site in case of a forward analysis or from the beginning of the called subroutine /// to the callsite in a backward analysis. -#[derive(PartialEq, Eq, Serialize, Deserialize, Clone)] +#[derive(PartialEq, Eq, Serialize, Deserialize, Clone, Debug)] pub enum NodeValue { /// A single abstract value Value(T), diff --git a/src/cwe_checker_lib/src/analysis/taint/mod.rs b/src/cwe_checker_lib/src/analysis/taint/mod.rs index 346c8940..f6ed16b7 100644 --- a/src/cwe_checker_lib/src/analysis/taint/mod.rs +++ b/src/cwe_checker_lib/src/analysis/taint/mod.rs @@ -20,6 +20,7 @@ use crate::analysis::{ }; use crate::intermediate_representation::*; use crate::prelude::*; +use crate::utils::debug::ToJsonCompact; use std::convert::AsRef; use std::fmt::Display; @@ -593,6 +594,12 @@ impl RegisterDomain for Taint { } } +impl ToJsonCompact for Taint { + fn to_json_compact(&self) -> serde_json::Value { + serde_json::json!(self.to_string()) + } +} + impl Taint { /// Checks whether the given value is in fact tainted. pub fn is_tainted(&self) -> bool { diff --git a/src/cwe_checker_lib/src/analysis/taint/state.rs b/src/cwe_checker_lib/src/analysis/taint/state.rs index d936773f..583ae67a 100644 --- a/src/cwe_checker_lib/src/analysis/taint/state.rs +++ b/src/cwe_checker_lib/src/analysis/taint/state.rs @@ -12,6 +12,7 @@ use crate::analysis::pointer_inference::Data as PiData; use crate::analysis::vsa_results::VsaResult; use crate::intermediate_representation::*; use crate::prelude::*; +use crate::utils::debug::ToJsonCompact; use std::collections::HashMap; @@ -30,6 +31,31 @@ pub struct State { memory_taint: HashMap>, } +impl ToJsonCompact for State { + fn to_json_compact(&self) -> serde_json::Value { + let mut state_map = serde_json::Map::new(); + + let register_taint = self + .register_taint + .iter() + .map(|(reg, taint)| (reg.name.clone(), taint.to_json_compact())) + .collect(); + let register_taint = serde_json::Value::Object(register_taint); + + let memory_taint = self + .memory_taint + .iter() + .map(|(mem_id, mem_region)| (mem_id.to_string(), mem_region.to_json_compact())) + .collect(); + let memory_taint = serde_json::Value::Object(memory_taint); + + state_map.insert("registers".into(), register_taint); + state_map.insert("memory".into(), memory_taint); + + serde_json::Value::Object(state_map) + } +} + impl PartialEq for State { /// Two states are equal if the same values are tainted in both states. fn eq(&self, other: &Self) -> bool { diff --git a/src/cwe_checker_lib/src/checkers/cwe_252.rs b/src/cwe_checker_lib/src/checkers/cwe_252.rs index 7a08f8e8..eaab9b26 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252.rs @@ -66,7 +66,7 @@ use crate::intermediate_representation::{ExternSymbol, Jmp, Project, Term}; use crate::pipeline::AnalysisResults; use crate::prelude::*; use crate::utils::log::{CweWarning, LogMessage}; -use crate::utils::{symbol_utils, ToJsonCompact}; +use crate::utils::symbol_utils; use crate::CweModule; use petgraph::visit::EdgeRef; diff --git a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs index ca497db9..ad7fee2c 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs @@ -25,6 +25,23 @@ pub type FpComputation<'a, 'b> = fixpoint::Computation< forward_interprocedural_fixpoint::GeneralizedContext<'a, TaCompCtx<'a, 'b>>, >; +impl ToJsonCompact for FpComputation<'_, '_> { + fn to_json_compact(&self) -> serde_json::Value { + let graph = self.get_graph(); + let mut json_nodes = serde_json::Map::new(); + + for (node_index, node_value) in self.node_values().iter() { + let node = graph.node_weight(*node_index).unwrap(); + + if let NodeValue::Value(value) = node_value { + json_nodes.insert(format!("{node}"), value.to_json_compact()); + } + } + + serde_json::Value::Object(json_nodes) + } +} + /// Type that represents the definition of the taint analysis. /// /// Values of this type represent the taint analysis for a particular call to an diff --git a/src/cwe_checker_lib/src/utils/debug.rs b/src/cwe_checker_lib/src/utils/debug.rs new file mode 100644 index 00000000..10b59658 --- /dev/null +++ b/src/cwe_checker_lib/src/utils/debug.rs @@ -0,0 +1,21 @@ +//! Little helpers for developers that try to understand what their code is +//! doing. + +/// Central utility for debug printing in the `cwe_checker`. +/// +/// The canonical way to do printf-debugging in `cwe_checker` development is to +/// implement this trait for the type you want to inspect and then print it +/// via `value.print_compact_json()`. +pub trait ToJsonCompact { + /// Returns a json representation of values of type `self` that is + /// suitable for debugging purposes. + /// + /// The idea is that printing of complex types is facilitated by + /// implementing `to_json_compact` for all of their constituent parts. + fn to_json_compact(&self) -> serde_json::Value; + + /// Print values of type `Self` for debugging purposes. + fn print_compact_json(&self) { + println!("{:#}", self.to_json_compact()) + } +} diff --git a/src/cwe_checker_lib/src/utils/mod.rs b/src/cwe_checker_lib/src/utils/mod.rs index 38fccc04..93beae26 100644 --- a/src/cwe_checker_lib/src/utils/mod.rs +++ b/src/cwe_checker_lib/src/utils/mod.rs @@ -2,6 +2,7 @@ pub mod arguments; pub mod binary; +pub mod debug; pub mod ghidra; pub mod graph_utils; pub mod log; From 20311b1c690f19029c64024071ebee1da4d61685 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Thu, 28 Mar 2024 16:33:24 +0100 Subject: [PATCH 08/26] lib/analysis/vsa: add `get_call_renaming_map` to `VsaResult` Add a method to obtain the information how to translate abstract identifiers from the callee to the caller context given the result of a VSA. Signed-off-by: Valentin Obst --- .../analysis/pointer_inference/vsa_result_impl.rs | 7 +++++++ src/cwe_checker_lib/src/analysis/vsa_results/mod.rs | 13 ++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/cwe_checker_lib/src/analysis/pointer_inference/vsa_result_impl.rs b/src/cwe_checker_lib/src/analysis/pointer_inference/vsa_result_impl.rs index 7193c360..6b20859a 100644 --- a/src/cwe_checker_lib/src/analysis/pointer_inference/vsa_result_impl.rs +++ b/src/cwe_checker_lib/src/analysis/pointer_inference/vsa_result_impl.rs @@ -50,4 +50,11 @@ impl<'a> VsaResult for PointerInference<'a> { None } } + + fn get_call_renaming_map( + &self, + call: &Tid, + ) -> Option<&BTreeMap> { + self.id_renaming_maps_at_calls.get(call) + } } diff --git a/src/cwe_checker_lib/src/analysis/vsa_results/mod.rs b/src/cwe_checker_lib/src/analysis/vsa_results/mod.rs index d8b8b81a..18b029df 100644 --- a/src/cwe_checker_lib/src/analysis/vsa_results/mod.rs +++ b/src/cwe_checker_lib/src/analysis/vsa_results/mod.rs @@ -1,11 +1,13 @@ //! This module provides the [`VsaResult`] trait //! which defines an interface for the results of analyses similar to a value set analysis. -use crate::abstract_domain::AbstractLocation; +use crate::abstract_domain::{AbstractIdentifier, AbstractLocation}; use crate::analysis::graph::NodeIndex; use crate::intermediate_representation::{Arg, Expression}; use crate::prelude::*; +use std::collections::BTreeMap; + /// Trait for types that provide access to the result of a value set analysis. /// /// The generic type parameter can be used to implement this trait multiple @@ -59,4 +61,13 @@ pub trait VsaResult { /// Evaluate the given expression at the given node of the graph that the /// value set analysis was computed on. fn eval_at_node(&self, node: NodeIndex, expression: &Expression) -> Option; + + /// Returns the mapping of abstract identfiers in the callee to values in + /// the caller for the given call. + fn get_call_renaming_map( + &self, + _call: &Tid, + ) -> Option<&BTreeMap> { + None + } } From b450b01e93cdb372bc02bf8235f8bbaf5d2ead0b Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Thu, 28 Mar 2024 16:36:27 +0100 Subject: [PATCH 09/26] lib/analysis/taint: propagate memory taint Remove the limitation that `update_return_callee` can not be used to propagate memory taint from the callee to the caller. Do so by renaming abstract identifiers in the default implementation of `update_return` in the `TaintAnalysis` trait. In particular, implementers of `update_return_callee` do not have to case about renaming and can return the abstract identifiers of the callee context. Adjust CWE252 to make use of this new feature. Signed-off-by: Valentin Obst --- src/cwe_checker_lib/src/analysis/taint/mod.rs | 57 ++--- .../src/analysis/taint/state.rs | 206 +++++++++++++++--- src/cwe_checker_lib/src/checkers/cwe_252.rs | 2 + .../src/checkers/cwe_252/context.rs | 120 +++++----- .../src/checkers/cwe_252/isolated_returns.rs | 7 +- 5 files changed, 267 insertions(+), 125 deletions(-) diff --git a/src/cwe_checker_lib/src/analysis/taint/mod.rs b/src/cwe_checker_lib/src/analysis/taint/mod.rs index f6ed16b7..154ed97a 100644 --- a/src/cwe_checker_lib/src/analysis/taint/mod.rs +++ b/src/cwe_checker_lib/src/analysis/taint/mod.rs @@ -190,8 +190,8 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef /// the table in [`update_return`]. The `state` parameter corresponds to /// the taint state at the return sites of the called subroutine. /// - /// By implementing this method you can perform a **limited** - /// interprocedural taint analysis: + /// By implementing this method you can perform an interprocedural taint + /// analysis: /// - If you return `Some(state)` you may influence the taint /// state in the caller (see the documentation of [`update_return`] for /// more information), by having it be [`merged`] into the state coming @@ -204,17 +204,6 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef /// [`update_return`]: TaintAnalysis::update_return /// [`merged`]: State::merge /// - /// # Limitations - /// - /// The interprocedural analysis capabilities are limited since it is - /// forbidden to propagate memory taint. Memory objects from the callee are - /// meaningless in the caller and thus any state returned by this method - /// **MUST** have an empty memory taint, i.e., `State::has_memory_taint` - /// must evaluate to `false`. Future work may lift this requirement. - // FIXME: Add translation of memory objects to default implementation. - /// - /// It is possible to propagate taint in registers. - /// /// # Default /// /// Returns an empty state, i.e., information is propagated through the call @@ -263,23 +252,21 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef (Some(state_before_call), Some(state_before_return)) => { let state_from_caller = self.update_call_generic(state_before_call, &call_term.tid, calling_convention); - let state_from_callee = self - .update_return_callee( - state_before_return, - call_term, - return_term, - calling_convention, - ) - .inspect(|s| { - assert!( - !s.has_memory_taint(), - "TA: BUG: `update_return_callee` returned memory taint." - ) - }); + let state_from_callee = self.update_return_callee( + state_before_return, + call_term, + return_term, + calling_convention, + ); match (state_from_caller, state_from_callee) { - (Some(state_caller), Some(state_callee)) => { - Some(state_caller.merge(&state_callee)) + (Some(mut state_caller), Some(state_callee)) => { + state_caller.merge_with_renaming( + &state_callee, + self.vsa_result().get_call_renaming_map(&call_term.tid), + ); + + Some(state_caller) } // If one implementation indicated that no information // should be propagated by returning `None` we ignore what @@ -297,11 +284,15 @@ pub trait TaintAnalysis<'a>: HasCfg<'a> + HasVsaResult + AsRef return_term, calling_convention, ) - .inspect(|s| { - assert!( - !s.has_memory_taint(), - "TA: BUG: `update_return_callee` returned memory taint." - ) + .map(|state_callee| { + let mut dummy_caller_state = State::new_empty(); + + dummy_caller_state.merge_with_renaming( + &state_callee, + self.vsa_result().get_call_renaming_map(&call_term.tid), + ); + + dummy_caller_state }), _ => None, }; diff --git a/src/cwe_checker_lib/src/analysis/taint/state.rs b/src/cwe_checker_lib/src/analysis/taint/state.rs index 583ae67a..7ac728c0 100644 --- a/src/cwe_checker_lib/src/analysis/taint/state.rs +++ b/src/cwe_checker_lib/src/analysis/taint/state.rs @@ -14,21 +14,36 @@ use crate::intermediate_representation::*; use crate::prelude::*; use crate::utils::debug::ToJsonCompact; -use std::collections::HashMap; +use std::collections::{hash_map, BTreeMap, HashMap}; use super::Taint; #[cfg(test)] mod tests; +// FIXME: Turn those type aliases into proper New Types that are +// `AbstractDomain`s. +// +// Background: Those types are in fact representing complete lattices +// (space of total functionsfrom some set to complete lattice) and `State` is +// just their cartesian product. Having this explicit in the type system would +// make the code in this module a lot cleaner. (When doing this one should also +// fix the memory merging such that this one actually IS a complete lattice...). +/// Represents our knowledge about taint in registers at a particular point in +/// the program. +pub type RegisterTaint = HashMap; +/// Represents our knowledge about taint in memory at a particular point in the +/// program. +pub type MemoryTaint = HashMap>; + /// The state object of the taint analysis representing all known tainted memory /// and register values at a certain location within the program. #[derive(Serialize, Deserialize, Debug, Eq, Clone)] pub struct State { /// The set of currently tainted registers. - register_taint: HashMap, + register_taint: RegisterTaint, /// The Taint contained in memory objects - memory_taint: HashMap>, + memory_taint: MemoryTaint, } impl ToJsonCompact for State { @@ -68,41 +83,16 @@ impl AbstractDomain for State { /// /// Any value tainted in at least one input state is also tainted in the /// merged state. - // FIXME: The used algorithm for merging the taints contained in memory - // regions is unsound when merging taints that intersect only partially. - // If, for example, in state A `RSP[0:3]` is tainted and in state B - // `RSP[2:3]` is tainted, A u B will only report `RSP[2:3]` as tainted. - // - // For the NULL pointer dereference check, however, this should not have an - // effect in practice, since these values are usually unsound or a sign of - // incorrect behavior of the analysed program. fn merge(&self, other: &Self) -> Self { - let mut register_taint = self.register_taint.clone(); - for (var, other_taint) in other.register_taint.iter() { - if let Some(taint) = self.register_taint.get(var) { - register_taint.insert(var.clone(), taint.merge(other_taint)); - } else { - register_taint.insert(var.clone(), *other_taint); - } - } + let mut new_state = self.clone(); - let mut memory_taint = self.memory_taint.clone(); - for (tid, other_mem_region) in other.memory_taint.iter() { - if let Some(mem_region) = memory_taint.get_mut(tid) { - for (index, taint) in other_mem_region.iter() { - mem_region.insert_at_byte_index(*taint, *index); - // FIXME: Unsound in theory for partially intersecting - // taints. Should not matter in practice. - } - } else { - memory_taint.insert(tid.clone(), other_mem_region.clone()); - } - } + new_state.merge_register_taint(&other.register_taint); - State { - register_taint, - memory_taint, + for (aid, other_memory_object) in other.memory_taint.iter() { + new_state.merge_memory_object_with_offset(aid, other_memory_object, 0); } + + new_state } /// The state has no explicit Top element. @@ -459,6 +449,154 @@ impl State { .flat_map(|(_, mem_region)| mem_region.iter()) .any(|(_, taint)| taint.is_tainted()) } + + /// Merges the given `other` state into this state with renaming of abstract + /// identifiers. + /// + /// The set of valid abstract identfiers (aIDs) is local to a given + /// function. When merging states across function boundaries it is necessary + /// to map aIDs into the set of valid aIDs in the target context before + /// performing the merging. + /// + /// This function assumes that the target context is the one of `self` and + /// that `renaming_map` specifies how valid aIDs in the context of `other` + /// correspond to the aIDs of this context. + pub fn merge_with_renaming( + &mut self, + other: &Self, + renaming_map: Option<&BTreeMap>, + ) { + let Self { + register_taint: other_register_taint, + memory_taint: other_memory_taint, + } = other; + + // Naive merging works for register taint. + self.merge_register_taint(other_register_taint); + + let Some(renaming_map) = renaming_map else { + // Without a renaming rule we can not do anything meaningful with + // the memory objects of the other state, i.e., we are done here. + return; + }; + + for (other_aid, other_memory_object) in other_memory_taint.iter() { + let Some(value) = renaming_map.get(other_aid) else { + // The pointer inference decided that this object is not + // referenced in the context of `self`, so no need to merge it. + continue; + }; + // There is more information in `value` that we could base our + // decision on; however, + // - we decide to ignore whether the `value` may be absolute in the + // context of `self`. This is not important for taint analyses. + // - in cases where it may be some unknown base + offset it is still + // worth handling the bases that we know about. + for (aid, offset_interval) in value.get_relative_values() { + let Ok(offset) = offset_interval.try_to_offset() else { + // The offset of the old memory object into the new one is + // not known exactly. At this point we could merge the old + // object at every possible offset (sound) or not merge at + // all (unsound). + // + // Depending on the analysis it will lead to more FP + // (CWE252) or FN (CWE476); on the upside, we have to track + // less state and are faster. + continue; + }; + + // Starts tracking the object if it does not exist. + self.merge_memory_object_with_offset(aid, other_memory_object, offset); + } + } + } + + /// Merges the given register taint into the state. + /// + /// Equivalent to merging + /// `other_register_taint` x bottom + /// into the state. + fn merge_register_taint(&mut self, other_register_taint: &RegisterTaint) { + use hash_map::Entry::*; + + for (reg, other_taint) in other_register_taint.iter() { + match self.register_taint.entry(reg.clone()) { + Occupied(mut taint) => { + // FIXME: We create a new owned taint even though we could + // modify in-place. + let new_taint = taint.get().merge(other_taint); + + taint.insert(new_taint); + } + Vacant(entry) => { + entry.insert(*other_taint); + } + } + } + } + + /// Merges the given pair of abstract identifier and memory object into the + /// state. + /// + /// Equivalent to merging + /// bottom x (`aid` -> (`other_memory_object` + `offset`)) + /// into the state. + fn merge_memory_object_with_offset( + &mut self, + aid: &AbstractIdentifier, + other_memory_object: &MemRegion, + offset: i64, + ) { + use hash_map::Entry::*; + + match self.memory_taint.entry(aid.clone()) { + Occupied(mut current_memory_object) => { + let current_memory_object = current_memory_object.get_mut(); + + merge_memory_object_with_offset(current_memory_object, other_memory_object, offset); + } + Vacant(entry) => { + let mut new_memory_object = other_memory_object.clone(); + + new_memory_object.add_offset_to_all_indices(offset); + entry.insert(new_memory_object); + } + } + } + + /// Deconstructs a `State` into its register and memory taint maps. + pub fn into_mem_reg_taint(self) -> (RegisterTaint, MemoryTaint) { + (self.register_taint, self.memory_taint) + } + + /// Constructs a `State` from register and memory taint maps. + pub fn from_mem_reg_taint(register_taint: RegisterTaint, memory_taint: MemoryTaint) -> Self { + Self { + register_taint, + memory_taint, + } + } +} + +// FIXME: The used algorithm for merging the taints contained in memory +// regions is unsound when merging taints that intersect only partially. +// If, for example, in state A `RSP[0:3]` is tainted and in state B +// `RSP[2:3]` is tainted, A u B will only report `RSP[2:3]` as tainted. +// +// For the NULL pointer dereference check, however, this should not have an +// effect in practice, since these values are usually unsound or a sign of +// incorrect behavior of the analysed program. +// FIXME: This looks a lot like it should be a method on `MemRegion`. +fn merge_memory_object_with_offset( + mem_object: &mut MemRegion, + other_memory_object: &MemRegion, + offset: i64, +) { + for (index, taint) in other_memory_object.iter() { + mem_object.insert_at_byte_index(*taint, *index + offset); + // FIXME: Unsound in theory for partially intersecting + // taints. Should not matter in practice. + } } impl State { diff --git a/src/cwe_checker_lib/src/checkers/cwe_252.rs b/src/cwe_checker_lib/src/checkers/cwe_252.rs index eaab9b26..82e7a9a2 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252.rs @@ -224,6 +224,8 @@ impl<'a, 'b: 'a> CweAnalysis<'a, 'b> { Vec::new(), self.cwe_collector .try_iter() + // FIXME: It would be nice to preerve all reasons during + // deduplication. .map(|msg| (msg.tids.clone(), msg)) .collect::>() .into_values() diff --git a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs index ad7fee2c..7e62b495 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs @@ -1,7 +1,7 @@ //! Definition of the Taint Analysis for CWE252. //! -//! Implementation of the [`TaintAnalysis`] for this CWE check. See the module -//! documentation for more details on the algorithm and its limitations. +//! Implementation of the [`TaintAnalysis`] trait for this CWE check. See the +//! module documentation for more details on the algorithm and its limitations. use super::MustUseCall; @@ -12,7 +12,8 @@ use crate::analysis::forward_interprocedural_fixpoint::{ use crate::analysis::graph::{Graph as Cfg, HasCfg}; use crate::analysis::interprocedural_fixpoint_generic::NodeValue; use crate::analysis::pointer_inference::{Data as PiData, PointerInference}; -use crate::analysis::taint::{state::State as TaState, TaintAnalysis}; +use crate::analysis::taint::state::{MemoryTaint, RegisterTaint, State as TaState}; +use crate::analysis::taint::TaintAnalysis; use crate::analysis::vsa_results::{HasVsaResult, VsaResult}; use crate::intermediate_representation::{Blk, ExternSymbol, Jmp, Project, Term, Tid}; use crate::utils::debug::ToJsonCompact; @@ -233,19 +234,22 @@ impl<'a> TaintAnalysis<'a> for TaCompCtx<'a, '_> { /// Propagates taint from callee to caller. /// - /// The check performs a limited interprocedural taint analysis. The main + /// The check performs a bottom-up interprocedural taint analysis. The main /// idea is that: *If a function may propagate the return value of a /// fallible function call to its caller without checking it, then the /// function itself becomes "must check".* (In the sense that the caller is /// now responsible for checking its return value.) /// + /// The taint may be returned directly, indirectly by returning a pointer + /// that can be used to reach taint, or written to global variables + /// (possibly as a pointer). + /// /// Limitations stem from two sources: /// - /// - It is not possible to propagate memory taint from the callee to the - /// caller. - /// - The calling convention may be unknown. + /// - The calling convention may be unknown, which means we can not + /// determine precisely which taint will be reachable for the caller. /// - /// The the source code comments for further information. + /// See the the source code comments for further information. /// /// Furthermore, this callback is responsible for detecting cases where /// taint may reach the end of a function without being returned. @@ -255,57 +259,42 @@ impl<'a> TaintAnalysis<'a> for TaCompCtx<'a, '_> { fn update_return_callee( &self, state: &TaState, - _call_term: &Term, + call_term: &Term, return_term: &Term, calling_convention: &Option, ) -> Option { - if !state.has_register_taint() { - // No register taint means that we can not propagate any taint to - // the caller (since we are currently unable to propagate memory - // taint from callee to caller). - // - // We consider the information in the tainted memory to be "lost", - // and thus we generate a CWE warning since its presence implies - // that the return value of a fallible function call may have been - // ignored by the called function. - self.generate_cwe_warning(&return_term.tid, "return_no_reg_taint"); - - // We still want to allow for the propagation of taint from the - // call site. - Some(TaState::new_empty()) - } else if let Some(calling_convention) = self + let (register_taint, memory_taint) = state.clone().into_mem_reg_taint(); + + // Only keep memory taint that will be propagated to the caller. We + // compute this here since we want to notice when no taint is + // propagated. + let renaming_map = self.pi_result.get_call_renaming_map(&call_term.tid); + let propagated_memory_taint: MemoryTaint = memory_taint + .into_iter() + .filter(|(aid, _)| { + // This is still an over-approximation to the taint that will be + // available to the caller since it might happen that all relative + // values have non-exactly-known offsets. + renaming_map.is_some_and(|renaming_map| { + renaming_map + .get(aid) + .is_some_and(|value| value.referenced_ids().next().is_some()) + }) + }) + .collect(); + + let propagated_register_taint: RegisterTaint = if let Some(calling_convention) = self .project .get_specific_calling_convention(calling_convention) { - // We have tainted registers and we know the calling convention: - // - // If there is a non-empty overlap between the tainted registers and - // the return registers we propagate the taint to the caller, - // which makes them responsible for checking it. - // - // Else it means that the called function may have ignored the - // return value of a fallible function call and we emit a warning. - let propagated_state = calling_convention.get_all_return_register().iter().fold( - TaState::new_empty(), - |mut propagated_state, return_register| { - propagated_state.set_register_taint( - return_register, - state.get_register_taint(return_register), - ); - - propagated_state - }, - ); - - if propagated_state.has_register_taint() { - Some(propagated_state) - } else { - self.generate_cwe_warning(&return_term.tid, "return_no_taint_returned"); - - // We still want to allow for the propagation of taint from the - // call site. - Some(TaState::new_empty()) - } + let return_registers = calling_convention.get_all_return_register(); + + // If there are tainted return registers we propagate the taint to + // the caller, which makes them responsible for checking it. + register_taint + .into_iter() + .filter(|(reg, taint)| return_registers.contains(®) && taint.is_tainted()) + .collect() } else { // We have tainted registers but we do not know the calling // convention. Here we simply return the complete register taint @@ -320,11 +309,30 @@ impl<'a> TaintAnalysis<'a> for TaCompCtx<'a, '_> { // by the caller such that the taint is immediatly eliminated and // we catch cases where the called function has ignored tainted // values. - let mut new_state = state.clone(); + register_taint + }; - new_state.remove_all_memory_taints(); + let propagated_state = TaState::from_mem_reg_taint( + propagated_register_taint, + propagated_memory_taint + ); - Some(new_state) + if propagated_state.is_empty() { + // If we can not propagate any taint to the caller it is implied + // that + // + // - the return value of a fallible function call may have been + // ignored by the callee + // - AND the result is not returned to the caller + // + // Thus, callers of this function have no way to know if the + // operation it performs was successful and the function itself + // might not have checked it either. + self.generate_cwe_warning(&return_term.tid, "return_no_taint"); + + None + } else { + Some(propagated_state) } } } diff --git a/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs b/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs index 3d7b6364..b32e8b77 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs @@ -80,7 +80,10 @@ impl<'a> IsolatedReturnAnalysis<'a> { /// Checks isolated return sites for taint with the results of the given /// `computation`. /// - /// Generates CWE warnings when non-returned taint is found. + /// Generates CWE warnings when non-returned taint is found. We have no + /// caller context, i.e., no aID renaming map, so we can not tell if memory + /// taint is returned or not. Currently we always assume that memory taint + /// will *not* be reachable for the caller (generates FPs). pub fn analyze(&self, computation: &context::FpComputation<'_, '_>) { for (taint_state, calling_convention, ret_insn_tid) in // Filter isolated returns with a taint state. @@ -120,7 +123,7 @@ impl<'a> IsolatedReturnAnalysis<'a> { &self.cwe_sender, &self.call, ret_insn_tid, - "isolated_returns_no_taint_returned", + "isolated_returns_no_reg_taint_returned", ); } } From bcbe4a3d992e647251f0f7df230e16d40f698e3e Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Sun, 31 Mar 2024 23:07:21 +0200 Subject: [PATCH 10/26] lib/analysis/taint: add tests for mem obj merging Signed-off-by: Valentin Obst --- .../src/analysis/taint/state/tests.rs | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/src/cwe_checker_lib/src/analysis/taint/state/tests.rs b/src/cwe_checker_lib/src/analysis/taint/state/tests.rs index 7eb48903..cda9aefd 100644 --- a/src/cwe_checker_lib/src/analysis/taint/state/tests.rs +++ b/src/cwe_checker_lib/src/analysis/taint/state/tests.rs @@ -1,7 +1,9 @@ use super::*; + use crate::analysis::graph::NodeIndex; use crate::analysis::pointer_inference::{tests::MockVsaResult, State as PiState, ValueDomain}; use crate::{abstract_domain::*, expr, variable}; + use std::collections::BTreeSet; impl State { @@ -58,6 +60,69 @@ fn new_pointer(location: &str, offset: i64) -> DataDomain { DataDomain::from_target(id, bv(offset)) } +// FIXME: This illustrates the current, unsound merging of memory taints. Make +// sure to change this test when you work on a better memory model. +#[test] +fn merge_memory_object_overlapping() { + let taint_8 = Taint::Tainted(ByteSize::new(8)); + let taint_4 = Taint::Tainted(ByteSize::new(4)); + + let mut memory_object = MemRegion::::new(ByteSize::new(8)); + let mut other_memory_object = MemRegion::::new(ByteSize::new(8)); + + memory_object.insert_at_byte_index(taint_4, 2); + memory_object.insert_at_byte_index(taint_8, 8); + memory_object.insert_at_byte_index(taint_4, 16); + other_memory_object.insert_at_byte_index(taint_8, 0); + other_memory_object.insert_at_byte_index(taint_4, 8); + other_memory_object.insert_at_byte_index(taint_4, 14); + + merge_memory_object_with_offset(&mut memory_object, &other_memory_object, 0); + + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(0)), + Some(taint_8) + ); + assert_eq!(memory_object.get_unsized(Bitvector::from_i64(2)), None); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(8)), + Some(taint_4) + ); + assert_eq!(memory_object.get_unsized(Bitvector::from_i64(12)), None); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(14)), + Some(taint_4) + ); + assert_eq!(memory_object.get_unsized(Bitvector::from_i64(16)), None); +} + +#[test] +fn merge_memory_object_nonoverlapping() { + let taint_8 = Taint::Tainted(ByteSize::new(8)); + let taint_4 = Taint::Tainted(ByteSize::new(4)); + let untaint_4 = Taint::Top(ByteSize::new(4)); + + let mut memory_object = MemRegion::::new(ByteSize::new(8)); + let mut other_memory_object = MemRegion::::new(ByteSize::new(8)); + + memory_object.insert_at_byte_index(taint_8, 8); + other_memory_object.insert_at_byte_index(untaint_4, 0); + other_memory_object.insert_at_byte_index(taint_4, 4); + other_memory_object.insert_at_byte_index(untaint_4, 8); + + merge_memory_object_with_offset(&mut memory_object, &other_memory_object, 0); + + assert_eq!(memory_object.get_unsized(Bitvector::from_i64(0)), None); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(4)), + Some(taint_4) + ); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(8)), + Some(taint_8) + ); +} + #[test] fn merge_state() { let taint = Taint::Tainted(ByteSize::new(8)); From 46273ba0d1d769d73fd0f6b2c17220ef76e901b5 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Sun, 31 Mar 2024 23:11:49 +0200 Subject: [PATCH 11/26] lib/checkers/cwe252: rename `TaCmpCtx` to `TaComputationContext` Signed-off-by: Valentin Obst --- src/cwe_checker_lib/src/checkers/cwe_252.rs | 11 ++++++++-- .../src/checkers/cwe_252/context.rs | 20 +++++++++---------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/cwe_checker_lib/src/checkers/cwe_252.rs b/src/cwe_checker_lib/src/checkers/cwe_252.rs index 82e7a9a2..d5ef3717 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252.rs @@ -196,7 +196,9 @@ impl<'a, 'b: 'a> CweAnalysis<'a, 'b> { /// Pops a call of the worklist and returns the taint analysis definition /// for it. - fn next_call_ctx(&mut self) -> Option<(IsolatedReturnAnalysis<'_>, TaCompCtx<'_, 'b>)> { + fn next_call_ctx( + &mut self, + ) -> Option<(IsolatedReturnAnalysis<'_>, TaComputationContext<'_, 'b>)> { self.worklist.calls.pop_front().map(|call| { ( IsolatedReturnAnalysis::new( @@ -205,7 +207,12 @@ impl<'a, 'b: 'a> CweAnalysis<'a, 'b> { self.project, self.cwe_sender_proto.clone(), ), - TaCompCtx::new(call, self.project, self.pi_result, &self.cwe_sender_proto), + TaComputationContext::new( + call, + self.project, + self.pi_result, + &self.cwe_sender_proto, + ), ) }) } diff --git a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs index 7e62b495..a6ea7807 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs @@ -23,7 +23,7 @@ use std::convert::AsRef; /// Type of the fixpoint computation of the taint analysis. pub type FpComputation<'a, 'b> = fixpoint::Computation< - forward_interprocedural_fixpoint::GeneralizedContext<'a, TaCompCtx<'a, 'b>>, + forward_interprocedural_fixpoint::GeneralizedContext<'a, TaComputationContext<'a, 'b>>, >; impl ToJsonCompact for FpComputation<'_, '_> { @@ -47,7 +47,7 @@ impl ToJsonCompact for FpComputation<'_, '_> { /// /// Values of this type represent the taint analysis for a particular call to an /// external function. -pub struct TaCompCtx<'a, 'b: 'a> { +pub struct TaComputationContext<'a, 'b: 'a> { /// Extern function call that is analyzed. call: MustUseCall<'a>, project: &'a Project, @@ -56,7 +56,7 @@ pub struct TaCompCtx<'a, 'b: 'a> { cwe_sender: crossbeam_channel::Sender, } -impl<'a, 'b: 'a> TaCompCtx<'a, 'b> { +impl<'a, 'b: 'a> TaComputationContext<'a, 'b> { /// Creates a new taint analysis context for the given call to an external /// function. pub(super) fn new( @@ -95,25 +95,25 @@ impl<'a, 'b: 'a> TaCompCtx<'a, 'b> { } } -impl<'a> HasCfg<'a> for TaCompCtx<'a, '_> { +impl<'a> HasCfg<'a> for TaComputationContext<'a, '_> { fn get_cfg(&self) -> &Cfg<'a> { self.pi_result.get_graph() } } -impl HasVsaResult for TaCompCtx<'_, '_> { +impl HasVsaResult for TaComputationContext<'_, '_> { fn vsa_result(&self) -> &impl VsaResult { self.pi_result } } -impl AsRef for TaCompCtx<'_, '_> { +impl AsRef for TaComputationContext<'_, '_> { fn as_ref(&self) -> &Project { self.project } } -impl<'a> TaintAnalysis<'a> for TaCompCtx<'a, '_> { +impl<'a> TaintAnalysis<'a> for TaComputationContext<'a, '_> { /// Generates a CWE warning when a transition function returns the empty /// state. /// @@ -312,10 +312,8 @@ impl<'a> TaintAnalysis<'a> for TaCompCtx<'a, '_> { register_taint }; - let propagated_state = TaState::from_mem_reg_taint( - propagated_register_taint, - propagated_memory_taint - ); + let propagated_state = + TaState::from_mem_reg_taint(propagated_register_taint, propagated_memory_taint); if propagated_state.is_empty() { // If we can not propagate any taint to the caller it is implied From 30e6abc57c80915ca5ad8f8a96a63035ae720b27 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Mon, 1 Apr 2024 07:31:06 +0200 Subject: [PATCH 12/26] lib/checkers/cwe252: small doc fixes Signed-off-by: Valentin Obst --- src/cwe_checker_lib/src/checkers/cwe_252.rs | 7 ++++--- src/cwe_checker_lib/src/checkers/cwe_252/context.rs | 2 +- .../src/checkers/cwe_252/isolated_returns.rs | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/cwe_checker_lib/src/checkers/cwe_252.rs b/src/cwe_checker_lib/src/checkers/cwe_252.rs index d5ef3717..45d2cc82 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252.rs @@ -9,7 +9,7 @@ //! //! # Examples //! -//! See CVE for examples in Linux user-mode programs: +//! See the following CVEs for examples in Linux user-mode programs: //! //! - CVE-2007-5191 //! - CVE-2017-6964 @@ -34,7 +34,7 @@ //! call was successful. //! //! Taint propagation is stopped along paths as soon as a conditional control -//! flow transfer depend on a tainted value. +//! flow transfer depends on a tainted value. //! //! # Limitations //! @@ -47,7 +47,8 @@ //! irrespective of the success or failure of the call. //! - Patterns where the return value is handled by giving it as an argument to //! another function call. -//! - ... +//! - Taint loss due to Pointer Inference inexactness may cause the return value +//! check to not be recognized as such. //! //! ## False Negatives //! diff --git a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs index a6ea7807..c28ae1f3 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs @@ -249,7 +249,7 @@ impl<'a> TaintAnalysis<'a> for TaComputationContext<'a, '_> { /// - The calling convention may be unknown, which means we can not /// determine precisely which taint will be reachable for the caller. /// - /// See the the source code comments for further information. + /// See the source code comments for further information. /// /// Furthermore, this callback is responsible for detecting cases where /// taint may reach the end of a function without being returned. diff --git a/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs b/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs index b32e8b77..5c7a4bba 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252/isolated_returns.rs @@ -2,7 +2,7 @@ //! //! Taint that reaches a return site implies that there is some path from the //! call to a fallible function to the return instruction where the return value -//! is not checked. If additionally, the taint is not not returned to the caller +//! is not checked. If additionally, the taint is not returned to the caller //! we have a bug, since with the return of the function all information about //! success or failure of the function call is lost. //! From 48196b3e95c991eb79ac248c78c5329970a59854 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Tue, 2 Apr 2024 08:42:34 +0200 Subject: [PATCH 13/26] test: add acceptance test for cwe252 Signed-off-by: Valentin Obst --- test/artificial_samples/SConstruct | 5 +- test/artificial_samples/cwe_252.c | 342 +++++++++++++++++++++++++++++ test/src/lib.rs | 42 +++- 3 files changed, 384 insertions(+), 5 deletions(-) create mode 100644 test/artificial_samples/cwe_252.c diff --git a/test/artificial_samples/SConstruct b/test/artificial_samples/SConstruct index 99e80640..fde8286d 100644 --- a/test/artificial_samples/SConstruct +++ b/test/artificial_samples/SConstruct @@ -3,7 +3,7 @@ import os build_path = 'build' supported_architectures = ['x64', 'x86', 'arm', 'aarch64', 'mips', 'mipsel', 'mips64', 'mips64el', 'ppc', 'ppc64', 'ppc64le'] -skip_for_pe = ['cwe_782.c', 'cwe_426.c', 'cwe_243.c', 'cwe_243_clean.c'] +skip_for_pe = ['cwe_252.c', 'cwe_782.c', 'cwe_426.c', 'cwe_243.c', 'cwe_243_clean.c'] c_compilers = {'x64': ['gcc', 'x86_64-w64-mingw32-gcc', 'clang'], 'x86': ['gcc', 'i686-w64-mingw32-gcc', 'clang'], @@ -79,8 +79,11 @@ def which(pgm): def optimize(filename): optimize_me = ["cwe_119.c"] + hyperoptimize_me = ["cwe_252.c"] if filename in optimize_me: return ' -O1' + elif filename in hyperoptimize_me: + return ' -O2' else: return ' -O0' diff --git a/test/artificial_samples/cwe_252.c b/test/artificial_samples/cwe_252.c new file mode 100644 index 00000000..2d9c2283 --- /dev/null +++ b/test/artificial_samples/cwe_252.c @@ -0,0 +1,342 @@ +#include +#include +#include +#include + +char *g_ret; +char **g_ret_store; + +int main() +{ + return 1; +} + +__attribute_noinline__ int some_func(int x) +{ + int seed = x + (int)time(NULL); + srand(seed); + return seed; +} + +/*----------------------------------------------------------------------------*/ + +__attribute_noinline__ static void cwe252_inter_callee_g_checked(char *buf, + size_t n) +{ + g_ret = fgets(buf, n, stdin); +} + +int cwe252_inter_caller_g_checked(void) +{ + int number = rand(); + char buf[10]; + size_t n = sizeof(buf); + + cwe252_inter_callee_g_checked(buf, n); + + if (g_ret) + printf("%s", buf); + + printf("My number was %d\n", number); + + return number * number; +} + +/*----------------------------------------------------------------------------*/ + +__attribute_noinline__ static void cwe252_inter_callee_g_unchecked(void) +{ + char buf[10]; + + g_ret = fgets(buf, sizeof(buf), stdin); // CWE_WARNING, 1 + + printf("%p: %s\n", buf, buf); +} + +int cwe252_inter_caller_g_unchecked(void) +{ + int number = rand(); + + cwe252_inter_callee_g_unchecked(); + + printf("My number was %d\n", number); + + return number * + number; // CWE_WARNING, 2, reason=isolated_returns_no_reg_taint +} + +/*----------------------------------------------------------------------------*/ + +__attribute_noinline__ static void cwe252_inter_callee_h_gptr_checked(char *buf, + size_t n) +{ + g_ret_store = malloc(8); + + if (!g_ret_store) + exit(1); + + *g_ret_store = fgets(buf, n, stdin); +} + +int cwe252_inter_caller_h_gptr_checked(void) +{ + int number = rand(); + char buf[10]; + size_t n = sizeof(buf); + + cwe252_inter_callee_h_gptr_checked(buf, n); + + if (*g_ret_store) + printf("%s", buf); + + printf("My number was %d\n", number); + + return number * number; +} + +/*----------------------------------------------------------------------------*/ + +__attribute_noinline__ static char ** +cwe252_inter_callee_h_return_ptr_checked(char *buf, size_t n) +{ + char **ret_store = malloc(8); + + if (!ret_store) + return NULL; + + *ret_store = fgets(buf, n, stdin); + + return ret_store; +} + +int cwe252_inter_caller_h_return_ptr_checked(void) +{ + int number = rand(); + char buf[10]; + size_t n = sizeof(buf); + char **ret_store = cwe252_inter_callee_h_return_ptr_checked(buf, n); + + if (!ret_store) + exit(1); + + if (*ret_store) + printf("%s", buf); + + printf("My number was %d\n", number); + + return number * number; +} + +/*----------------------------------------------------------------------------*/ + +__attribute_noinline__ static char *cwe252_inter_callee_r_return_lost(void) +{ + char buf[10]; + + return fgets(buf, sizeof(buf), stdin); // CWE_WARNING, 1 +} + +int cwe252_inter_caller_r_return_lost(void) +{ + int number = rand(); + + cwe252_inter_callee_r_return_lost(); + + printf("My number was %d\n", number); // CWE_WARNING, 2, reason=empty_state + + return number * number; +} + +/*----------------------------------------------------------------------------*/ + +__attribute_noinline__ static char *cwe252_inter_callee_rh_return_checked(void) +{ + char buf[10]; + char *volatile *ret_storage = malloc(8); + + if (!ret_storage) { + return NULL; + } + + *ret_storage = fgets(buf, sizeof(buf), stdin); + + return *ret_storage; +} + +int cwe252_inter_caller_rh_return_checked(void) +{ + int number = rand(); + char *ret; + + ret = cwe252_inter_callee_rh_return_checked(); + + if (!ret) { + abort(); + } + + printf("My number was %d\n", number); + + return number * number; +} + +/*----------------------------------------------------------------------------*/ + +__attribute_noinline__ static void +cwe252_inter_callee_s_return_param_checked(char **ret_store, char *buf, + size_t n) +{ + *ret_store = fgets(buf, n, stdin); +} + +int cwe252_inter_caller_s_return_param_checked(void) +{ + int number = rand(); + char buf[10]; + size_t n = sizeof(buf); + char *ret; + + cwe252_inter_callee_s_return_param_checked(&ret, buf, n); + + if (ret) + printf("%s", buf); + + printf("My number was %d\n", number); + + return number * number; +} + +/*----------------------------------------------------------------------------*/ + +__attribute_noinline__ static void cwe252_inter_callee_s_unchecked(void) +{ + char buf[10]; + char *volatile ret; + + ret = fgets(buf, sizeof(buf), stdin); // CWE_WARNING, 1 + + printf("%p: %s\n", buf, buf); // CWE_WARNING, 2, reason=return_no_taint +} + +int cwe252_inter_caller_s_unchecked(void) +{ + int number = rand(); + + cwe252_inter_callee_s_unchecked(); + + printf("My number was %d\n", number); + + return number * number; +} + +/*----------------------------------------------------------------------------*/ + +void cwe252_intra_g_lost(void) +{ + char buf[10]; + static char *volatile ret; + + ret = fgets(buf, sizeof(buf), stdin); // CWE_WARNING, 1 + + if (rand() == 0) { + ret = buf; // CWE_WARNING, 2, reason=empty_state + } + + if (ret) { + puts(buf); + } +} + +/*----------------------------------------------------------------------------*/ + +void cwe252_intra_g_unchecked(void) +{ + char buf[10]; + static char *volatile ret; + + ret = fgets(buf, sizeof(buf), stdin); // CWE_WARNING, 1 + + printf("%p: %s\n", buf, + buf); // CWE_WARNING, 2, reason=isolated_returns_no_reg_taint +} + +/*----------------------------------------------------------------------------*/ + +void cwe252_intra_h_lost(void) +{ + char buf[10]; + char *volatile *ret = malloc(8); + + if (!ret) { + return; + } + + *ret = fgets(buf, sizeof(buf), stdin); // CWE_WARNING, 1 + + if (rand() == 0) { + *ret = buf; // CWE_WARNING, 2, reason=empty_state + } + + if (*ret) { + puts(buf); + } +} + +/*----------------------------------------------------------------------------*/ + +void cwe252_intra_r_checked(void) +{ + char buf[10]; + char *ret; + + ret = fgets(buf, sizeof(buf), stdin); + + if (ret) { + puts(buf); + } +} + +/*----------------------------------------------------------------------------*/ + +extern int some_func(int); + +void cwe252_intra_r_lost(void) +{ + char buf[10]; + + fgets(buf, sizeof(buf), stdin); // CWE_WARNING, 1 + + some_func(42); // CWE_WARNING, 2, reason=empty_state + + puts(buf); +} + +/*----------------------------------------------------------------------------*/ + +void cwe252_intra_s_lost(void) +{ + char buf[10]; + char *volatile ret; + + ret = fgets(buf, sizeof(buf), stdin); // CWE_WARNING, 1 + + if (rand() == 0) { + ret = buf; // CWE_WARNING, 2, reason=empty_state + } + + if (ret) { + puts(buf); + } +} + +/*----------------------------------------------------------------------------*/ + +void cwe252_intra_s_unchecked(void) +{ + char buf[10]; + char *volatile ret; + + ret = fgets(buf, sizeof(buf), stdin); // CWE_WARNING, 1 + some_func(42); // CWE_MODULE, 2, reason=isolated_returns_no_reg_taint +} + +/*----------------------------------------------------------------------------*/ diff --git a/test/src/lib.rs b/test/src/lib.rs index 7c19ad62..86475a4a 100644 --- a/test/src/lib.rs +++ b/test/src/lib.rs @@ -41,15 +41,21 @@ pub struct CweTestCase { impl CweTestCase { /// Get the file path of the test binary fn get_filepath(&self) -> String { + let cwd = std::env::current_dir() + .unwrap() + .into_os_string() + .into_string() + .unwrap(); + if self.is_lkm { format!( - "lkm_samples/build/{}_{}_{}.ko", - self.cwe, self.architecture, self.compiler + "{}/lkm_samples/build/{}_{}_{}.ko", + cwd, self.cwe, self.architecture, self.compiler ) } else { format!( - "artificial_samples/build/{}_{}_{}.out", - self.cwe, self.architecture, self.compiler + "{}/artificial_samples/build/{}_{}_{}.out", + cwd, self.cwe, self.architecture, self.compiler ) } } @@ -418,6 +424,34 @@ mod tests { } } + #[test] + #[ignore] + fn cwe_252() { + let mut error_log = Vec::new(); + let mut tests = linux_test_cases("cwe_252", "CWE252"); + let num_expected_occurences = 9; + + mark_architecture_skipped(&mut tests, "ppc64"); + mark_architecture_skipped(&mut tests, "ppc64le"); + mark_skipped(&mut tests, "aarch64", "gcc"); + mark_skipped(&mut tests, "mips", "gcc"); + mark_skipped(&mut tests, "mips64", "gcc"); + mark_skipped(&mut tests, "mips64el", "gcc"); + mark_skipped(&mut tests, "mipsel", "gcc"); + mark_skipped(&mut tests, "x86", "gcc"); + + for test_case in tests { + if let Err(error) = test_case.run_test("[CWE252]", num_expected_occurences) { + error_log.push((test_case.get_filepath(), error)); + } + } + + if !error_log.is_empty() { + print_errors(error_log); + panic!(); + } + } + #[test] #[ignore] fn cwe_332() { From cc39d580ac8e0291c0f07e510398ce72f0a104be Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Tue, 2 Apr 2024 17:27:45 +0200 Subject: [PATCH 14/26] test: add LKM acceptance test for cwe252 Signed-off-by: Valentin Obst --- test/lkm_samples/Makefile | 1 + test/lkm_samples/cwe_252.c | 40 ++++++++++++++++++++++++++++++++++++++ test/src/lib.rs | 11 ++++++++++- 3 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 test/lkm_samples/cwe_252.c diff --git a/test/lkm_samples/Makefile b/test/lkm_samples/Makefile index 85a6ab58..a20b31ba 100644 --- a/test/lkm_samples/Makefile +++ b/test/lkm_samples/Makefile @@ -1,3 +1,4 @@ +obj-m += cwe_252.o obj-m += cwe_467.o obj-m += cwe_476.o obj-m += cwe_676.o diff --git a/test/lkm_samples/cwe_252.c b/test/lkm_samples/cwe_252.c new file mode 100644 index 00000000..94816104 --- /dev/null +++ b/test/lkm_samples/cwe_252.c @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include +#include +#include +#include +#include +#include + +void cwe252_intra_h_lost(void* uptr) +{ + char buf[10]; + long *ret_store = kmalloc(8, GFP_KERNEL); + + WRITE_ONCE(*ret_store, strncpy_from_user(buf, uptr, sizeof(buf))); // CWE_WARNING, 1 + + pr_info("Call some func\n"); + + WRITE_ONCE(*ret_store, 0); // CWE_WARNING, 2, reason=empty_state + + pr_info("buf: %s\n", buf); +} + +static int __init test_init(void) +{ + pr_info("Hello, World\n"); + + return 0; +} + +static void __exit test_exit(void) +{ + pr_info("Goodbye, World\n"); +} + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Valentin Obst"); + +module_init(test_init); +module_exit(test_exit); diff --git a/test/src/lib.rs b/test/src/lib.rs index 86475a4a..edfe4a4a 100644 --- a/test/src/lib.rs +++ b/test/src/lib.rs @@ -19,7 +19,7 @@ pub const LKM_ARCHITECTURES: &[&str] = &["aarch64"]; /// Compilers used for the Linux kernel module test samples. pub const LKM_COMPILERS: &[&str] = &["clang"]; /// CWEs that are supported for Linux kernel modules. -pub const LKM_CWE: &[&str] = &["cwe_467", "cwe_476", "cwe_676"]; +pub const LKM_CWE: &[&str] = &["cwe_252", "cwe_467", "cwe_476", "cwe_676"]; /// A test case containing the necessary information to run an acceptance test. #[derive(Debug, PartialEq, Eq, Hash, Clone)] @@ -430,6 +430,9 @@ mod tests { let mut error_log = Vec::new(); let mut tests = linux_test_cases("cwe_252", "CWE252"); let num_expected_occurences = 9; + let num_expected_occurences_lkm = 1; + + tests.extend(lkm_test_cases("cwe_252", "CWE252")); mark_architecture_skipped(&mut tests, "ppc64"); mark_architecture_skipped(&mut tests, "ppc64le"); @@ -441,6 +444,12 @@ mod tests { mark_skipped(&mut tests, "x86", "gcc"); for test_case in tests { + let num_expected_occurences = if test_case.is_lkm { + num_expected_occurences_lkm + } else { + num_expected_occurences + }; + if let Err(error) = test_case.run_test("[CWE252]", num_expected_occurences) { error_log.push((test_case.get_filepath(), error)); } From 1c0303bf5b02b4f10d897a3ffe06a3db538424d9 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Wed, 3 Apr 2024 17:20:35 +0200 Subject: [PATCH 15/26] lib/abstract_domain: add `merge_with` method to `AbstractDomain` The `merge` method always produces a new, owned value. This might be undesirable in situations where it is possible to modify an existing value in-place. Add a new method that allows an abstract domain to provide a method to merge one object into another in-place. It is a common pattern to see something like this: ``` *mut_ref = mut_ref.merge(other_ref); ``` where `mut_ref` is a mutable reference to a type that implements `AbstractDomain`. Note that it is common that such types are just wrappers around an `Arc` to an inner type that is expensive to clone. However, while cloning of one of the refs in `merge` may be cheap, it means that then there are >= 2 references to the underlying `Arc`, which means that it can never be cheaply modified, i.e., the retuned owned value will usually involve an expensive clone. However: ``` mut_ref.merge_with(other_ref); ``` can potentially do a cheap modification of the underlying `Arc`. Due to the default implementation it should always be OK to replace the first pattern with the second in generic code, i.e., it never decreases performance and can only increase it if the type provides an optimized implementation. Signed-off-by: Valentin Obst --- .../src/abstract_domain/mod.rs | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/cwe_checker_lib/src/abstract_domain/mod.rs b/src/cwe_checker_lib/src/abstract_domain/mod.rs index ee446693..5ed92207 100644 --- a/src/cwe_checker_lib/src/abstract_domain/mod.rs +++ b/src/cwe_checker_lib/src/abstract_domain/mod.rs @@ -36,10 +36,32 @@ pub use domain_map::*; /// Each abstract domain is partially ordered. /// Abstract domains of the same type can be merged. pub trait AbstractDomain: Sized + Eq + Clone { - /// Return an upper bound (with respect to the partial order on the domain) for the two inputs `self` and `other`. + /// Returns an upper bound (with respect to the partial order on the domain) + /// for the two inputs `self` and `other`. #[must_use] fn merge(&self, other: &Self) -> Self; + /// Returns an upper bound (with respect to the partial order on the domain) + /// for the two inputs `self` and `other`. + /// + /// Modifies `self` in-place to hold the result. This can be useful in + /// situations where it is not necessary to create a new object and more + /// efficient to modify an existing one in-place. + /// + /// # Default + /// + /// Calls [`AbstractDomain::merge`] on the inputs and overwrites `self` with + /// the result. Does nothing when `self` is equal to `other`. + fn merge_with(&mut self, other: &Self) { + if self == other { + return; + } + + let new_value = self.merge(other); + + *self = new_value; + } + /// Returns whether the element represents the top element (i.e. maximal with respect to the partial order) or not. /// If a domain has no maximal element, this function should always return false. fn is_top(&self) -> bool; From 108f3c4580700c0f5a6ce569f4d8955f0d0e0db5 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Wed, 3 Apr 2024 18:40:17 +0200 Subject: [PATCH 16/26] lib/abstract_domain/domain_map: add `merge_map_with` to `MapMergeStrategy`, implement `merge_with` Add a `merge_map_with` method to the `MapMergeStrategy` and provide a default implementation in terms of it for the `merge_map` method. Use `merge_map_with` to provide an optimized implementation of `merge_with` for `DomainMap`. Convert all existing implementations of `MapMergeStrategy` to implement `merge_map_with` instead. The rationale is change similar to the one detailed in Commit(" 7c0ffbe lib/abstract_domain: add `merge_with` method to `AbstractDomain`"). Signed-off-by: Valentin Obst --- .../src/abstract_domain/domain_map.rs | 147 ++++++++++++------ 1 file changed, 100 insertions(+), 47 deletions(-) diff --git a/src/cwe_checker_lib/src/abstract_domain/domain_map.rs b/src/cwe_checker_lib/src/abstract_domain/domain_map.rs index c3995867..fbc7cd2c 100644 --- a/src/cwe_checker_lib/src/abstract_domain/domain_map.rs +++ b/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) { + if self == other { + return; } + + let mut_map = Arc::make_mut(&mut self.inner); + + S::merge_map_with(mut_map, &other.inner); } /// A `DomainMap` is considered to be a `Top` element if it is empty. @@ -111,6 +122,33 @@ where } } +impl Default for DomainMap +where + K: PartialOrd + Ord + Clone, + V: AbstractDomain, + S: MapMergeStrategy + Clone + Eq, +{ + fn default() -> Self { + Self::new() + } +} + +impl DomainMap +where + K: PartialOrd + Ord + Clone, + V: AbstractDomain, + S: MapMergeStrategy + 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: @@ -118,8 +156,23 @@ where /// * [`IntersectMergeStrategy`] /// * [`MergeTopStrategy`] pub trait MapMergeStrategy { - /// This function determines how two [`DomainMap`] instances are merged as abstract domains. - fn merge_map(map_left: &BTreeMap, map_right: &BTreeMap) -> BTreeMap; + /// 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, map_right: &BTreeMap) -> BTreeMap { + 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, other: &BTreeMap); } /// A [`MapMergeStrategy`] where key-value pairs whose key is only present in one input map @@ -135,45 +188,43 @@ pub struct UnionMergeStrategy { } impl MapMergeStrategy for UnionMergeStrategy { - fn merge_map(map_left: &BTreeMap, map_right: &BTreeMap) -> BTreeMap { - 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, other: &BTreeMap) { + 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 MapMergeStrategy for IntersectMergeStrategy { - fn merge_map(map_left: &BTreeMap, map_right: &BTreeMap) -> BTreeMap { - 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, other: &BTreeMap) { + 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 MapMergeStrategy for MergeTopStrategy { - fn merge_map(map_left: &BTreeMap, map_right: &BTreeMap) -> BTreeMap { - 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, other: &BTreeMap) { + 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 } } From 2d253726589c9030c742cc3367db807d6e798bdf Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Thu, 4 Apr 2024 13:19:51 +0200 Subject: [PATCH 17/26] lib/analysis/taint: override `merge_with` impl for `Taint` Signed-off-by: Valentin Obst --- src/cwe_checker_lib/src/analysis/taint/mod.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/cwe_checker_lib/src/analysis/taint/mod.rs b/src/cwe_checker_lib/src/analysis/taint/mod.rs index 154ed97a..bda1a773 100644 --- a/src/cwe_checker_lib/src/analysis/taint/mod.rs +++ b/src/cwe_checker_lib/src/analysis/taint/mod.rs @@ -522,6 +522,18 @@ impl AbstractDomain for Taint { } } + /// Replaces `self` with `other` iff `self` is untainted and `other` is + /// tainted. + /// + /// No change to `self` is required in the other cases. + fn merge_with(&mut self, other: &Self) { + use Taint::*; + + if let (Top(_), Tainted(_)) = (&self, other) { + *self = *other; + }; + } + /// Checks whether the value is an untainted `Top`-value. fn is_top(&self) -> bool { matches!(self, Taint::Top(_)) From 1b599fb84c62452628ffaceaee630057f6173911 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Wed, 3 Apr 2024 19:43:11 +0200 Subject: [PATCH 18/26] lib/analysis/taint: use `DomainMap` for register and memory taint In the original implementation of memory taint propagation in Commit("e7c25f7 lib/analysis/taint: propagate memory taint") it was overlooked that we already have an abstraction for maps into abstract domains that are abstract domains themselves. Use the `DomainMap` abstraction for the register and memory taint maps that make up the `State` type. Signed-off-by: Valentin Obst --- .../src/analysis/taint/state.rs | 122 ++-------- .../src/analysis/taint/state/memory_taint.rs | 218 ++++++++++++++++++ .../analysis/taint/state/register_taint.rs | 10 + .../src/analysis/taint/state/tests.rs | 67 +----- .../src/checkers/cwe_252/context.rs | 35 ++- 5 files changed, 264 insertions(+), 188 deletions(-) create mode 100644 src/cwe_checker_lib/src/analysis/taint/state/memory_taint.rs create mode 100644 src/cwe_checker_lib/src/analysis/taint/state/register_taint.rs diff --git a/src/cwe_checker_lib/src/analysis/taint/state.rs b/src/cwe_checker_lib/src/analysis/taint/state.rs index 7ac728c0..44f7f5a3 100644 --- a/src/cwe_checker_lib/src/analysis/taint/state.rs +++ b/src/cwe_checker_lib/src/analysis/taint/state.rs @@ -14,27 +14,17 @@ use crate::intermediate_representation::*; use crate::prelude::*; use crate::utils::debug::ToJsonCompact; -use std::collections::{hash_map, BTreeMap, HashMap}; +use std::collections::BTreeMap; use super::Taint; +mod memory_taint; +mod register_taint; #[cfg(test)] mod tests; -// FIXME: Turn those type aliases into proper New Types that are -// `AbstractDomain`s. -// -// Background: Those types are in fact representing complete lattices -// (space of total functionsfrom some set to complete lattice) and `State` is -// just their cartesian product. Having this explicit in the type system would -// make the code in this module a lot cleaner. (When doing this one should also -// fix the memory merging such that this one actually IS a complete lattice...). -/// Represents our knowledge about taint in registers at a particular point in -/// the program. -pub type RegisterTaint = HashMap; -/// Represents our knowledge about taint in memory at a particular point in the -/// program. -pub type MemoryTaint = HashMap>; +use memory_taint::MemoryTaint; +use register_taint::RegisterTaint; /// The state object of the taint analysis representing all known tainted memory /// and register values at a certain location within the program. @@ -86,15 +76,16 @@ impl AbstractDomain for State { fn merge(&self, other: &Self) -> Self { let mut new_state = self.clone(); - new_state.merge_register_taint(&other.register_taint); - - for (aid, other_memory_object) in other.memory_taint.iter() { - new_state.merge_memory_object_with_offset(aid, other_memory_object, 0); - } + new_state.merge_with(other); new_state } + fn merge_with(&mut self, other: &Self) { + self.register_taint.merge_with(&other.register_taint); + self.memory_taint.merge_with(&other.memory_taint); + } + /// The state has no explicit Top element. fn is_top(&self) -> bool { false @@ -104,11 +95,9 @@ impl AbstractDomain for State { impl State { /// Returns an empty state. pub fn new_empty() -> Self { - // Using capacity zero guarantees that the implementation will not - // allocate. Do that to keep the cost of creating a new state low. Self { - register_taint: HashMap::with_capacity(0), - memory_taint: HashMap::with_capacity(0), + register_taint: RegisterTaint::new(), + memory_taint: MemoryTaint::new(), } } @@ -119,8 +108,8 @@ impl State { return_node: NodeIndex, ) -> Self { let mut state = Self { - register_taint: HashMap::new(), - memory_taint: HashMap::new(), + register_taint: RegisterTaint::new(), + memory_taint: MemoryTaint::new(), }; for return_arg in taint_source.return_values.iter() { @@ -227,7 +216,7 @@ impl State { /// Remove all knowledge about taints contained in memory objects. pub fn remove_all_memory_taints(&mut self) { - self.memory_taint = HashMap::new(); + self.memory_taint = MemoryTaint::new(); } /// Set the taint of a register. @@ -472,7 +461,7 @@ impl State { } = other; // Naive merging works for register taint. - self.merge_register_taint(other_register_taint); + self.register_taint.merge_with(other_register_taint); let Some(renaming_map) = renaming_map else { // Without a renaming rule we can not do anything meaningful with @@ -506,60 +495,8 @@ impl State { }; // Starts tracking the object if it does not exist. - self.merge_memory_object_with_offset(aid, other_memory_object, offset); - } - } - } - - /// Merges the given register taint into the state. - /// - /// Equivalent to merging - /// `other_register_taint` x bottom - /// into the state. - fn merge_register_taint(&mut self, other_register_taint: &RegisterTaint) { - use hash_map::Entry::*; - - for (reg, other_taint) in other_register_taint.iter() { - match self.register_taint.entry(reg.clone()) { - Occupied(mut taint) => { - // FIXME: We create a new owned taint even though we could - // modify in-place. - let new_taint = taint.get().merge(other_taint); - - taint.insert(new_taint); - } - Vacant(entry) => { - entry.insert(*other_taint); - } - } - } - } - - /// Merges the given pair of abstract identifier and memory object into the - /// state. - /// - /// Equivalent to merging - /// bottom x (`aid` -> (`other_memory_object` + `offset`)) - /// into the state. - fn merge_memory_object_with_offset( - &mut self, - aid: &AbstractIdentifier, - other_memory_object: &MemRegion, - offset: i64, - ) { - use hash_map::Entry::*; - - match self.memory_taint.entry(aid.clone()) { - Occupied(mut current_memory_object) => { - let current_memory_object = current_memory_object.get_mut(); - - merge_memory_object_with_offset(current_memory_object, other_memory_object, offset); - } - Vacant(entry) => { - let mut new_memory_object = other_memory_object.clone(); - - new_memory_object.add_offset_to_all_indices(offset); - entry.insert(new_memory_object); + self.memory_taint + .merge_memory_object_with_offset(aid, other_memory_object, offset); } } } @@ -578,27 +515,6 @@ impl State { } } -// FIXME: The used algorithm for merging the taints contained in memory -// regions is unsound when merging taints that intersect only partially. -// If, for example, in state A `RSP[0:3]` is tainted and in state B -// `RSP[2:3]` is tainted, A u B will only report `RSP[2:3]` as tainted. -// -// For the NULL pointer dereference check, however, this should not have an -// effect in practice, since these values are usually unsound or a sign of -// incorrect behavior of the analysed program. -// FIXME: This looks a lot like it should be a method on `MemRegion`. -fn merge_memory_object_with_offset( - mem_object: &mut MemRegion, - other_memory_object: &MemRegion, - offset: i64, -) { - for (index, taint) in other_memory_object.iter() { - mem_object.insert_at_byte_index(*taint, *index + offset); - // FIXME: Unsound in theory for partially intersecting - // taints. Should not matter in practice. - } -} - impl State { /// Get a more compact json-representation of the state. /// Intended for pretty printing, not useable for serialization/deserialization. diff --git a/src/cwe_checker_lib/src/analysis/taint/state/memory_taint.rs b/src/cwe_checker_lib/src/analysis/taint/state/memory_taint.rs new file mode 100644 index 00000000..c780c08b --- /dev/null +++ b/src/cwe_checker_lib/src/analysis/taint/state/memory_taint.rs @@ -0,0 +1,218 @@ +//! Tracking of taint in memory. + +use crate::abstract_domain::{AbstractIdentifier, DomainMap, MapMergeStrategy, MemRegion}; + +use super::Taint; + +use std::collections::BTreeMap; + +/// Strategy for merging two memory taint states. +/// +/// Essentially a [`UnionMergeStrategy`], i.e., the set of keys is the union of +/// the individual key sets, but we do not use the merging provided by the +/// [`MemRegion`] type on the intersection. Instead, we implement our own +/// merging of `MemRegion` in [`merge_memory_object_with_offset`]. +/// +/// [`UnionMergeStrategy`]: crate::abstract_domain::UnionMergeStrategy +#[derive(Debug, PartialEq, Eq, Clone)] +pub struct MemoryTaintMergeStrategy { + _private: (), // Marker to prevent instantiation +} + +impl MapMergeStrategy> for MemoryTaintMergeStrategy { + fn merge_map_with( + memory_taint: &mut BTreeMap>, + other_memory_taint: &BTreeMap>, + ) { + for (aid, other_memory_object) in other_memory_taint.iter() { + memory_taint + .entry(aid.clone()) + .and_modify(|memory_object| { + merge_memory_object_with_offset(memory_object, other_memory_object, 0); + }) + .or_insert_with(|| other_memory_object.clone()); + } + } +} + +/// Represents our knowledge about taint in memory at a particular point in the +/// program. +pub type MemoryTaint = DomainMap, MemoryTaintMergeStrategy>; + +impl MemoryTaint { + /// Merges the given pair of abstract identifier and memory object into the + /// state. + pub fn merge_memory_object_with_offset( + &mut self, + aid: &AbstractIdentifier, + other_memory_object: &MemRegion, + offset: i64, + ) { + use std::collections::btree_map::Entry::*; + + match self.entry(aid.clone()) { + Occupied(mut current_memory_object) => { + let current_memory_object = current_memory_object.get_mut(); + + merge_memory_object_with_offset(current_memory_object, other_memory_object, offset); + } + Vacant(entry) => { + let mut new_memory_object = other_memory_object.clone(); + + new_memory_object.add_offset_to_all_indices(offset); + entry.insert(new_memory_object); + } + } + } +} + +// FIXME: The used algorithm for merging the taints contained in memory +// regions is unsound when merging taints that intersect only partially. +// If, for example, in state A `RSP[0:3]` is tainted and in state B +// `RSP[2:3]` is tainted, A u B will only report `RSP[2:3]` as tainted. +// +// For the NULL pointer dereference check, however, this should not have an +// effect in practice, since these values are usually unsound or a sign of +// incorrect behavior of the analysed program. +// FIXME: This looks a lot like it should be a method on `MemRegion`. +/// Merges `other_memory_object` into `memory_object`. +/// +/// The `other_memory_object` is shifted by `offset` before the merging is +/// performed. For partially overlapping taints, the value of +/// `other_memory_object` always "wins", i.e., it ends up in the merged object +/// and the overlapped value in `memory_object` is discarded. +fn merge_memory_object_with_offset( + memory_object: &mut MemRegion, + other_memory_object: &MemRegion, + offset: i64, +) { + for (index, taint) in other_memory_object.iter() { + // WARNING: This is not using the `merge` function of `Taint`. It + // will become even more incorrect once we have more complicated taint. + memory_object.insert_at_byte_index(*taint, *index + offset); + // FIXME: Unsound in theory for partially intersecting + // taints. Should not matter in practice. + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::prelude::*; + + // FIXME: This illustrates the current, unsound merging of memory taints. Make + // sure to change this test when you work on a better memory model. + #[test] + fn merge_memory_object_overlapping() { + let taint_8 = Taint::Tainted(ByteSize::new(8)); + let taint_4 = Taint::Tainted(ByteSize::new(4)); + + let mut memory_object = MemRegion::::new(ByteSize::new(8)); + let mut other_memory_object = MemRegion::::new(ByteSize::new(8)); + + memory_object.insert_at_byte_index(taint_4, 2); + memory_object.insert_at_byte_index(taint_8, 8); + memory_object.insert_at_byte_index(taint_4, 16); + other_memory_object.insert_at_byte_index(taint_8, 0); + other_memory_object.insert_at_byte_index(taint_4, 8); + other_memory_object.insert_at_byte_index(taint_4, 14); + + merge_memory_object_with_offset(&mut memory_object, &other_memory_object, 0); + + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(0)), + Some(taint_8) + ); + assert_eq!(memory_object.get_unsized(Bitvector::from_i64(2)), None); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(8)), + Some(taint_4) + ); + assert_eq!(memory_object.get_unsized(Bitvector::from_i64(12)), None); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(14)), + Some(taint_4) + ); + assert_eq!(memory_object.get_unsized(Bitvector::from_i64(16)), None); + } + + #[test] + fn merge_memory_object_nonoverlapping() { + let taint_8 = Taint::Tainted(ByteSize::new(8)); + let taint_4 = Taint::Tainted(ByteSize::new(4)); + let untaint_4 = Taint::Top(ByteSize::new(4)); + + let mut memory_object = MemRegion::::new(ByteSize::new(8)); + let mut other_memory_object = MemRegion::::new(ByteSize::new(8)); + + memory_object.insert_at_byte_index(taint_8, 8); + other_memory_object.insert_at_byte_index(untaint_4, 0); + other_memory_object.insert_at_byte_index(taint_4, 4); + other_memory_object.insert_at_byte_index(untaint_4, 8); + + merge_memory_object_with_offset(&mut memory_object, &other_memory_object, 0); + + assert_eq!(memory_object.get_unsized(Bitvector::from_i64(0)), None); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(4)), + Some(taint_4) + ); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(8)), + Some(taint_8) + ); + } + + #[test] + fn merge_memory_object_with_offset_nonoverlapping() { + let taint_8 = Taint::Tainted(ByteSize::new(8)); + let taint_4 = Taint::Tainted(ByteSize::new(4)); + + let mut memory_object = MemRegion::::new(ByteSize::new(8)); + let mut other_memory_object = MemRegion::::new(ByteSize::new(8)); + + memory_object.insert_at_byte_index(taint_8, 8); + other_memory_object.insert_at_byte_index(taint_4, 8); + other_memory_object.insert_at_byte_index(taint_4, 12); + + merge_memory_object_with_offset(&mut memory_object, &other_memory_object, 8); + + assert_eq!(memory_object.get_unsized(Bitvector::from_i64(0)), None); + assert_eq!(memory_object.get_unsized(Bitvector::from_i64(4)), None); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(8)), + Some(taint_8) + ); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(16)), + Some(taint_4) + ); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(20)), + Some(taint_4) + ); + + merge_memory_object_with_offset(&mut memory_object, &other_memory_object, -8); + + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(0)), + Some(taint_4) + ); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(4)), + Some(taint_4) + ); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(8)), + Some(taint_8) + ); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(16)), + Some(taint_4) + ); + assert_eq!( + memory_object.get_unsized(Bitvector::from_i64(20)), + Some(taint_4) + ); + } +} diff --git a/src/cwe_checker_lib/src/analysis/taint/state/register_taint.rs b/src/cwe_checker_lib/src/analysis/taint/state/register_taint.rs new file mode 100644 index 00000000..f37767bf --- /dev/null +++ b/src/cwe_checker_lib/src/analysis/taint/state/register_taint.rs @@ -0,0 +1,10 @@ +//! Tracking of taint in registers. + +use crate::abstract_domain::{DomainMap, UnionMergeStrategy}; +use crate::intermediate_representation::Variable; + +use super::Taint; + +/// Represents our knowledge about taint in registers at a particular point in +/// the program. +pub type RegisterTaint = DomainMap; diff --git a/src/cwe_checker_lib/src/analysis/taint/state/tests.rs b/src/cwe_checker_lib/src/analysis/taint/state/tests.rs index cda9aefd..b8c28462 100644 --- a/src/cwe_checker_lib/src/analysis/taint/state/tests.rs +++ b/src/cwe_checker_lib/src/analysis/taint/state/tests.rs @@ -9,8 +9,8 @@ use std::collections::BTreeSet; impl State { pub fn mock() -> State { State { - register_taint: HashMap::new(), - memory_taint: HashMap::new(), + register_taint: RegisterTaint::new(), + memory_taint: MemoryTaint::new(), } } @@ -60,69 +60,6 @@ fn new_pointer(location: &str, offset: i64) -> DataDomain { DataDomain::from_target(id, bv(offset)) } -// FIXME: This illustrates the current, unsound merging of memory taints. Make -// sure to change this test when you work on a better memory model. -#[test] -fn merge_memory_object_overlapping() { - let taint_8 = Taint::Tainted(ByteSize::new(8)); - let taint_4 = Taint::Tainted(ByteSize::new(4)); - - let mut memory_object = MemRegion::::new(ByteSize::new(8)); - let mut other_memory_object = MemRegion::::new(ByteSize::new(8)); - - memory_object.insert_at_byte_index(taint_4, 2); - memory_object.insert_at_byte_index(taint_8, 8); - memory_object.insert_at_byte_index(taint_4, 16); - other_memory_object.insert_at_byte_index(taint_8, 0); - other_memory_object.insert_at_byte_index(taint_4, 8); - other_memory_object.insert_at_byte_index(taint_4, 14); - - merge_memory_object_with_offset(&mut memory_object, &other_memory_object, 0); - - assert_eq!( - memory_object.get_unsized(Bitvector::from_i64(0)), - Some(taint_8) - ); - assert_eq!(memory_object.get_unsized(Bitvector::from_i64(2)), None); - assert_eq!( - memory_object.get_unsized(Bitvector::from_i64(8)), - Some(taint_4) - ); - assert_eq!(memory_object.get_unsized(Bitvector::from_i64(12)), None); - assert_eq!( - memory_object.get_unsized(Bitvector::from_i64(14)), - Some(taint_4) - ); - assert_eq!(memory_object.get_unsized(Bitvector::from_i64(16)), None); -} - -#[test] -fn merge_memory_object_nonoverlapping() { - let taint_8 = Taint::Tainted(ByteSize::new(8)); - let taint_4 = Taint::Tainted(ByteSize::new(4)); - let untaint_4 = Taint::Top(ByteSize::new(4)); - - let mut memory_object = MemRegion::::new(ByteSize::new(8)); - let mut other_memory_object = MemRegion::::new(ByteSize::new(8)); - - memory_object.insert_at_byte_index(taint_8, 8); - other_memory_object.insert_at_byte_index(untaint_4, 0); - other_memory_object.insert_at_byte_index(taint_4, 4); - other_memory_object.insert_at_byte_index(untaint_4, 8); - - merge_memory_object_with_offset(&mut memory_object, &other_memory_object, 0); - - assert_eq!(memory_object.get_unsized(Bitvector::from_i64(0)), None); - assert_eq!( - memory_object.get_unsized(Bitvector::from_i64(4)), - Some(taint_4) - ); - assert_eq!( - memory_object.get_unsized(Bitvector::from_i64(8)), - Some(taint_8) - ); -} - #[test] fn merge_state() { let taint = Taint::Tainted(ByteSize::new(8)); diff --git a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs index c28ae1f3..72586511 100644 --- a/src/cwe_checker_lib/src/checkers/cwe_252/context.rs +++ b/src/cwe_checker_lib/src/checkers/cwe_252/context.rs @@ -12,7 +12,7 @@ use crate::analysis::forward_interprocedural_fixpoint::{ use crate::analysis::graph::{Graph as Cfg, HasCfg}; use crate::analysis::interprocedural_fixpoint_generic::NodeValue; use crate::analysis::pointer_inference::{Data as PiData, PointerInference}; -use crate::analysis::taint::state::{MemoryTaint, RegisterTaint, State as TaState}; +use crate::analysis::taint::state::State as TaState; use crate::analysis::taint::TaintAnalysis; use crate::analysis::vsa_results::{HasVsaResult, VsaResult}; use crate::intermediate_representation::{Blk, ExternSymbol, Jmp, Project, Term, Tid}; @@ -263,27 +263,25 @@ impl<'a> TaintAnalysis<'a> for TaComputationContext<'a, '_> { return_term: &Term, calling_convention: &Option, ) -> Option { - let (register_taint, memory_taint) = state.clone().into_mem_reg_taint(); + let (mut propagated_register_taint, mut propagated_memory_taint) = + state.clone().into_mem_reg_taint(); // Only keep memory taint that will be propagated to the caller. We // compute this here since we want to notice when no taint is // propagated. let renaming_map = self.pi_result.get_call_renaming_map(&call_term.tid); - let propagated_memory_taint: MemoryTaint = memory_taint - .into_iter() - .filter(|(aid, _)| { - // This is still an over-approximation to the taint that will be - // available to the caller since it might happen that all relative - // values have non-exactly-known offsets. - renaming_map.is_some_and(|renaming_map| { - renaming_map - .get(aid) - .is_some_and(|value| value.referenced_ids().next().is_some()) - }) + propagated_memory_taint.retain(|aid, _| { + // This is still an over-approximation to the taint that will be + // available to the caller since it might happen that all relative + // values have non-exactly-known offsets. + renaming_map.is_some_and(|renaming_map| { + renaming_map + .get(aid) + .is_some_and(|value| value.referenced_ids().next().is_some()) }) - .collect(); + }); - let propagated_register_taint: RegisterTaint = if let Some(calling_convention) = self + if let Some(calling_convention) = self .project .get_specific_calling_convention(calling_convention) { @@ -291,10 +289,8 @@ impl<'a> TaintAnalysis<'a> for TaComputationContext<'a, '_> { // If there are tainted return registers we propagate the taint to // the caller, which makes them responsible for checking it. - register_taint - .into_iter() - .filter(|(reg, taint)| return_registers.contains(®) && taint.is_tainted()) - .collect() + propagated_register_taint + .retain(|reg, taint| return_registers.contains(®) && taint.is_tainted()); } else { // We have tainted registers but we do not know the calling // convention. Here we simply return the complete register taint @@ -309,7 +305,6 @@ impl<'a> TaintAnalysis<'a> for TaComputationContext<'a, '_> { // by the caller such that the taint is immediatly eliminated and // we catch cases where the called function has ignored tainted // values. - register_taint }; let propagated_state = From fdc81d21398da09486d3ee700a74827c6a9c4f5f Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Fri, 5 Apr 2024 17:07:01 +0200 Subject: [PATCH 19/26] lib/abstract_domain: make `merge_with` return `&mut Self` This facilitates method chaining. Added to make the API more flexible, even though there are no users of it at the moment. Signed-off-by: Valentin Obst --- .../src/abstract_domain/domain_map.rs | 14 +++++++------- src/cwe_checker_lib/src/abstract_domain/mod.rs | 12 ++++++------ src/cwe_checker_lib/src/analysis/taint/mod.rs | 4 +++- src/cwe_checker_lib/src/analysis/taint/state.rs | 4 +++- 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/cwe_checker_lib/src/abstract_domain/domain_map.rs b/src/cwe_checker_lib/src/abstract_domain/domain_map.rs index fbc7cd2c..878f86da 100644 --- a/src/cwe_checker_lib/src/abstract_domain/domain_map.rs +++ b/src/cwe_checker_lib/src/abstract_domain/domain_map.rs @@ -106,14 +106,14 @@ where } } - fn merge_with(&mut self, other: &Self) { - if self == other { - return; - } + fn merge_with(&mut self, other: &Self) -> &mut Self { + if self != other { + let mut_map = Arc::make_mut(&mut self.inner); - let mut_map = Arc::make_mut(&mut self.inner); + S::merge_map_with(mut_map, &other.inner); + } - S::merge_map_with(mut_map, &other.inner); + self } /// A `DomainMap` is considered to be a `Top` element if it is empty. @@ -244,7 +244,7 @@ impl MapMergeStrategy for Merg fn merge_map_with(map: &mut BTreeMap, other: &BTreeMap) { map.retain(|key, value| { if let Some(value_other) = other.get(key) { - value.merge_with(value_other) + value.merge_with(value_other); } else { let top = value.top(); diff --git a/src/cwe_checker_lib/src/abstract_domain/mod.rs b/src/cwe_checker_lib/src/abstract_domain/mod.rs index 5ed92207..74122ee5 100644 --- a/src/cwe_checker_lib/src/abstract_domain/mod.rs +++ b/src/cwe_checker_lib/src/abstract_domain/mod.rs @@ -52,14 +52,14 @@ pub trait AbstractDomain: Sized + Eq + Clone { /// /// Calls [`AbstractDomain::merge`] on the inputs and overwrites `self` with /// the result. Does nothing when `self` is equal to `other`. - fn merge_with(&mut self, other: &Self) { - if self == other { - return; - } + fn merge_with(&mut self, other: &Self) -> &mut Self { + if self != other { + let new_value = self.merge(other); - let new_value = self.merge(other); + *self = new_value; + } - *self = new_value; + self } /// Returns whether the element represents the top element (i.e. maximal with respect to the partial order) or not. diff --git a/src/cwe_checker_lib/src/analysis/taint/mod.rs b/src/cwe_checker_lib/src/analysis/taint/mod.rs index bda1a773..bc2fb3c2 100644 --- a/src/cwe_checker_lib/src/analysis/taint/mod.rs +++ b/src/cwe_checker_lib/src/analysis/taint/mod.rs @@ -526,12 +526,14 @@ impl AbstractDomain for Taint { /// tainted. /// /// No change to `self` is required in the other cases. - fn merge_with(&mut self, other: &Self) { + fn merge_with(&mut self, other: &Self) -> &mut Self { use Taint::*; if let (Top(_), Tainted(_)) = (&self, other) { *self = *other; }; + + self } /// Checks whether the value is an untainted `Top`-value. diff --git a/src/cwe_checker_lib/src/analysis/taint/state.rs b/src/cwe_checker_lib/src/analysis/taint/state.rs index 44f7f5a3..a955965b 100644 --- a/src/cwe_checker_lib/src/analysis/taint/state.rs +++ b/src/cwe_checker_lib/src/analysis/taint/state.rs @@ -81,9 +81,11 @@ impl AbstractDomain for State { new_state } - fn merge_with(&mut self, other: &Self) { + fn merge_with(&mut self, other: &Self) -> &mut Self { self.register_taint.merge_with(&other.register_taint); self.memory_taint.merge_with(&other.memory_taint); + + self } /// The state has no explicit Top element. From 28882ba387f1603c9e40882decaa589856c67a91 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Fri, 5 Apr 2024 17:13:29 +0200 Subject: [PATCH 20/26] lib/analysis/taint/state: overwrite memory taint in more cases Currently we only overwrite memory taint if the PI result for the target address is very exact. Weaken the conditions under which we overwrite taint information by allowing possibly constant or top values for the target address as long as the target memory object and offset are unique. This may lead to taint being overwritten with non-tainted values in more cases. Signed-off-by: Valentin Obst --- .../src/analysis/taint/state.rs | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/cwe_checker_lib/src/analysis/taint/state.rs b/src/cwe_checker_lib/src/analysis/taint/state.rs index a955965b..2b2a9bc3 100644 --- a/src/cwe_checker_lib/src/analysis/taint/state.rs +++ b/src/cwe_checker_lib/src/analysis/taint/state.rs @@ -5,7 +5,8 @@ use crate::abstract_domain::AbstractLocation; use crate::abstract_domain::{ - AbstractDomain, AbstractIdentifier, MemRegion, RegisterDomain, SizedDomain, TryToBitvec, + AbstractDomain, AbstractIdentifier, IntervalDomain, MemRegion, RegisterDomain, SizedDomain, + TryToBitvec, }; use crate::analysis::graph::NodeIndex; use crate::analysis::pointer_inference::Data as PiData; @@ -187,10 +188,15 @@ impl State { /// Mark the value at the given address with the given taint. /// /// If the address may point to more than one object, we merge the taint - /// object with the object at the targets, possibly tainting all possible - /// targets. + /// into all objects for which the corresponding offset is exact. Since we + /// merge, this will never remove any taint. + /// + /// If the pointee object and offset are exactly known, we write the + /// `taint` to the object at the given offset. This may remove taint. + /// + /// In all other cases we do nothing. pub fn save_taint_to_memory(&mut self, address: &PiData, taint: Taint) { - if let Some((mem_id, offset)) = address.get_if_unique_target() { + if let Some((mem_id, offset)) = get_if_unique_target(address) { if let Ok(position) = offset.try_to_bitvec() { if let Some(mem_region) = self.memory_taint.get_mut(mem_id) { mem_region.add(taint, position); @@ -547,3 +553,17 @@ impl State { Value::Object(Map::from_iter(state_map)) } } + +/// Returns target ID and offset iff there is a single relative value. +/// +/// In contrast to `DataDomain::get_if_unique_target` this function also +/// returns the pair when the `is_top` flag is set or the value may be absolute. +fn get_if_unique_target(address: &PiData) -> Option<(&AbstractIdentifier, &IntervalDomain)> { + let relative_values = address.get_relative_values(); + + if relative_values.len() == 1 { + Some(relative_values.iter().next().unwrap()) + } else { + None + } +} From 4a07762234788d1c4037832989ce320257ab23ff Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Fri, 5 Apr 2024 17:46:12 +0200 Subject: [PATCH 21/26] test/cwe252: use `-O0`, do not skip mingw We gain 4 arch-compiler pairs and loose 2 by making this change. Tests were developed with `-O2` in mind so they might not work as expected, despite reporting the correct number of warnings. I only verified correctness manually for `aarch64-clang`. Signed-off-by: Valentin Obst --- test/artificial_samples/SConstruct | 5 +---- test/artificial_samples/cwe_252.c | 24 ++++++++++-------------- test/src/lib.rs | 10 ++++------ 3 files changed, 15 insertions(+), 24 deletions(-) diff --git a/test/artificial_samples/SConstruct b/test/artificial_samples/SConstruct index fde8286d..99e80640 100644 --- a/test/artificial_samples/SConstruct +++ b/test/artificial_samples/SConstruct @@ -3,7 +3,7 @@ import os build_path = 'build' supported_architectures = ['x64', 'x86', 'arm', 'aarch64', 'mips', 'mipsel', 'mips64', 'mips64el', 'ppc', 'ppc64', 'ppc64le'] -skip_for_pe = ['cwe_252.c', 'cwe_782.c', 'cwe_426.c', 'cwe_243.c', 'cwe_243_clean.c'] +skip_for_pe = ['cwe_782.c', 'cwe_426.c', 'cwe_243.c', 'cwe_243_clean.c'] c_compilers = {'x64': ['gcc', 'x86_64-w64-mingw32-gcc', 'clang'], 'x86': ['gcc', 'i686-w64-mingw32-gcc', 'clang'], @@ -79,11 +79,8 @@ def which(pgm): def optimize(filename): optimize_me = ["cwe_119.c"] - hyperoptimize_me = ["cwe_252.c"] if filename in optimize_me: return ' -O1' - elif filename in hyperoptimize_me: - return ' -O2' else: return ' -O0' diff --git a/test/artificial_samples/cwe_252.c b/test/artificial_samples/cwe_252.c index 2d9c2283..8e318e94 100644 --- a/test/artificial_samples/cwe_252.c +++ b/test/artificial_samples/cwe_252.c @@ -11,7 +11,7 @@ int main() return 1; } -__attribute_noinline__ int some_func(int x) +int some_func(int x) { int seed = x + (int)time(NULL); srand(seed); @@ -20,8 +20,7 @@ __attribute_noinline__ int some_func(int x) /*----------------------------------------------------------------------------*/ -__attribute_noinline__ static void cwe252_inter_callee_g_checked(char *buf, - size_t n) +static void cwe252_inter_callee_g_checked(char *buf, size_t n) { g_ret = fgets(buf, n, stdin); } @@ -44,7 +43,7 @@ int cwe252_inter_caller_g_checked(void) /*----------------------------------------------------------------------------*/ -__attribute_noinline__ static void cwe252_inter_callee_g_unchecked(void) +static void cwe252_inter_callee_g_unchecked(void) { char buf[10]; @@ -67,8 +66,7 @@ int cwe252_inter_caller_g_unchecked(void) /*----------------------------------------------------------------------------*/ -__attribute_noinline__ static void cwe252_inter_callee_h_gptr_checked(char *buf, - size_t n) +static void cwe252_inter_callee_h_gptr_checked(char *buf, size_t n) { g_ret_store = malloc(8); @@ -96,8 +94,7 @@ int cwe252_inter_caller_h_gptr_checked(void) /*----------------------------------------------------------------------------*/ -__attribute_noinline__ static char ** -cwe252_inter_callee_h_return_ptr_checked(char *buf, size_t n) +static char **cwe252_inter_callee_h_return_ptr_checked(char *buf, size_t n) { char **ret_store = malloc(8); @@ -129,7 +126,7 @@ int cwe252_inter_caller_h_return_ptr_checked(void) /*----------------------------------------------------------------------------*/ -__attribute_noinline__ static char *cwe252_inter_callee_r_return_lost(void) +static char *cwe252_inter_callee_r_return_lost(void) { char buf[10]; @@ -149,7 +146,7 @@ int cwe252_inter_caller_r_return_lost(void) /*----------------------------------------------------------------------------*/ -__attribute_noinline__ static char *cwe252_inter_callee_rh_return_checked(void) +static char *cwe252_inter_callee_rh_return_checked(void) { char buf[10]; char *volatile *ret_storage = malloc(8); @@ -181,9 +178,8 @@ int cwe252_inter_caller_rh_return_checked(void) /*----------------------------------------------------------------------------*/ -__attribute_noinline__ static void -cwe252_inter_callee_s_return_param_checked(char **ret_store, char *buf, - size_t n) +static void cwe252_inter_callee_s_return_param_checked(char **ret_store, + char *buf, size_t n) { *ret_store = fgets(buf, n, stdin); } @@ -207,7 +203,7 @@ int cwe252_inter_caller_s_return_param_checked(void) /*----------------------------------------------------------------------------*/ -__attribute_noinline__ static void cwe252_inter_callee_s_unchecked(void) +static void cwe252_inter_callee_s_unchecked(void) { char buf[10]; char *volatile ret; diff --git a/test/src/lib.rs b/test/src/lib.rs index edfe4a4a..7d408fd0 100644 --- a/test/src/lib.rs +++ b/test/src/lib.rs @@ -428,20 +428,18 @@ mod tests { #[ignore] fn cwe_252() { let mut error_log = Vec::new(); - let mut tests = linux_test_cases("cwe_252", "CWE252"); + let mut tests = all_test_cases("cwe_252", "CWE252"); let num_expected_occurences = 9; let num_expected_occurences_lkm = 1; - tests.extend(lkm_test_cases("cwe_252", "CWE252")); - mark_architecture_skipped(&mut tests, "ppc64"); mark_architecture_skipped(&mut tests, "ppc64le"); - mark_skipped(&mut tests, "aarch64", "gcc"); mark_skipped(&mut tests, "mips", "gcc"); - mark_skipped(&mut tests, "mips64", "gcc"); - mark_skipped(&mut tests, "mips64el", "gcc"); + mark_skipped(&mut tests, "mips64", "clang"); + mark_skipped(&mut tests, "mips64el", "clang"); mark_skipped(&mut tests, "mipsel", "gcc"); mark_skipped(&mut tests, "x86", "gcc"); + mark_skipped(&mut tests, "x86", "mingw32-gcc"); for test_case in tests { let num_expected_occurences = if test_case.is_lkm { From 54df500d58dcfb4f674bcfca19dbb13495d19a4f Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Fri, 5 Apr 2024 18:02:20 +0200 Subject: [PATCH 22/26] test/cwe252: add explanatory comment Add instructions how to interpret the acceptance tests for CWE252 to the source file. Signed-off-by: Valentin Obst --- test/artificial_samples/cwe_252.c | 37 +++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/artificial_samples/cwe_252.c b/test/artificial_samples/cwe_252.c index 8e318e94..15c1b4fc 100644 --- a/test/artificial_samples/cwe_252.c +++ b/test/artificial_samples/cwe_252.c @@ -3,6 +3,43 @@ #include #include +/* +Explanation of the general naming scheme used for functions in this test and how +to parse the comments. + +Everything between two horizontal / *---- ... ---* / lines belongs to a single +test case. If there are no `CWE_WARNING...` comments, then the test should not +cause a warning. If there are such comments, then the test case should cause +exactly one warning, where the first reported address (source) belongs to the +line with the comment that has a `1` and the second reported address (sink) +belongs to a line with a `2`. The `reason=` field in the comment should match +the one in the cwe warning. + +Now to the names: + +cwe252_ INTER_OR_INTRA STORAGE+ RETURN? USAGE + +INTER_OR_INTRA -> intra | inter_caller | inter_callee +Tells you if the test is exercising our interprocedural or intraprocedural +analysis, and if the function is the caller or callee for interprocedural tests. + +STORAGE -> r | s | h | g +Tells you where the relevant tainted values will be stored (register, stack, +heap, global). So the test checks that we properly track those locations. + +RETURN -> return | return ptr | return param | gptr +For interprocedural test cases, this tells you how the information is propagated +from the callee to the caller. Taint may be returned directly, a pointer to +taint may be returned, taint may be written to a pointer parameter, taint may be +written to a pointer that is stored in a global variable. + +USAGE -> lost | unchecked | checked +This tells you what the test does with the tainted value. Is there a point where +all taint vanishes from the state (lost)? Is the taint reaching the end of a +function without being returned (unchecked)? Or is there a check on every path +from the call site to the end of the function (checked)? +*/ + char *g_ret; char **g_ret_store; From 575fa310b875dccab52822238c4f1cdfdd15aa90 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Fri, 5 Apr 2024 18:34:21 +0200 Subject: [PATCH 23/26] test/cwe252: memory object propagation with offset Modify existing test case such that it also covers the case where a memory object from the callee must be merged into an object in the caller with a non-zero offset. Signed-off-by: Valentin Obst --- test/artificial_samples/cwe_252.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/artificial_samples/cwe_252.c b/test/artificial_samples/cwe_252.c index 15c1b4fc..7cfa7a7a 100644 --- a/test/artificial_samples/cwe_252.c +++ b/test/artificial_samples/cwe_252.c @@ -133,7 +133,7 @@ int cwe252_inter_caller_h_gptr_checked(void) static char **cwe252_inter_callee_h_return_ptr_checked(char *buf, size_t n) { - char **ret_store = malloc(8); + char **ret_store = (char**)malloc(16) + 1; if (!ret_store) return NULL; From 5bcf58fe6fbbf24c17896dd1fe7ea78f4983e504 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Mon, 8 Apr 2024 10:53:22 +0200 Subject: [PATCH 24/26] changes: remove trailing whitespace Signed-off-by: Valentin Obst --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 6df99102..c0a4a34e 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -11,7 +11,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) ==== From f155f1cacdbc1468c157e06157558e0c4d008564 Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Mon, 8 Apr 2024 10:50:42 +0200 Subject: [PATCH 25/26] changes: add addition of new check for CWE252 Signed-off-by: Valentin Obst --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index c0a4a34e..3edad677 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,7 @@ 0.9-dev === +- 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) From 12012cd8eafea4a02f55a9a330a1f799958d2f8f Mon Sep 17 00:00:00 2001 From: Valentin Obst Date: Mon, 8 Apr 2024 10:52:36 +0200 Subject: [PATCH 26/26] changes: add improvements made to `TaintAnalysis` Signed-off-by: Valentin Obst --- CHANGES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 3edad677..65ba2ab9 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,8 @@ 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)