Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wrong parameter recognition #26

Closed
atar-axis opened this issue Feb 13, 2018 · 10 comments
Closed

wrong parameter recognition #26

atar-axis opened this issue Feb 13, 2018 · 10 comments
Labels

Comments

@atar-axis
Copy link

atar-axis commented Feb 13, 2018

I am not totally sure, but i guess that MOV ... is not one of the parameters - right? :D

5duxcci
https://i.imgur.com/5duxcci.png

@ThunderCls ThunderCls added the bug label Feb 14, 2018
@ThunderCls
Copy link
Owner

You're right 😀.
How would you analyze and pick, in this case, which instructions (from 21E6 to 21F7) belong to the function call and which don't. How would you purge that and code it in a standard algorithm that could be applied to every other function call?

@atar-axis
Copy link
Author

atar-axis commented Feb 14, 2018

hum, good question 😉
I don't know enough about x64/xAnalyzer internals to really answer that. how is it done at the moment?

As a human I would think somehow like that:
0x2621FA is a call to a WinAPI function, therefore it is a stdcall-function. stdcall-arguments are nearly always pushed to the stack, the only exception I can think of is moving it to esp, but i never saw that in the wild. I think that - until there is a better solution - you can simply ignore everything except pushes (at leat for stdcalls) - what do you think?

Btw: Do you know why there is no black line on the left?

@ThunderCls
Copy link
Owner

  • The internals of xAnalyzer are already there...it's an open source project xD.
    You can give a look at: "xanalyzer.cpp -> IsArgumentInstruction" function to find out how the plugin chooses which instructions are allowed and which don't.
    xAnalyzer then uses a stack to store the "valid" instructions and when a function call is found it'll take out the last instructions according to the amount of arguments the function has on the definition file. In this case of yours, the mov instruction is the first one in coming out of the stack as part of the valid arguments list instructions for this call. That's how the plugin basically handles the argument instructions for a function call.

  • Your reasoning of selecting ONLY the "push" instructions was the initial scheme of xAnalyzer and, of course it would only be valid for x86 executables, since x64 can handle function arguments using mov, lea, xor, and....etc.

  • As you stated before, depending on the compiler and how fancy it gets with its optimizations, the "mov" [esp*] or [ebp*] instruction could also be used for passing arguments to the function, making the plugin to fail in these scenarios. That's why xAnalyzer was updated in order to contemplate these specific "mov" cases as well.

If only "push"es are allowed, the plugin is gonna fails on the mov scenarios and visceversa, so, until a workaround or a better algorithm to filter out/predict the "correct argument instructions" for both cases is found, the plugin is gonna be buggy regarding this feature. That's what I meant in my previous comment, it's not a trivial task.

The arguments line should be drawn correctly, are you using the last version of both, the plugin and x64dbg? If you are, could you upload that sample to give it a look?

@atar-axis
Copy link
Author

atar-axis commented Feb 15, 2018

Thank you for the insight!
I never debugged any x86_64 executable, therefore I had to look that up and... yes, for sure, you are right.

In my case it was/is an x86 executable - therefore the problem is somehow IsMovStack(). We maybe need something that checks if...

  • in case of [ESP+n]: |n| *4 is bigger than the argument count
  • in case of [EBP+n]: ...?

But yes, I understand, that's not so easy to implement, because you would have to predict/trace.

Anyhow!
At least in the moment of calling we know exactly which stack-portion is which argument.
So what do you think about a show in argument widget option?

Something that turns

1: [esp] 00000020 
2: [esp+4] 00000020 
3: [esp+8] 00000000 
4: [esp+C] 00000024 
5: [esp+10] 00000001 

into

1: [esp]    PVOID HeapHandle 00000020 
2: [esp+4]  ULONG Flags      00000020 
3: [esp+8]  PVOID HeapBase   00000000 
4: [esp+C]  00000024 <-- automatically removed since xAnalyzer could set the Argument Counter
5: [esp+10] 00000001 <-- automatically removed since xAnalyzer could set the Argument Counter

I uploaded the file you are asked for, but beware - it is ransomware! (pasword: malware).

malware.zip

@ThunderCls
Copy link
Owner

OK I think you got something there to somehow workaround the mov issue at least for now

If(n / 4 <= function_argument_count)
VALID ARGUMENT INSTRUCTION

In your code that "mov [ebp + 24]" would be out of the picture since the function only has 6 parameters and that +24 would be way off that. So yeah I think I can come up with some code by using this approach. I'll keep this issue updated once is done.

Regarding the arguments widget yeah, I'm all for implementing this in xAnalyzer, we just gotta work on some plugin interface on x64dbg to allow plugins to handle that info, I just haven't had time to implement it myself, anyways , if you're interested we could work that out.

@atar-axis
Copy link
Author

atar-axis commented Feb 17, 2018

So yeah I think I can come up with some code by using this approach. I'll keep this issue updated once is done.

Nice! No hurry ;)

Regarding the arguments widget yeah, I'm all for implementing this in xAnalyzer ... if you're interested we could work that out

Jap! I already talked to mrexodia about this some days before, he said it would be a "minor time investment" and shouldn't be difficult. I'm relatively busy at the moment, too. Let's do that in some weeks - we should open an independent enhancement-issue in the meantime.

@ThunderCls
Copy link
Owner

ThunderCls commented Feb 18, 2018

Ok, now we have an issue here. Even when we can predict [ESP*] instructions, there's no way that I can find to choose the correct argument instructions using [EBP*]. High stack addresses correspond to the stack base pointer [EBP+n] and those refer to the actual function parameters. On the other hand [EBP-n] or low stack addresses correspond to local vars, which are most likely to be used as parameters for function calls inside the actual function. The thing is that anything from [EBP-n] to [EBP+n] could be used as a function parameter and there's no reliable way (at least that I can find) to statically predict which of all is gona be used as an actual parameter in a function call. I could just ban every [EBP*] instruction...I'm out of ideas.

@atar-axis
Copy link
Author

atar-axis commented Feb 19, 2018

Hum, tell me if I am wrong, but...

When a function is called, normally a new stack frame is created at the adress where ESP is pointing to at the moment.
So EBP after calling is pointing to (nearly, let's ignore the return address) the same postion as ESP was before calling, and therefore moving the argument to [ESP+n] is nearly the same as pushing it.

After the call, EBP+8 is the location of the first argument, no matter if it was pushed or moved at the caller-function.
This is e.g discussed / explained here:
https://stackoverflow.com/questions/39232880/using-push-or-mov-to-pass-arguments-to-a-function
https://en.wikibooks.org/wiki/X86_Disassembly/Functions_and_Stack_Frames

But... why should I mov sth to [EBP+n] in order to pass an argument to a stdcall function?
As far as I understood, moving something to [EBP+n] is like overwriting the callers arguments not the callee-arguments.

Can you maybe show me an example, you have lost me somewhere.

@ThunderCls
Copy link
Owner

ThunderCls commented Feb 19, 2018

Sorry I didn't read your entire message before writing mine so I deleted my prev message.
Anyways...I guess we both agree that we should just ban [EBP+/-] as a possible instruction for passing arguments to a function.
Regarding this, I've been updating the plugin to fix this issue as well as a couple more additions, being one the possibility to recognize indirect calls to functions like: CALL {REGISTER}, CALL {POINTER}, etc.

@atar-axis
Copy link
Author

perfect - I think now we are in tune ;) Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants