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

MLIL doesn't properly infer _guard_dispatch_icall_nop as taking rax as the first parameter. #1760

Closed
marpie opened this issue Jun 9, 2020 · 7 comments
Assignees
Labels
Effort: Medium Issue should take < 1 month Impact: Medium Issue is impactful with a bad, or no, workaround Type: Enhancement Issue is a small enhancement to existing functionality
Milestone

Comments

@marpie
Copy link
Contributor

marpie commented Jun 9, 2020

Current Windows PE files (from Microsoft, on x64) contain (most of the time) CFG constructs (like _guard_dispatch_icall* functions) that are replaced by the loader to invoke control flow guard protections.

In current Binary Ninja versions (at least on the latest stable and 2.0.2201-dev) the CFG calls are not replaced by the targeted function, e.g.

image

which is translated to HLIL as:

image

The correct behavior would be to eliminate the guard call (which is just a call rax) and display it like:

RtlQueryRegistryValues(1, "NdisWan\Parameters", r8, 0, 0)

The following file can be used as an example that implements the CFG icall:

https://msdl.microsoft.com/download/symbols/ndiswan.sys/559f3aa835000/ndiswan.sys

  • Function: NdisWanReadRegistry (0x1c002fd65)
  • ical: 0x1c002fdff (to RtlQueryRegistryValues@IAT)
@joshwatson
Copy link
Contributor

It's actually calling NdiswanDWORDQueryRoutine and RtlQueryRegistryValuesEx is a UNICODE_STRING being passed in, but the vast majority of the time, these types of calls are not going to have a clear cut symbol to call. So, it can't just be replaced. Instead, it's better to just annotate the call with its parameters.

@marpie
Copy link
Contributor Author

marpie commented Jun 9, 2020

It's actually calling NdiswanDWORDQueryRoutine and RtlQueryRegistryValuesEx is a UNICODE_STRING being passed in, but the vast majority of the time, these types of calls are not going to have a clear cut symbol to call. So, it can't just be replaced. Instead, it's better to just annotate the call with its parameters.

Oh ok, I thought as the _guard_dispatch_icall_nop is a jmp rax it would go to RtlQueryRegistryValues as that is placed in rax. I will look at it more closely.

@plafosse plafosse added the Type: Enhancement Issue is a small enhancement to existing functionality label Jun 10, 2020
@ccarpenter04
Copy link

It would be nice if this could be improved upon since I believe Microsoft turns this option on by default in msvc for release builds.

@plafosse
Copy link
Member

This will likely be dependent on #1606 as we would need a platform specific analysis pass.

@plafosse plafosse added Effort: Medium Issue should take < 1 month Impact: Medium Issue is impactful with a bad, or no, workaround labels Apr 21, 2021
@plafosse plafosse added this to the 3.1 (Windows) milestone Jan 31, 2022
@plafosse plafosse modified the milestones: 3.1 (Debugger), 3.2 (Windows) Apr 4, 2022
@plafosse
Copy link
Member

One quick hack to get this analyzing better would be to on the dynamic dispatch call changed click 'E' and edit the instruction to be call [rax] this helps analysis be accurate.

@plafosse plafosse changed the title Analysis / HLIL - Improve Windows CFG Handling MLIL doesn't properly infer _guard_dispatch_icall_nop as taking rax as the first parameter. Jul 12, 2022
@plafosse
Copy link
Member

The problem actually doesn't appear to have anything to do with HLIL. The MLIL layer isn't properly inferring the parameters.

@plafosse plafosse self-assigned this Oct 5, 2022
@plafosse
Copy link
Member

plafosse commented Oct 6, 2022

Fixed in 3.1.3731 which adds a new feature to translate these calls to a call rax at the call site. This is controllable by the analysis.experimental.translateWindowsCfgCalls setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: Medium Issue should take < 1 month Impact: Medium Issue is impactful with a bad, or no, workaround Type: Enhancement Issue is a small enhancement to existing functionality
Projects
None yet
Development

No branches or pull requests

4 participants