MCH3: W10 Insider DllHost injecting crash

[EaSy]

Hi,
we have new issue with MCH in the insider version of new W10 update.

Back in 2016-03-16 you modified Memory Protection Rules "(3) fixed some PAGE_EXECUTE_READWRITE security issues" you set for _InjectLibXX structs in processes PAGE_EXECUTE_READ. The problem is with dllhost.exe where NtProtectVirtualMemory fails with STATUS_DYNAMIC_CODE_BLOCKED. So the else code branch is executed. But this branch is relying on the _InjectLibXX buffer is in the PAGE_EXECUTE_READWRITE page since it modifies beginning of the _InjectLibXX buffer, but the page is PAGE_EXECUTE_READ, so it crashes.

I see two main problems. First is the fail of the NtProtectVirtualMemory function with STATUS_DYNAMIC_CODE_BLOCKED. The second is that you forgot about the direct code manipulation when setting only PAGE_EXECUTE_READ instead of PAGE_EXECUTE_READWRITE as you did before.

Any thoughts?

PP

[madshi]

Should already be fixed in the current 3.1.15 build:

http://madshi.net/madCollection.exe

[EaSy]

Hi,
I checked the new code. It improved situation but did not fix it at all. Now, the dllhost is running CPU 100% until it is killed after 30s.

I cannot check why, because windbg has problems with ProcessDynamicCodePolicy as well. So, I presume that:

Problem is in NtProtectVirtualMemory returning STATUS_DYNAMIC_CODE_BLOCKED. So your code finally ends up changing nothing. So ntdll!nttestalert is hooked forever. Forever calling LoadLibrary that fails too, since the process has also Signatures restricted set to microsoft only. So it will never load our dll anyway.

[madshi]

That sounds weird. In case the NtProtectVirtualMemory call fails, the NtTestAlert callback function is overwritten with the original NtTestAlert code. This should be similar to unhooking.

I've had a customer report problems with win10 RS2, with protected processes, in both 32bit and 64bit, and he confirmed to me that the problem was fixed with the latest madCodeHook build. I'm not completely sure if he tested with madCodeHook 3.x or 4.x, but I believe it was 3.x. The changes are the same in both versions, anyway.

Hmmmm... We *are* talking about injections done by the driver, correct? We're not talking about CreateProcessEx() or a manually called InjectLibrary(someProcessHandle), are we?

[EaSy]

We are talking about driver injections.

[madshi]

In the driver you can see this code:

Code: Select all

               // step 3: finally load the to-be-injected dll
               p1 := buf.pOldApi;
               c1 := 5;
               if npvm(dword(-1), p1, c1, PAGE_EXECUTE_READWRITE, c2) = 0 then begin
                 buf.pOldApi^ := buf.oldApi;
                 c1 := 5;
                 npvm(dword(-1), p1, c1, c2, c2);
               end else begin
                 // For some reason we can't uninstall our patch correctly.
                 // As a workaround we modify our callback to execute our copy of the original API code.
                 p1 := buf;
                 c1 := 32;
                 if npvm(dword(-1), p1, c1, PAGE_EXECUTE_READWRITE, c2) = 0 then begin
                   TPAInt64(buf)[3] := buf.oldCode[3];
                   TPAInt64(buf)[2] := buf.oldCode[2];
                   TPAInt64(buf)[1] := buf.oldCode[1];
                   TPAInt64(buf)[0] := buf.oldCode[0];
                   c1 := 32;
                   npvm(dword(-1), p1, c1, c2, c2);
                 end;
               end;

The workaround to modify the callback to execute the original NtTestAlert code was just added in the latest version. And it should in the problematic situation make sure that NtTestAlert jumps to our callback function, and our callback function then simply executes the original NtTestAlert code. I don't see how there could still be problems with this approach?

[EaSy]

Yes, but this won't work, because

Code: Select all

// step 3: finally load the to-be-injected dll
               p1 := buf.pOldApi;
               c1 := 5;
               if npvm(dword(-1), p1, c1, PAGE_EXECUTE_READWRITE, c2) = 0 then begin  <<<<<<-------- fails STATUS_DYNAMIC_CODE_BLOCKED
                 buf.pOldApi^ := buf.oldApi;
                 c1 := 5;
                 npvm(dword(-1), p1, c1, c2, c2);
               end else begin
                 // For some reason we can't uninstall our patch correctly.
                 // As a workaround we modify our callback to execute our copy of the original API code.
                 p1 := buf;
                 c1 := 32;
                 if npvm(dword(-1), p1, c1, PAGE_EXECUTE_READWRITE, c2) = 0 then begin <<<<<<-------- fails STATUS_DYNAMIC_CODE_BLOCKED
                   TPAInt64(buf)[3] := buf.oldCode[3];
                   TPAInt64(buf)[2] := buf.oldCode[2];
                   TPAInt64(buf)[1] := buf.oldCode[1];
                   TPAInt64(buf)[0] := buf.oldCode[0];
                   c1 := 32;
                   npvm(dword(-1), p1, c1, c2, c2);
                 end;
                 <<<<<<-------- no ELSE here, nttestalert is still hooked
               end;


[madshi]

Ouch, that's bad...

I suppose I could change the code logic a bit to make the whole design work. A quick workaround for you would be to add dllhost.exe as an "excluded" process, when calling InjectLibrary().

[EaSy]

OK, great.

Thx.

PP

[EaSy]

Hi,
sorry, but I must ask you. Will you be able to fix this or at least work around this before the creators update goes live (in 4 days)? Thx.
We would love to have at least 1 day to prepare updates, because it also affects all versions of MCH previously released.

PP

[EaSy]

A very nice under the hood document on this topic:

https://www.slideshare.net/JamesForshaw1/the-joy-of-sandbox-mitigations

[iconic]

if npvm(dword(-1), p1, c1, PAGE_EXECUTE_READWRITE, c2)

Might also want to modify param 1, on 64-bit systems the current process pseudo handle is not a ULONG/DWORD (32-bit) but a ULONG_PTR instead. Better to use (HANDLE)-1

--Iconic

[madshi]

You can work around it yourself by adding dllhost to the list of excluded processes, when calling InjectLibrary(). Isn't that good enough as a quick workaround?

@iconic, that's the 32bit stub code. The 64bit stub code uses (HANDLE) -1.

[iconic]

I guess you could also call SetProcessMitigationPolicy() and instead of removing dynamic code altering restrictions, which wouldn't be advisable due to security implications, instead only allow a certain thread of your choosing to be exempt from these security checks. A thread can "opt out" and be permitted to perform run-time memory modifications from what I gather.

Read about AllowThreadOptOut and ThreadDynamicCodePolicy here

https://msdn.microsoft.com/en-us/library/windows/desktop/mt706243(v=vs.85).aspx

--Iconic

[EaSy]

Hi,
I know that we can exclude DllHost.exe from injected processes. The problem is that this process is used in a lot of another scenarios that include some important to us, so we see exception on DllHost as the last resort.