search

Vector35 / binaryninja-api

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

Stack Array Creation #2570

Open d0mnik opened 3 years ago

d0mnik commented 3 years ago

Binary Ninja Version 2.4.2846 Personal

Describe the bug

Binary Ninja does not recognize an array access, resulting in the creation of stack variables, throwing off decompilation.

To Reproduce Steps to reproduce the behavior:

  1. View HLIL of sub_8048b48

Screenshots

image image

Binary Ninja does not recognize that the var_1c array is being recognized and thus creates var_20, whose value is not set anywhere.

Hex-Rays more accurate decompilation:

image

Binary Ninja also fails to recognize a stack variable being passed as arguments to a function call, resulting in inaccurate decompilation

image

esp_1 is throwing off the decompilation.

More accurate Hex-Rays decompilation:

image

Version and Platform (required):

Additional context The binary can be found here: https://drive.google.com/file/d/14X1Y7Dj4sn95giIKaKdbJJ6N6P1-n2mj/view?usp=sharing

psifertex commented 3 years ago

As a temporary fix for the second issue (which is caused by two different paths to the basic block at 0x8048a30 with different stack alignment) you can right-click on the branch at 0x80489bd and select "always branch". You lose the first conditional, but it results in the stack being properly aligned which lets the function argument detection succeed for the remainder of the function. I don't know enough about the internal stack alignment calculation to know whether this is a bug or simply a missing analysis feature but I'll talk to others on the team.

Thanks for the detailed bug report.

psifertex commented 3 years ago

Another related issue here is the 32bit x86 libc library isn't shipping as you can see the 64bit binary doesn't have the same problems (and the register based calling convention of course means that the stack-misalignment doesn't occur)

bomb.zip

You can see the status of those libraries (which are actively being worked on) here: https://github.com/Vector35/binaryninja-api/issues/1495

(I removed my original paragraph here since there is still an issue with the 32bit x86 version having mis-aligned stack that removing the conditional branch at the top does still work-around)

d0mnik commented 3 years ago

Not quite sure why, but the decompilation of the 64bit binary doesnt quite look the same? It appears to be vastly different from the Hex-Rays decompilation.

psifertex commented 3 years ago

I didn't have the exact source of the one you were using so I just searched for a different binary from CMU. Found this: http://csapp.cs.cmu.edu/3e/bomb.tar which may or may not be the same binary if that's what you mean?

psifertex commented 3 years ago

If you've got source to the 32bit one you had above and can rebuild as 64bit that should also show the correct stack alignment but there's so many different bomblab variants out there it's possible it's a different one.

psifertex commented 3 years ago

Though now that you mention it, comparing IDAs output on that sample they do decidedly worse on the main function.

Screen Shot 2021-07-29 at 12 07 49 AM

versus:

Screen Shot 2021-07-29 at 12 07 57 AM
d0mnik commented 3 years ago

I didn't have the exact source of the one you were using so I just searched for a different binary from CMU. Found this: http://csapp.cs.cmu.edu/3e/bomb.tar which may or may not be the same binary if that's what you mean?

Oh. I just naturally assumed it was the same bomblab but in 64bit, and wondered why the output was so different. Did not realize you were referring to the stack misalignment.

plafosse commented 3 years ago

Yeah the stack misalignment here is due to us getting the wrong parameter count for fopen. This would have been solved by type libraries or improvements on our stack resolution algorithm