From 0c99be061a3019093157f0d71745f80adb62822a Mon Sep 17 00:00:00 2001 From: Damian Pfammatter Date: Wed, 29 Jan 2025 10:37:30 +0100 Subject: [PATCH] 28 incorrect slicing of function parameters (#30) - Fixes issue #28. - Clear source's target instruction when reanalyzing a binary - Remove `is_definition` attribute - Added unit-tests --- mole/core/data.py | 1 + mole/core/slice.py | 193 ++++++++++++++++++--------------- test/src/function_calling-05.c | 28 +++++ test/src/function_calling-06.c | 29 +++++ test/src/function_calling-07.c | 28 +++++ test/src/function_calling-08.c | 29 +++++ test/test_slicing.py | 75 +++++++++++++ 7 files changed, 297 insertions(+), 86 deletions(-) create mode 100644 test/src/function_calling-05.c create mode 100644 test/src/function_calling-06.c create mode 100644 test/src/function_calling-07.c create mode 100644 test/src/function_calling-08.c diff --git a/mole/core/data.py b/mole/core/data.py index 9fc47c0..9e161e7 100644 --- a/mole/core/data.py +++ b/mole/core/data.py @@ -178,6 +178,7 @@ class SourceFunction(Function): """ This method finds a set of target instructions that a static backward slice should hit on. """ + self.target_insts.clear() code_refs = SymbolHelper.get_code_refs( bv, self.symbols, diff --git a/mole/core/slice.py b/mole/core/slice.py index 28255ca..0124f94 100644 --- a/mole/core/slice.py +++ b/mole/core/slice.py @@ -26,20 +26,16 @@ class MediumLevelILInstructionGraph(nx.DiGraph): def add_node( self, inst: bn.MediumLevelILInstruction, - is_definition: bool = None, call_level: int = None, caller_site: bn.MediumLevelILFunction = None ) -> None: """ This method adds a node for the given instruction `inst` with the following node attributes: - The attribute `is_definition` should indicate whether or not instruction `inst` defines a - SSA variable, i.e. represents an assigning instruction. The attribute `call_level` should - indicate the `inst`'s level within the call stack. The attribute `caller_site` should - indicate the function that called `inst.function`. + The attribute `call_level` is expected to be `inst`'s level within the call stack. The + attribute `caller_site` is expected to be the function that called `inst.function`. """ super().add_node( inst, - is_definition=is_definition, call_level=call_level, caller_site=caller_site ) @@ -47,27 +43,27 @@ class MediumLevelILInstructionGraph(nx.DiGraph): def add_edge( self, - inst_from: bn.MediumLevelILInstruction, - inst_to: bn.MediumLevelILInstruction + from_inst: bn.MediumLevelILInstruction, + to_inst: bn.MediumLevelILInstruction ) -> None: """ - This method adds an edge from `inst_from` to `inst_to`. + This method adds an edge from `from_inst` to `to_inst`. """ - if not inst_from in self.nodes: - info = InstructionHelper.get_inst_info(inst_from) + if not from_inst in self.nodes: + info = InstructionHelper.get_inst_info(from_inst) self._log.warn( self._tag, f"Edge not added to instruction graph due to an inexisting from node ({info:s})" ) return - if not inst_to in self.nodes: - info = InstructionHelper.get_inst_info(inst_to) + if not to_inst in self.nodes: + info = InstructionHelper.get_inst_info(to_inst) self._log.warn( self._tag, f"Edge not added to instruction graph due to an inexisting to node ({info:s})" ) return - super().add_edge(inst_from, inst_to) + super().add_edge(from_inst, to_inst) return @@ -96,7 +92,7 @@ class MediumLevelILFunctionGraph(nx.DiGraph): ) -> None: """ This method adds a node for the given `call_site`, with the following node attribute: The - attribute `call_level` should indicate the `call_site`'s level within the call stack. + attribute `call_level` is expected to be the `call_site`'s level within the call stack. """ super().add_node( call_site, @@ -106,27 +102,27 @@ class MediumLevelILFunctionGraph(nx.DiGraph): def add_edge( self, - call_site_from: bn.MediumLevelILFunction, - call_site_to: bn.MediumLevelILFunction, + from_call_site: bn.MediumLevelILFunction, + to_call_site: bn.MediumLevelILFunction, ) -> None: """ - This method adds an edge from `call_site_from` to `call_site_to`. + This method adds an edge from `from_call_site` to `to_call_site`. """ - if not call_site_from in self.nodes: - info = FunctionHelper.get_func_info(call_site_from) + if not from_call_site in self.nodes: + info = FunctionHelper.get_func_info(from_call_site) self._log.warn( self._tag, f"Edge not added to function graph due to an inexisting from node ({info:s})" ) return - if not call_site_to in self.nodes: - info = FunctionHelper.get_func_info(call_site_to) + if not to_call_site in self.nodes: + info = FunctionHelper.get_func_info(to_call_site) self._log.warn( self._tag, f"Edge not added to function graph due to an inexisting to node ({info:s})" ) return - super().add_edge(call_site_from, call_site_to) + super().add_edge(from_call_site, to_call_site) return @@ -162,44 +158,48 @@ class MediumLevelILBackwardSlicer: caller_site: bn.MediumLevelILFunction = None ) -> None: """ - This method first tries to find the instruction defining variable `ssa_var` within `inst`'s - function. If it is found, slicing proceeds at the identified defining instruction. If no - defining instruction is found, it is checked whether `ssa_var` belongs to a function - argument, and if so, defining instructions are searched within the callers (only if we go - down the call stack). Slicing then proceeds in all found defining instructions. + This method first tries to find the instruction defining variable `ssa_var` within + `inst.function`. If it is found, slicing proceeds at the identified defining instruction. If + no defining instruction is found, the method distinguishes whether we went up + (caller_level <= call_level) or down (caller_level > call_level) the call stack. If we went + up, we know from which caller we came from and can proceed only this single caller site. If + we went down, we don't know this and need to follow all caller sites. """ # Try finding the definition withing the current function inst_def = inst.function.get_ssa_var_definition(ssa_var) if inst_def: - self._inst_graph.add_node(inst, False, call_level, caller_site) - self._inst_graph.add_node(inst_def, True, call_level, caller_site) + self._inst_graph.add_node(inst, call_level, caller_site) + self._inst_graph.add_node(inst_def, call_level, caller_site) self._inst_graph.add_edge(inst, inst_def) self._slice_backwards(inst_def, call_level, caller_site) return - # Try finding the definition in another function if we go down the call stack + # Try finding the definition in another function if abs(call_level) > self._max_call_level and self._max_call_level >= 0: return - caller_site_level = self._call_graph.nodes.get(caller_site, {}).get("call_level", None) - if caller_site_level is None or call_level < caller_site_level: - for parm_idx, parm_var in enumerate(inst.function.source_function.parameter_vars): - if parm_var != ssa_var.var: continue - for cs in inst.function.source_function.caller_sites: - try: - inst_caller: bn.MediumLevelILInstruction = cs.mlil.ssa_form - parm_caller: bn.MediumLevelILInstruction = inst_caller.params[parm_idx] - func_caller: bn.MediumLevelILFunction = parm_caller.function - self._log.debug( - self._tag, - f"[{call_level:+d}] Follow '{ssa_var.name}#{ssa_var.version}' to caller '0x{inst_caller.address:x}: {str(inst_caller):s}'" - ) - self._inst_graph.add_node(inst, False, call_level, caller_site) - self._inst_graph.add_node(parm_caller, False, call_level-1, inst.function) - self._inst_graph.add_edge(inst, parm_caller) - self._call_graph.add_node(inst.function, call_level) - self._call_graph.add_node(func_caller, call_level-1) - self._call_graph.add_edge(inst.function, func_caller) - self._slice_backwards(parm_caller, call_level-1, inst.function) - except: - continue + caller_level = self._call_graph.nodes.get(caller_site, {}).get("call_level", None) + for parm_idx, parm_var in enumerate(inst.function.source_function.parameter_vars): + if parm_var != ssa_var.var: continue + for cs in inst.function.source_function.caller_sites: + try: + cs_inst = cs.mlil.ssa_form + cs_parm = cs_inst.params[parm_idx] + # Visit specific caller site if we go up the call stack (all caller sites otherwise) + if not caller_level is None and caller_level <= call_level: + if caller_site != cs_inst.function: + continue + cs_info = InstructionHelper.get_inst_info(cs_inst, False) + self._log.debug( + self._tag, + f"Follow parameter '{ssa_var.name}#{ssa_var.version}' to caller '{cs_info:s}'" + ) + self._inst_graph.add_node(inst, call_level, caller_site) + self._inst_graph.add_node(cs_parm, call_level-1, inst.function) + self._inst_graph.add_edge(inst, cs_parm) + self._call_graph.add_node(cs_inst.function, call_level-1) + self._call_graph.add_node(inst.function, call_level) + self._call_graph.add_edge(cs_inst.function, inst.function) + self._slice_backwards(cs_parm, call_level-1, inst.function) + except: + continue return def _slice_backwards( @@ -209,7 +209,9 @@ class MediumLevelILBackwardSlicer: caller_site: bn.MediumLevelILFunction = None ) -> None: """ - This method backward slices instruction `inst` based on its type. + This method backward slices instruction `inst` based on its type. Parameter `call_level` is + expected to be `inst`'s level within the call stack. Parameter `caller_site` is expected to + be the function that called `inst.function`. """ info = InstructionHelper.get_inst_info(inst) # Instruction sliced before @@ -248,8 +250,8 @@ class MediumLevelILBackwardSlicer: bn.MediumLevelILLowPart() | bn.MediumLevelILFneg() | bn.MediumLevelILFloatConv()): - self._inst_graph.add_node(inst, False, call_level, caller_site) - self._inst_graph.add_node(inst.src, None, call_level, caller_site) + self._inst_graph.add_node(inst, call_level, caller_site) + self._inst_graph.add_node(inst.src, call_level, caller_site) self._inst_graph.add_edge(inst, inst.src) self._slice_backwards(inst.src, call_level, caller_site) case (bn.MediumLevelILAdd() | @@ -274,18 +276,18 @@ class MediumLevelILBackwardSlicer: bn.MediumLevelILFsub() | bn.MediumLevelILFmul() | bn.MediumLevelILFdiv()): - self._inst_graph.add_node(inst, False, call_level, caller_site) - self._inst_graph.add_node(inst.left, None, call_level, caller_site) + self._inst_graph.add_node(inst, call_level, caller_site) + self._inst_graph.add_node(inst.left, call_level, caller_site) self._inst_graph.add_edge(inst, inst.left) self._slice_backwards(inst.left, call_level, caller_site) - self._inst_graph.add_node(inst, False, call_level, caller_site) - self._inst_graph.add_node(inst.right, None, call_level, caller_site) + self._inst_graph.add_node(inst, call_level, caller_site) + self._inst_graph.add_node(inst.right, call_level, caller_site) self._inst_graph.add_edge(inst, inst.right) self._slice_backwards(inst.right, call_level, caller_site) case (bn.MediumLevelILRet()): for ret in inst.src: - self._inst_graph.add_node(inst, False, call_level, caller_site) - self._inst_graph.add_node(ret, None, call_level, caller_site) + self._inst_graph.add_node(inst, call_level, caller_site) + self._inst_graph.add_node(ret, call_level, caller_site) self._inst_graph.add_edge(inst, ret) self._slice_backwards(ret, call_level, caller_site) case (bn.MediumLevelILSetVarSsa() | @@ -293,8 +295,8 @@ class MediumLevelILBackwardSlicer: bn.MediumLevelILSetVarAliasedField() | bn.MediumLevelILSetVarSsaField() | bn.MediumLevelILSetVarSplitSsa()): - self._inst_graph.add_node(inst, True, call_level, caller_site) - self._inst_graph.add_node(inst.src, None, call_level, caller_site) + self._inst_graph.add_node(inst, call_level, caller_site) + self._inst_graph.add_node(inst.src, call_level, caller_site) self._inst_graph.add_edge(inst, inst.src) self._slice_backwards(inst.src, call_level, caller_site) case (bn.MediumLevelILVarPhi()): @@ -306,45 +308,64 @@ class MediumLevelILBackwardSlicer: match dest_inst: case (bn.MediumLevelILConstPtr(constant=func_addr) | bn.MediumLevelILImport(constant=func_addr)): - # Backward slice into functions defined within the binary try: func = self._bv.get_function_at(func_addr).mlil.ssa_form + func_info = FunctionHelper.get_func_info(func, False) + symb = func.source_function.symbol for func_inst in func.instructions: + # TODO: Support all return instructions match func_inst: - # TODO: Support all return instructions - # Backward slice starting from possible return instructions case (bn.MediumLevelILRet() | - bn.MediumLevelILTailcallSsa()): + bn.MediumLevelILTailcallSsa()): if ( - self._max_call_level < 0 or - (self._max_call_level != 0 and abs(call_level) < self._max_call_level) + self._max_call_level < 0 or + ( + abs(call_level) < self._max_call_level and + self._max_call_level != 0 + ) ): - self._inst_graph.add_node(inst, True, call_level, caller_site) - self._inst_graph.add_node(func_inst, None, call_level+1, inst.function) - self._inst_graph.add_edge(inst, func_inst) - self._call_graph.add_node(inst.function, call_level) - self._call_graph.add_node(func, call_level+1) - self._call_graph.add_edge(inst.function, func) - self._slice_backwards(func_inst, call_level+1, inst.function) + # Function + if symb.type == bn.SymbolType.FunctionSymbol: + ret_info = InstructionHelper.get_inst_info(func_inst, False) + self._log.debug( + self._tag, + f"Follow return instruction '{ret_info:s}' of function '{func_info:s}'" + ) + self._inst_graph.add_node(inst, call_level, caller_site) + self._inst_graph.add_node(func_inst, call_level+1, inst.function) + self._inst_graph.add_edge(inst, func_inst) + self._call_graph.add_node(inst.function, call_level) + self._call_graph.add_node(func, call_level+1) + self._call_graph.add_edge(inst.function, func) + self._slice_backwards(func_inst, call_level+1, inst.function) + # Imported function + elif symb.type == bn.SymbolType.ImportedFunctionSymbol: + for par in inst.params: + par_info = InstructionHelper.get_inst_info(par, False) + self._log.debug( + self._tag, + f"Follow parameter '{par_info:s}' of imported function '{func_info:s}'" + ) + self._inst_graph.add_node(inst, call_level, caller_site) + self._inst_graph.add_node(par, call_level, caller_site) + self._inst_graph.add_edge(inst, par) + self._slice_backwards(par, call_level, caller_site) + else: + self._log.warn(self._tag, f"Function '{func_info:s}' has an uexpected type '{str(symb.type):s}'") else: self._log.debug( self._tag, - f"[{call_level:+d}] {dest_info:s}: Maximum function depth {self._max_call_level:d} reached" + f"[{call_level:+d}] {dest_info:s}: Maximum call level {self._max_call_level:d} reached" ) except: + # Function not found within the binary pass case _: self._log.warn(self._tag, f"[{call_level:+d}] {dest_info:s}: Missing handler") - # Backward slice function parameters - for par in inst.params: - self._inst_graph.add_node(inst, True, call_level, caller_site) - self._inst_graph.add_node(par, None, call_level, caller_site) - self._inst_graph.add_edge(inst, par) - self._slice_backwards(par, call_level, caller_site) case (bn.MediumLevelILSyscallSsa()): for par in inst.params: - self._inst_graph.add_node(inst, False, call_level, caller_site) - self._inst_graph.add_node(par, None, call_level, caller_site) + self._inst_graph.add_node(inst, call_level, caller_site) + self._inst_graph.add_node(par, call_level, caller_site) self._inst_graph.add_edge(inst, par) self._slice_backwards(par, call_level, caller_site) case _: diff --git a/test/src/function_calling-05.c b/test/src/function_calling-05.c new file mode 100644 index 0000000..6a25fc9 --- /dev/null +++ b/test/src/function_calling-05.c @@ -0,0 +1,28 @@ +#include +#include +#include + +/* +Testcase Description: +- allow function inlining +- do not follow all function parameters blindly (positive) +*/ + +char* func(char* env) { + size_t len = strlen(env); + char *cmd = (char *) malloc(len+1); + for(int i = 0; i +#include +#include + +/* +Testcase Description: +- disallow function inlining +- do not follow all function parameters blindly (positive) +*/ + +__attribute__ ((noinline)) +char* func(char* env) { + size_t len = strlen(env); + char *cmd = (char *) malloc(len+1); + for(int i = 0; i +#include +#include + +/* +Testcase Description: +- allow function inlining +- do not follow all function parameters blindly (negative) +*/ + +char* func(char* env) { + char *cmd = (char *) malloc(4); + cmd[0] = 'l'; + cmd[1] = 's'; + cmd[2] = '\0'; + cmd[3] = '\0'; + fprintf(stdout, "--- FUN ---\n"); + fprintf(stdout, "env: '%s'\ncmd: '%s'\n", env, cmd); + return cmd; +} + +int main(int argc, char *argv[]) { + char *env, *cmd; + env = getenv("CMD"); + cmd = func(env); + system(cmd); + return EXIT_SUCCESS; +} \ No newline at end of file diff --git a/test/src/function_calling-08.c b/test/src/function_calling-08.c new file mode 100644 index 0000000..48f1406 --- /dev/null +++ b/test/src/function_calling-08.c @@ -0,0 +1,29 @@ +#include +#include +#include + +/* +Testcase Description: +- disallow function inlining +- do not follow all function parameters blindly (negative) +*/ + +__attribute__ ((noinline)) +char* func(char* env) { + char *cmd = (char *) malloc(4); + cmd[0] = 'l'; + cmd[1] = 's'; + cmd[2] = '\0'; + cmd[3] = '\0'; + fprintf(stdout, "--- FUN ---\n"); + fprintf(stdout, "env: '%s'\ncmd: '%s'\n", env, cmd); + return cmd; +} + +int main(int argc, char *argv[]) { + char *env, *cmd; + env = getenv("CMD"); + cmd = func(env); + system(cmd); + return EXIT_SUCCESS; +} \ No newline at end of file diff --git a/test/test_slicing.py b/test/test_slicing.py index b229789..934b3b2 100644 --- a/test/test_slicing.py +++ b/test/test_slicing.py @@ -294,6 +294,81 @@ class TestFunctionCalling(TestCase): ) -> None: self.test_02(filenames) return + + def test_05( + self, + filenames: List[str] = ["function_calling-05"] + ) -> None: + for file in load_files(filenames): + # Load and analyze test binary with Binary Ninja + bv = bn.load(file) + bv.update_analysis_and_wait() + # Analyze test binary + paths = self.ctr.analyze_binary(bv, max_call_level=3, enable_all_funs=True) + # Assert results + self.assertTrue(len(paths) == 1, "1 path identified") + path = paths[0] + self.assertIn(path.src_sym_name, ["getenv"], "source has symbol 'getenv'") + self.assertTrue( + isinstance(path.insts[-1], bn.MediumLevelILInstruction), + "source is a MLIL instruction" + ) + self.assertIn(path.snk_sym_name, ["system"], "sink has symbol 'system'") + self.assertTrue( + ( + isinstance(path.insts[0], bn.MediumLevelILCallSsa) or + isinstance(path.insts[0], bn.MediumLevelILTailcallSsa) + ), + "sink is a MLIL call instruction" + ) + self.assertEqual(path.snk_par_idx, 0, "arg1") + self.assertTrue( + isinstance(path.snk_par_var, bn.MediumLevelILVarSsa), + "argument is a MLIL variable" + ) + calls = [path.snk_sym_name] + for inst in path.insts: + call = inst.function.source_function.name + if calls[-1] != call: + calls.append(call) + calls.append(path.src_sym_name) + self.assertEqual( + calls, + ["system", "main", "func", "main", "getenv"] + ) + # Close test binary + bv.file.close() + return + + def test_06( + self, + filenames: List[str] = ["function_calling-06"] + ) -> None: + self.test_05(filenames) + return + + def test_07( + self, + filenames: List[str] = ["function_calling-07"] + ) -> None: + for file in load_files(filenames): + # Load and analyze test binary with Binary Ninja + bv = bn.load(file) + bv.update_analysis_and_wait() + # Analyze test binary + paths = self.ctr.analyze_binary(bv, max_call_level=3, enable_all_funs=True) + # Assert results + self.assertTrue(len(paths) == 0, "0 paths identified") + # Close test binary + bv.file.close() + return + + def test_08( + self, + filenames: List[str] = ["function_calling-08"] + ) -> None: + self.test_07(filenames) + return class TestPointerAnalysis(TestCase):