Vector35 / binaryninja-api

Public API, examples, documentation and issues for Binary Ninja
https://binary.ninja/
MIT License
944 stars 213 forks source link

jump(pop) with dataflow resolved address does not update stack #1931

Open CouleeApps opened 4 years ago

CouleeApps commented 4 years ago

When binja is configured to lift return instructions as jump(pop), dataflow can resolve the address of the pop but does modify the stack. Lifting returns as esp = esp + 4 ; jmp *(esp - 4) resolves correctly.

Current behavior: image

Expected behavior:
After the first ret, the stack is popped and the second ret resolves to the next address pushed onto the stack, resolving an indirect jump, and continuing the chain.

Binary: asmtest.zip Arch hook for x86:

from binaryninja import *

class X86RetHook(ArchitectureHook):

    def __init__(self, base_arch):
        super(X86RetHook, self).__init__(base_arch)

    def get_instruction_info(self, data, addr):
        info = super(X86RetHook, self).get_instruction_info(data, addr)
        branches = info.branches
        # So dataflow doesn't stop at a return
        for b in branches:
            if b.type == BranchType.FunctionReturn:
                b.type = BranchType.UnresolvedBranch
        info.branches = branches
        return info

    def get_instruction_low_level_il(self, data, addr, il: LowLevelILFunction):
        is_ret = data[0] == 0xc3  # ret
        if is_ret:
            # Produces buggy behavior shown above
            il.append(il.jump(il.pop(il.arch.address_size)))

            # Produces correct dataflow
            # aas = il.arch.address_size
            # reg = "esp" if aas == 4 else "rsp"
            # il.append(il.set_reg(aas, reg,
            #                      il.add(aas,
            #                             il.reg(aas, reg),
            #                             il.const(aas, aas))))
            # il.append(il.jump(il.load(aas,
            #                           il.sub(aas,
            #                                  il.reg(aas, reg),
            #                                  il.const(aas, aas)))))
            return 1
        else:
            return super(X86RetHook, self).get_instruction_low_level_il(data, addr, il)

X86RetHook(Architecture['x86']).register()
CouleeApps commented 4 years ago

And for reference, here is what the alternative lifting for ret produces: image

joshwatson commented 4 years ago

It would be cool if this were fixed, I've asked about it a few times now. Current workaround is to lift it as

temp0 = pop
jump(temp0)
yrp604 commented 4 years ago

Branches have very similar behavior: https://github.com/Vector35/binaryninja-api/issues/1908