Improving Windows PE Support [CWE416]

[JamesShaker]

Hello!

I'm hoping to have a crack at improving Windows PE support by patching things so that some of the current skipped unit tests no longer need to be skipped. Please let me know if PRs or posts here on this matter would not be helpful at this time, or if my style needs tweaking, but I thought I'd document my progress on debugging/patching to invite insights or comments as I go. The first CWE I'd like to attack is CWE416.

My initial debugging identified that on line 199 of src/cwe_checker_lib/src/analysis/pointer_inference/object_list/cwe_helpers.rs the call:

let ids: Vec<AbstractIdentifier> = object_pointer.referenced_ids().cloned().collect();

returns an empty vector for the 64 bit PE case, but a vector with length one in the 64 bit ELF case. That vector contains the abstract memory objects pointed to by the pointer passed to free. This explains why we're not picking up UAF: our calls to free are having no effect on the abstract state. Some further investigation revealed this vector being empty indicates the relevant register (in which the pointer passed to free should be) has the value top. So why is this happening? Putting in some basic instrumentation of Load and Assign handling as well as free handling reveals that the issue occurs around the following p-code instructions

$U3100:8 = INT_ADD RBP, -16:8
$Ubd80:8 = LOAD ram($U3100:8)
RAX = COPY $Ubd80:8
RCX = COPY RAX #RCX is RDI in the ELF version
#call free occurs here

I identified that it is INT_ADD RBP, -16:8 which introduces top, then the load is loading from a top address and so returns top, the two copies propagate top, until we pass top to free. Given that -16 is a constant this indicates we must have top in RBP. That is, we are unable to track the base pointer properly in PE at this point in this example. With my current instrumentation the previous interaction with RBP seems to have placed a non-top value in it. I thought perhaps a call in between had broken it if a strange calling convention is included for PEs that isn't for ELFs which by some mistake considers RBP mutable. However inspecting the calling conventions indicates that RBP is a callee saved register in all the calling conventions used in PE and ELF as we'd expect. I will need to dig a little deeper to determine why RBP is top. I will post again when I've made some more progress.

[Enkelmann]

Let me help you a bit with debugging! :-) In the main function we lose track of the value of the RBP register after a call to the (internal) function __main. Everything else seems to be a consequence of this mistake.

Further investigation revealed a bug with the handling of calling conventions for PE files: Yes, RBP is a callee-saved register in all calling conventions. The bug is, that we do not choose any of the known calling conventions when handling internal function calls for PE files and instead fall back to the (very stupid) strategy of forgetting everything. I documented some more about the bug in issue #250 if you want to take a shot at fixing it. :-) You may have to ask some more questions for that, since I do not know whether the current documentation is enough for finding the relevant places in the code that need fixing. And you do not have to worry about style just yet, I will gladly help with fixing any style mistakes!

[JamesShaker]

Great! Thanks! Will read through the issue and poke around the code then post here with any new thoughts/questions!

[JamesShaker]

So I'm getting my head around this and looking at potential fixes, but I was just wondering why is it we need to worry about calling conventions for calls to internal functions at all? I see how the calling convention is being used to determine which registers to explicitly empty of state before entering a callee function and explicitly restore state to before returning to the caller. But couldn't the Abstract Interpretation just continue and model the function's register preserving behaviour naturally? i.e. If the called function saves state to stack and restores it later then the abstract interpretation should capture that unassisted without the need for explicit enforcement of calling conventions shouldn't it? Would doing this result in a hit to accuracy or performance? Just trying to understand the different design decisions.

[Enkelmann]

At the moment it should improve accuracy and performance:

• Performance should be better, since we can ignore contents of non-parameter registers in callees.
• Accuracy is better, since we can correct some analysis mistakes this way: For example, if the callee function contains a buffer overflow bug which allow overwriting the whole stack then the analysis has to assume that all the callee-saved registers, which were saved on the stack by the callee, are overwritten. This means, the callee cannot restore them before returning and thus we would also forget their contents in the caller. The result would be a huge precision loss, which we can mitigate (to a degree) by simply restoring callee-saved registers from the caller instead of tracking them in the callee.

I should also mention that at the moment the reasons for forgetting stack variables is most of the time imprecision in the analysis itself instead of real bugs in the analyzed code.

In a perfect world we would actually derive calling conventions for internal functions from the assembly code of functions themselves instead of just guessing a calling convention (standard or not). But such an analysis is not implemented yet.

[JamesShaker]

Ah thanks this makes sense!

[JamesShaker]

So I've determined that the Ghidra plugin API offers inferred calling conventions for internal functions and I'm able to propagate them from the p-code extractor plugin all the way through the to the analysis (being stored in the relevant Sub in IR), where they can be used to select the relevant convention for each function from the project. This seems to mend CWE415, and CWE416 for 64 bit PEs, but not x86 PEs. For 415 and 416 it seems x86 is disabled in unit tests for ELF so perhaps it is a similar issue in PEs. re. difficulty tracking alignment fixes.

[Enkelmann]

That sounds great! Would you like to write a pull request for your solution? :-)

Regarding alignment tracking: If I remember correctly, this affects almost all of the x86-gcc test cases. It seems to be a particularity of the x86-gcc compiler that it uses the alignment of the stack in the main function. So I would assume that it affects both PE and ELF files.

[JamesShaker]

Sure thing! Will just have to finalise things when I get a chance. Apologies but may be a little slow to get the PR in :(

…

On 3 Nov 2021, at 5:24 pm, Enkelmann ***@***.***> wrote:  That sounds great! Would you like to write a pull request for your solution? :-) Regarding alignment tracking: If I remember correctly, this affects almost all of the x86-gcc test cases. It seems to be a particularity of the x86-gcc compiler that it uses the alignment of the stack in the main function. So I would assume that it affects both PE and ELF files. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.