diff --git a/rop3/gadget.py b/rop3/gadget.py index d67bd33..d2b6488 100644 --- a/rop3/gadget.py +++ b/rop3/gadget.py @@ -61,7 +61,7 @@ def has_src(self) -> bool: def calculate_side_effects(self) -> None: arch = arch_singleton.arch - excluded = {arch.normalize_reg(r) for r in (self.dst, self.src, arch.sp) if r is not None} + excluded = {arch.normalize_reg(r) for r in (self.dst, arch.sp) if r is not None} for decode in self.decodes: explicit = {decode.reg_name(r) for r in decode.regs_write} diff --git a/rop3/operation.py b/rop3/operation.py index 45792bc..64b1c16 100644 --- a/rop3/operation.py +++ b/rop3/operation.py @@ -249,9 +249,16 @@ def is_dst(self): def set_dst(self, dst): if self.is_dst(): - self.reg = dst self.generic = False - self.type = arch_singleton.arch.op_reg + if self.is_mem(): + self.reg = dst + else: + try: + self.imm = self._parse_imm(dst) + self.type = arch_singleton.arch.op_imm + except (ValueError, TypeError): + self.reg = dst + self.type = arch_singleton.arch.op_reg def is_src(self): if self.reg is not None: @@ -261,17 +268,21 @@ def is_src(self): def set_src(self, src): if self.is_src(): self.generic = False - try: - self.imm = self._parse_imm(src) - self.type = arch_singleton.arch.op_imm - except (ValueError, TypeError): + if self.is_mem(): self.reg = src - self.type = arch_singleton.arch.op_reg + else: + try: + self.imm = self._parse_imm(src) + self.type = arch_singleton.arch.op_imm + except (ValueError, TypeError): + self.reg = src + self.type = arch_singleton.arch.op_reg def is_equal(self, decode, operand): operand_reg = None - if self.generic and self.is_src() and operand.type == arch_singleton.arch.op_imm: + # Allows generic reg -> imm substitution (not for mem) + if self.generic and self.is_src() and self.is_reg() and operand.type == arch_singleton.arch.op_imm: return (True, operand.value.imm) if self.type != operand.type: diff --git a/rop3/ropchain.py b/rop3/ropchain.py index 4063ee5..59ff4ef 100644 --- a/rop3/ropchain.py +++ b/rop3/ropchain.py @@ -234,9 +234,19 @@ def backtrack( for gad in comb_gadgets[index]: for side_reg in gad.side_regs: side_effected[side_reg] += 1 + + norm_dst = arch.normalize_reg(gad.dst) if gad.dst else None + saved_dst = side_effected[norm_dst] if norm_dst else 0 + if saved_dst: + side_effected[norm_dst] = 0 + ropchain.append(gad) yield from backtrack(index + 1, ropchain, side_effected) ropchain.pop() + + if saved_dst: + side_effected[norm_dst] = saved_dst + for side_reg in gad.side_regs: side_effected[side_reg] -= 1 diff --git a/rop3/roplang/mov.yaml b/rop3/roplang/mov.yaml index 7297780..fae5504 100644 --- a/rop3/roplang/mov.yaml +++ b/rop3/roplang/mov.yaml @@ -44,18 +44,18 @@ mov: op2: src # clc - # cmovc dst, src - - + # cmovae dst, src + - - mnemonic: clc - - mnemonic: cmovc + - mnemonic: cmovae op1: dst op2: src # stc - # cmovc dst, src + # cmovb dst, src - - mnemonic: stc - - mnemonic: cmovc + - mnemonic: cmovb op1: dst op2: src diff --git a/tests/test_operation.py b/tests/test_operation.py index 6826a6f..7bba3d5 100644 --- a/tests/test_operation.py +++ b/tests/test_operation.py @@ -65,3 +65,81 @@ def test_operand_parse_imm_supports_hex_and_negative(x64): assert op._parse_imm('0xffffffff') == 0xffffffff assert op._parse_imm('-1') == -1 assert op._parse_imm(42) == 42 + + +def test_ld_with_src_matches_memory_not_register(x64): + ''' + Regression (#30, #33): `ld` (mov dst, [src]) with a concrete --src must + match a memory load `mov , [src]`, not a register move `mov , src`. + A previous bug overwrote the memory operand type with op_reg in set_src. + ''' + gadgets = [ + make_gadget(b'\x48\x8b\x03\xc3', 0x1000), # mov rax, [rbx] ; ret + make_gadget(b'\x48\x89\xd8\xc3', 0x1010), # mov rax, rbx ; ret (must NOT match) + ] + matched = operation.Operation('ld', src='rbx').filter_gadgets(gadgets) + assert [g.text_repr for g in matched] == ['mov rax, qword ptr [rbx] ; ret'] + + +def test_ld_does_not_match_immediate_load(x64): + ''' + Regression (#33, error 3): a generic memory address must not be resolved + into an immediate, so `mov rax, 0xcafe` is not a valid `ld` (load). + ''' + gadgets = [ + make_gadget(b'\x48\xc7\xc0\xfe\xca\x00\x00\xc3', 0x1000), # mov rax, 0xcafe ; ret + ] + assert operation.Operation('ld').filter_gadgets(gadgets) == [] + + +def test_set_dst_preserves_memory_type(x64): + ''' + Regression (#33, error 1): binding a concrete register to a `[dst]` + placeholder must keep the operand a memory operand, not turn it into a reg. + ''' + op = operation.Operand('[dst]') + assert op.is_mem() + op.set_dst('rax') + assert op.is_mem() + assert op.reg == 'rax' + + +def test_set_src_preserves_memory_type(x64): + ''' Regression (#33, error 1): same as above for the `[src]` placeholder. ''' + op = operation.Operand('[src]') + assert op.is_mem() + op.set_src('rbx') + assert op.is_mem() + assert op.reg == 'rbx' + + +def test_set_dst_accepts_immediate(x64): + ''' + Regression (#33, error 2): set_dst must accept immediates like set_src, + producing an op_imm operand rather than rejecting the value. + ''' + op = operation.Operand('dst') + op.set_dst('0x10') + assert op.is_imm() + assert op.imm == 0x10 + + +def test_xchg_src_counted_as_side_effect(x64): + ''' + Regression (#31): in `xchg dst, src` the `src` register is clobbered, so it + must be reported as a side effect (it was wrongly excluded before). + ''' + gadget = make_gadget(b'\x48\x93\xc3', 0x1000) # xchg rbx, rax ; ret + matched = operation.Operation('mov', dst='rbx', src='rax').filter_gadgets([gadget]) + assert len(matched) == 1 + assert 'rax' in matched[0].side_regs + + +def test_mov_matches_clc_cmovae(x64): + ''' + Regression (#32): the mov ROPLang uses the valid Capstone mnemonics + `cmovae`/`cmovb` (not `cmovc`), so `clc ; cmovae dst, src` is a valid mov. + ''' + gadget = make_gadget(b'\xf8\x48\x0f\x43\xc3\xc3', 0x1000) # clc ; cmovae rax, rbx ; ret + matched = operation.Operation('mov', dst='rax', src='rbx').filter_gadgets([gadget]) + assert [g.text_repr for g in matched] == ['clc ; cmovae rax, rbx ; ret'] diff --git a/tests/test_ropchain.py b/tests/test_ropchain.py index 738f408..b1a06f0 100644 --- a/tests/test_ropchain.py +++ b/tests/test_ropchain.py @@ -65,3 +65,32 @@ def test_search_generic_registers(x64): results = list(RopChain(None).search(gadgets, chain)) assert results assert all(len(r) == 1 for r in results) + + +def test_explicit_dst_clears_clobbered_register(x64): + ''' + Regression (#34): a register written by a later step must no longer be + considered clobbered by an earlier step. + + Step 1 `lc(rax)` uses `pop rax ; pop rbx ; ret`, which clobbers rbx. + Step 2 `lc(rbx)` rewrites rbx, so it must be clean again for step 3 + `mov(rdx, rbx)`, which reads rbx. Before the fix the stale clobber on rbx + blocked step 3 and no chain was found. + ''' + gadgets = [ + make_gadget(b'\x58\x5b\xc3', 0x1000), # pop rax ; pop rbx ; ret + make_gadget(b'\x5b\xc3', 0x1010), # pop rbx ; ret + make_gadget(b'\x48\x89\xda\xc3', 0x1020), # mov rdx, rbx ; ret + ] + chain = [ + _op('lc', dst='rax'), + _op('lc', dst='rbx'), + _op('mov', dst='rdx', src='rbx'), + ] + results = list(RopChain(None).search(gadgets, chain)) + assert results + assert [g.text_repr for g in results[0]] == [ + 'pop rax ; pop rbx ; ret', + 'pop rbx ; ret', + 'mov rdx, rbx ; ret', + ]