Undetectable Thunks

[astrelsky]

Describe the bug Known thunk functions matching specified patterns are not being applied due to constraining rules in CreateThunkFunctionCmd.

To Reproduce The following is the set of MIPS instructions for a non-virtual thunk. I have added in the function name and the function being thunked to since I know what they are from my wip rtti analyzer.

****************************************************************************************
*   non-virtual thunk to std::basic_fstream<char, char_traits<char>>::~basic_fstream   *
****************************************************************************************
addiu      sp,sp,-0x10
addiu      a0,a0,-0x8
sd         ra,0x0(sp)=>local_10
ld         ra,0x0(sp)=>local_10
j          std::basic_fstream<char,std--char_traits<char>>::~basic_fstream
_addiu     sp,sp,0x10

Below are the raw bytes for the set of instructions.

0xf0ffbd27
0xf8ff8424
0x0000bfff
0x0000bfdf
0x2a9a0a08
0x1000bd27

This also occurs for virtual thunks. An example is below.

****************************************************************************************
*   virtual thunk to std::basic_fstream<char,std--char_traits<char>>::~basic_fstream   *
****************************************************************************************
addiu      sp,sp,-0x10
sd         ra,0x0(sp)=>local_10
lw         t7,this(a0)
ld         ra,0x0(sp)=>local_10
lw         t6,-0xc(t7)
addu       this,this,t6
j          std::basic_fstream<char,std--char_traits<char>>::~basic_fstream
_addiu     sp,sp,0x10

Expected behavior The function to be determined to be a thunk to the function being jumped to.

Environment (please complete the following information):

• OS: Microsoft Windows [Version 10.0.17763.557]
• Java Version: 12.0.1
• Ghidra Version: 9.0.4

Additional context The defined byte patterns for the thunks are below:

<pattern> <!-- EE Non-Virtual Thunk -->
      <data> 0xf0ffbd27 0x....8424 0x0000bfff 0x0000bfdf 0x......08 0x1000bd27 </data>
      <!-- addiu      sp,sp,-0x10
           addiu      a0,a0,xxxx
           sd         ra,(sp)
           ld         ra,(sp)
           j          thunked_function
           _addiu     sp,sp,0x10
       -->
      <funcstart validcode="function" thunk="true"/>
   </pattern>

  <pattern> <!-- EE Virtual Thunk -->
      <data> 0xf0ffbd27 0x0000bfff 0x00008f8c 0x0000bfdf 0x....ee8d 0x......08 0x1000bd27 </data>
      <!-- addiu      sp,sp,-0x10
           sd         ra,(sp)
           lw         t7,(a0) # load vtable address from this
           ld         ra,(sp)
           lw         t6,offset(t7) # load ptrdiff_t (offset to virtual base) into t6
           addu       a0,a0,t6 # shift this to it's virtual base
           j          thunked_function
           _addiu     sp,sp,0x10
       -->
      <funcstart validcode="function" thunk="true"/>
   </pattern>

[astrelsky]

Trying CreateThunkFunctionCmd.getThunkedAddr in python returns None. However, stepping through what it does I was able to get the correct result. I noticed in the functions comments it says "Only go three instructions deep" yet MAX_NUMBER_OF_THUNKING_INSTRUCTIONS is set to 5. This would cause the virtual thunks to never be determined to be a thunk. Shouldn't there be another way of determining that the function cannot be a thunk? Also, the instr is never changed with the code below. It will keep going to that statement until the max limit is reached.

// keep going if flow target is right below, allow only a simple branch.
if (isLocalBranch(listing, instr, flowType)) {
    continue;
}

CreateThunkFunctionCmd also always returns false on the x86_64 variation of this thunk. The instructions in x86_64 are as follows:

                               LAB_0040a5ef                                    XREF[2]:     00575a78(*), 005ac460(*)  
        0040a5ef 48 83 ef 10     SUB        RDI,0x10
        0040a5f3 e9 6e ff        JMP        std::basic_fstream<char,std--char_traits<char>   void ~basic_fstream(basic_fstrea
                 ff ff
                             -- Flow Override: CALL_RETURN (CALL_TERMINATOR)

[astrelsky]

I have however boiled the CreateThunkFunctionCmd not creating the thunks to be due to the addRegisterUsage function not allowing the shifting of the stack pointer register. Many compilers do it automatically at the start of a function, even for thunks. Some compilers even do stupid useless operations like the storing and loading of the return address register as seen in the example above. Since the FunctionStartAnalyzer is using the CreateThunkFunctionCmd some valid thunk patterns are never applied.

I think the usage of the stack pointer register, storing/loading of the return address, and the usage of the register/stack parameter this and any temporary registers set by it should be considered valid for a thunk.

[emteere]

The rules for recognizing a thunks automatically are supposed to be simple with no side-effects. The restrictions could be loosened but it would have to be done carefully. Anything you mention above could be a side-effect that makes the function not a thunk, unless more expensive checking is done. The test is meant to be quick.

A more comprehensive test could be done at the expense of more compute time on the functions. I suppose it could be built into the constant propagation while it is analyzing, which can better detect side-effects.

CreateThunkFunctionCmd should allow you to specify the function and the location to be thunked without a check if these things are supplied.

That said, I'm not sure why your pattern doesn't work. As long as there is a valid function already defined at the start of the pattern and the destination thunk address being has been laid down on the jump, that should bypass the check for thunk validation. At least that was the intent. There is a pattern in AARCH64 that is meant for this exact case, as your pattern is, to counter the optimizer leaving an extra add instruction that has no effect.

Is there a function defined at the beginning of your thunk? It may also be a bug trying to find the thunk address versus doing a simple taint flow check on the data flow to make sure there are no side-effects.

[astrelsky]

The rules for recognizing a thunks automatically are supposed to be simple with no side-effects. The restrictions could be loosened but it would have to be done carefully. Anything you mention above could be a side-effect that makes the function not a thunk, unless more expensive checking is done. The test is meant to be quick.

A more comprehensive test could be done at the expense of more compute time on the functions. I suppose it could be built into the constant propagation while it is analyzing, which can better detect side-effects.

CreateThunkFunctionCmd should allow you to specify the function and the location to be thunked without a check if these things are supplied.

That said, I'm not sure why your pattern doesn't work. As long as there is a valid function already defined at the start of the pattern and the destination thunk address being has been laid down on the jump, that should bypass the check for thunk validation. At least that was the intent. There is a pattern in AARCH64 that is meant for this exact case, as your pattern is, to counter the optimizer leaving an extra add instruction that has no effect.

Is there a function defined at the beginning of your thunk? It may also be a bug trying to find the thunk address versus doing a simple taint flow check on the data flow to make sure there are no side-effects.

I apologize as I am aware I can be incoherent sometimes, especially when I am confused. The pattern itself does work but the thunk is never applied. This is because FunctionStartAnalyzer is using CreateThunkFunctionCmd to create the thunk function. The analyzer matches the pattern which says it is a thunk and then passes the information to CreateThunkFunctionCmd which then comes back and says it is not a thunk and then applyTo(program) returns false. The bug at hand is the conflict between the pattern and addRegisterUsage determining that the register usage is invalid.

There is a defined function at the address. The results are the same if there is not a function there and a NullPointerException is thrown is the instructions at the address are not disassembled.

I copied CreateThunkFunctionCmd into a test class and made everything public to help debug. ProgramContext.getRegistersWithValues() is only returning the gp register and its "subregisters" (ex: gp_lo) This causes addRegisterUsage to return false on the first instruction as it thinks sp is not set and considers it a side effect. However if I set the sp register it then fails on the next instruction which adjust the this parameter. This occurs even with the function defined with __thiscall. The adjusting of the this parameter should be an allowed "side effect". It is still a valid thunk and is a crucial part of c++ inheritance. In the virtual thunk the _vptr is loaded into t7, the "offset to virtual base" field of the vtable_prefix is then loaded in t6 which is then used to adjust the this pointer to its virtual base before entering the inherited function. While they could be a side effect that would not make them a thunk, forcing it to be considered to not be a thunk means that all c++ inheritance thunks would not be detected as thunks.

I can manually set it as a thunk function when analyzing the vtables as it should be possible to determine if it is a thunk from it's position in the vtable_prefix array and the thunked function from its position in said vtable_prefix's vftable. However the vtables are heavily dependent on it's class inheritance information I just haven't figured out a solid set of rules for them yet.

[astrelsky]

This can be resolved by only entering the for loop in getThunkedAddr if checkForSideEffects is true and setting MAX_NUMBER_OF_THUNKING_INSTRUCTIONS to the amount of instructions in the pattern.

[astrelsky]

@emteere there is a discrepancy between how gcc and visual studio treat thunking for classes in c++. The current CreateThunkFunctionCmd can only check for class thunks for binaries compiled with visual studio. This is because the visual studio compiler handles the shifting of the base offsets withing the function making the call to the thunk as opposed to gcc which does the shifting within the thunk function.

[emteere]

I think the best way to handle thunks with side-effects is as a pattern. The pattern with the tag thunk="true" should force it to not check for side-effects, and just create the thunk. I'll try and recreate the issue and work through it. The shifting of the base offset is not easy to characterize as a thunk generically without more context of use as you mention above.

One possibility for the epilog/prolog could be to detect that the stack pointer is set and then register values are saved and then loaded. However this would require alot more computation and could easily be led astray and allow things that aren't thunks. I believe the AARCH64 has a pattern to compensate for inefficiencies in the compiler.

[astrelsky]

One of the main problems at the moment is that CreateThunkFunctionCmd.getThunkedAddr is essentially ignoring the checkForSideEffects parameter. A lot of c++ class thunks can be accurately detected via a pointer in its vtable. I put in a pull request for a potential fix a while back. I outlined a couple potential issues with it in the request and will admit I've contemplated closing it a couple times because I still don't think it's good enough.

Also after a considerable amount of research over the summer, I've seen that as you've stated trying to find thunks with side effects without context can lead to false detection.

[astrelsky]

Resolved by 8fbdec4eca4bfeb5014dd059f2a2089cf259fa58