From f138032357933fb6905f76fd05e3534bb72dc1ef Mon Sep 17 00:00:00 2001 From: Vara Prasad Bandaru Date: Mon, 6 Mar 2023 00:55:25 +0530 Subject: [PATCH 1/2] Add tests cases for reduced outputs of detectors --- tests/detectors/detector_reduced_output.py | 125 +++++++++++++++++++++ tests/test_detectors_using_pyteal.py | 5 + 2 files changed, 130 insertions(+) create mode 100644 tests/detectors/detector_reduced_output.py diff --git a/tests/detectors/detector_reduced_output.py b/tests/detectors/detector_reduced_output.py new file mode 100644 index 0000000..fc7140d --- /dev/null +++ b/tests/detectors/detector_reduced_output.py @@ -0,0 +1,125 @@ +# pylint: skip-file +# mypy: ignore-errors +from pyteal import * # pylint: disable=wildcard-import, unused-wildcard-import + +from pyteal import * + +from tealer.detectors.all_detectors import IsUpdatable, IsDeletable + +from tests.pyteal_parsing.control_flow_constructs import ( + some_subroutines, + call_all_subroutines, + approval_program as control_flow_constructs_ap, +) + +barecalls = BareCallActions( + no_op=OnCompleteAction(action=Approve(), call_config=CallConfig.CREATE), + opt_in=OnCompleteAction(action=Approve(), call_config=CallConfig.ALL), + close_out=OnCompleteAction(action=Approve(), call_config=CallConfig.CALL), + update_application=OnCompleteAction(call_config=CallConfig.NEVER), + delete_application=OnCompleteAction(call_config=CallConfig.NEVER), +) + +router = Router( + name="Example", + bare_calls=barecalls, +) + + +@router.method(no_op=CallConfig.ALL) +def echo(input: abi.Uint64, *, output: abi.Uint64) -> Expr: + """ + Method config validations Teal pattern: + txn OnCompletion + int NoOp + == + txn ApplicationID + int 0 + != + && + assert // Assert(NoOp, CALL) + """ + return output.set(input.get()) + + +@router.method(update_application=CallConfig.ALL) +def update() -> Expr: + # method entry point: B13 + # path: B0, B1, B2, B13 + + # if execution reaches this method, the application can be updated. + # Usually, detectors report all enumerated paths. + # The number of output files can be reduced by truncating the paths to a certain basic block "Bt". + # The block "Bt" has the property that, all subsequent paths leading from "Bt" are considered vulnerable. + # In this example ("IsUpdatable") should report all paths that involves executing this method. + # Instead of reporting every path, the detector will only report path upto the method entry point block. + return Seq([some_subroutines(Int(1)), Approve()]) + + +@router.method(delete_application=CallConfig.ALL) +def delete() -> Expr: + # detector output should contain + return If( + Int(2) == Int(1) + App.globalGet(Bytes("key")), + Reject(), + # IsDeletable reports if execution reach this block + # path: B0, B1, B2, B3, B11, B56, B57 + Seq([call_all_subroutines(), Approve()]), + ) + + +@router.method(update_application=CallConfig.CALL) +def update_2() -> Expr: + return Cond( + # IsUpdatable reports path for this. This is also a leaf path. + # path: B0, B1, B2, B3, B4, B9, B63, B73 + [App.globalGet(Bytes("key")), Approve()], + # IsUpdatable does not report this. + [App.globalGet(Bytes("key2")), Reject()], + # IsUpdatable reports all paths that reached this block. + # IsUpdatable should output 2 files, one for above path and another for all paths leading from this block. + # path: B0, B1, B2, B3, B4, B9, B63, B64, B65, B67 + [Int(1), call_all_subroutines(), Approve()], + ) + + +@router.method(update_application=CallConfig.CALL) +def update_3() -> Expr: + return If( + App.globalGet(Bytes("key")), + # IsUpdatable reports path for this. This is also a leaf path. + # path: B0, B1, B2, B3, B4, B5, B7, B74, B80 + Approve(), + # IsUpdatable reports all paths that reached this block. + # IsUpdatable should output 1 file that represents all paths executing this method. + # path: B0, B1, B2, B3, B4, B5, B7, B74, B75 + Seq([call_all_subroutines(), Approve()]), + ) + + +# PyTeal creates intcblock, bytecblock if assemble_constants = True +# int NoOp, OptIn, ... are all replaced by intc_* instructions. +# pragma(compiler_version="0.22.0") +approval_program, clear_state_program, contract = router.compile_program( + version=7, + assemble_constants=True, # use intcblock, bytecblock + # optimize=OptimizeOptions(scratch_slots=True), +) + +IS_UPDATABLE_VULNERABLE_PATHS = [ + [0, 1, 2, 3, 4, 5, 7, 74], # update_3(), + [0, 1, 2, 3, 4, 9, 63, 64, 65, 67], # update_2() + [0, 1, 2, 3, 4, 9, 63, 73], # update_2() + [0, 1, 2, 13], # update() +] + +IS_DELETABLE_VULNERABLE_PATHS = [[0, 1, 2, 3, 11, 56, 57]] # delete() + +reduced_output_tests = [ + (approval_program, IsUpdatable, IS_UPDATABLE_VULNERABLE_PATHS), + (approval_program, IsDeletable, IS_DELETABLE_VULNERABLE_PATHS), +] + + +if __name__ == "__main__": + print(approval_program) diff --git a/tests/test_detectors_using_pyteal.py b/tests/test_detectors_using_pyteal.py index 734f5a1..0ab12ab 100644 --- a/tests/test_detectors_using_pyteal.py +++ b/tests/test_detectors_using_pyteal.py @@ -29,12 +29,17 @@ multiple_calls_to_subroutine_tests, ) +# pylint: disable=wrong-import-position +from tests.detectors.detector_reduced_output import reduced_output_tests + + TESTS: List[Tuple[str, Type[AbstractDetector], List[List[int]]]] = [ *router_with_assembled_constants, *txn_type_based_tests, *group_size_tests_pyteal, *subroutine_recursion_patterns_tests, *multiple_calls_to_subroutine_tests, + *reduced_output_tests, ] From 74990dc083b9951d4f407440584cf59771e171ae Mon Sep 17 00:00:00 2001 From: Vara Prasad Bandaru Date: Mon, 6 Mar 2023 14:13:23 +0530 Subject: [PATCH 2/2] Reduce reported paths of detectors --- tealer/detectors/utils.py | 64 ++++++++++++++++------ tests/detectors/detector_reduced_output.py | 8 ++- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/tealer/detectors/utils.py b/tealer/detectors/utils.py index d34b10c..7067b3b 100644 --- a/tealer/detectors/utils.py +++ b/tealer/detectors/utils.py @@ -87,13 +87,13 @@ def detect_missing_tx_field_validations( Returns a list of vulnerable execution paths: none of the blocks in the path check the fields. """ - def search_paths( + def search_paths( # pylint: disable=too-many-branches bb: "BasicBlock", current_path: List["BasicBlock"], paths_without_check: List[List["BasicBlock"]], current_call_stack: List[Tuple[Optional["BasicBlock"], "Subroutine"]], current_subroutine_executed: List[List["BasicBlock"]], - ) -> None: + ) -> bool: """ Args: current_call_stack: list of callsub blocks and called subroutine along the current path. @@ -162,19 +162,24 @@ def search_paths( bb = B1, current_path = [B0, B3, B5, B4, B1], current_call_stack = [(None, Main)], current_subroutine_executed = [[B0, B1]] .... + + Returns: + Returns True if ALL child paths from the path `current_path + [bb]` are vulnerable. Otherwise, returns False. + The return value is used to determine whether to add the path to `paths_without_check`. if all child paths are vulnerable, the path is not added. + If only some of the child paths are vulnerable, add the individual vulnerable paths. """ # check for loops if bb in current_subroutine_executed[-1]: logger_detectors.debug( f"Loop Path: current_full_path = {current_path}\n, current_subroutine_executed = {current_subroutine_executed[-1]}, current_block: {repr(bb)}" ) - return + return True if validated_in_block(bb, checks_field): logger_detectors.debug( f"Validated Path: current_full_path = {current_path}\n, current_block: {repr(bb)}" ) - return + return False current_path = current_path + [bb] @@ -186,8 +191,9 @@ def search_paths( logger_detectors.debug( f"Vulnerable Path Satisfied Report Condition: current_full_path = {current_path} " ) - paths_without_check.append(current_path) - return + # for leaf path, all child paths are vulnerable + return True + return False # do we need to make a copy of lists in [:-1]??? current_subroutine_executed = current_subroutine_executed[:-1] + [ @@ -201,7 +207,7 @@ def search_paths( already_called_subroutines = [frame[1] for frame in current_call_stack] if called_subroutine in already_called_subroutines: # recursion - return + return True current_call_stack = current_call_stack + [(bb, called_subroutine)] current_subroutine_executed = current_subroutine_executed + [[]] @@ -213,25 +219,47 @@ def search_paths( assert callsub_block is not None return_point = callsub_block.sub_return_point if return_point is not None and return_point in bb.next: - search_paths( + all_child_paths_vulnerable = search_paths( return_point, current_path, paths_without_check, current_call_stack[:-1], # returning from a subroutine current_subroutine_executed[:-1], ) - else: - for next_bb in bb.next: - search_paths( - next_bb, - current_path, - paths_without_check, - current_call_stack, - current_subroutine_executed, - ) + return all_child_paths_vulnerable + return True + + vulnerable_child_paths: List["BasicBlock"] = [] + all_child_paths_vulnerable = True + for next_bb in bb.next: + child_path_is_vulnerable = search_paths( + next_bb, + current_path, + paths_without_check, + current_call_stack, + current_subroutine_executed, + ) + if not child_path_is_vulnerable: + # one of the child path is not vulnerable. + all_child_paths_vulnerable = False + else: + vulnerable_child_paths.append(next_bb) + + if all_child_paths_vulnerable: + return True + + # All of the child paths from the "current_path" are not vulnerable. So, add individual vulnerable paths + for next_bb in vulnerable_child_paths: + print("current", current_path, f"{repr(next_bb)}") + paths_without_check.append(current_path + [next_bb]) + return False teal = entry_block.teal assert teal is not None paths_without_check: List[List["BasicBlock"]] = [] - search_paths(entry_block, [], paths_without_check, [(None, teal.main)], [[]]) + all_child_paths_vulnerable = search_paths( + entry_block, [], paths_without_check, [(None, teal.main)], [[]] + ) + if all_child_paths_vulnerable: + paths_without_check.append([entry_block]) return paths_without_check diff --git a/tests/detectors/detector_reduced_output.py b/tests/detectors/detector_reduced_output.py index fc7140d..6655c00 100644 --- a/tests/detectors/detector_reduced_output.py +++ b/tests/detectors/detector_reduced_output.py @@ -107,10 +107,14 @@ def update_3() -> Expr: ) IS_UPDATABLE_VULNERABLE_PATHS = [ - [0, 1, 2, 3, 4, 5, 7, 74], # update_3(), + [0, 1, 2, 3, 4, 5, 7], # update_3(), [0, 1, 2, 3, 4, 9, 63, 64, 65, 67], # update_2() [0, 1, 2, 3, 4, 9, 63, 73], # update_2() - [0, 1, 2, 13], # update() + # below three paths are not reduced to [0, 1, 2, 13] because + # [0, 1, 2, 13, 45, 46, 47, 48] is a child path and is not vulnerable + [0, 1, 2, 13, 45, 46, 47, 49], # update() + [0, 1, 2, 13, 45, 46, 51], # update() + [0, 1, 2, 13, 45, 53], # update() ] IS_DELETABLE_VULNERABLE_PATHS = [[0, 1, 2, 3, 11, 56, 57]] # delete()