rizin icon indicating copy to clipboard operation
rizin copied to clipboard

Incorrect local variable matching

Open k-badz opened this issue 2 years ago • 5 comments

Environment information

  • Operating System: FLARE-VM
  • Cutter version: 2.1.2
  • Obtained from: FLARE-VM
  • File format:

Describe the bug

In screenshots below, Cutter didn't match local variables correctly (lpFilename and var_60h_2), both of them have the same state of ESP, and are the same instructions. Their memory operand is [esp+60h].

You can see that Cutter assigned different stack offset for these (difference of 0xC). It looks like it didn't properly recognized GetModuleFileNameW calling convention and assumed that ESP should be cleaned by caller.

To Reproduce

Steps to reproduce the behavior:

  1. Open https://github.com/HuskyHacks/PMAT-labs/blob/main/labs/2-1.AdvancedStaticAnalysis/Dropper.DownloadFromURL.exe.malz/Dropper.DownloadFromURL.exe.7z/
  2. Navigate to "main" function.
  3. Check the offsets from screenshots.

Expected behavior

lpFilename and var_60h_2 should be considered the same local variable.

Both IDA Free and Ghidra do that properly.

Screenshots

Cutter: image image

Ghidra: image

IDA Free: image

k-badz avatar Jan 09 '23 23:01 k-badz

It should be fixed already in the latest dev branch. Could you please try it?

XVilka avatar Jan 09 '23 23:01 XVilka

@XVilka Just took latest dev, rebuilt it and it seems to still be there: image image

k-badz avatar Jan 10 '23 10:01 k-badz

@XVilka hello, just FYI - I took latest Cutter 2.2.0 release to check stack analysis fixes: image

Unfortunately, it seems issue is still there (12 bytes offset from 3 parameters being pushed to stack, like it didn't recognize proper stdcall calling convention): image image

k-badz avatar Mar 06 '23 22:03 k-badz

I think I discussed this with @thestr4ng3r already. We should consider the calling convention and perform the stack analysis in the calling order.

ret2libc avatar Mar 07 '23 07:03 ret2libc

Maybe it works only for statically linked functions as of now? I saw that you can manually change the convention for these, but dynamically linked stubs might not be properly recognized as functions. (?)

k-badz avatar Mar 07 '23 13:03 k-badz