1535704 - win32k lockdown causes mscom::ProcessRuntime::InitializeSecurity to fail

Status: RESOLVED FIXED

(Core :: IPC: MSCOM, defect, P1)

Description

[Michael Froman [:mjf]]

Attachment: win32k-error.patch

Summarizing an email thread:
When trying to enable Vorbis decoding on RDD, the Windows call CoInitializeEx was returning E_OUTOFMEMORY. Aaron suggested that it was because mscom::ProcessRuntime initialization was missing. That was removed during the win32k lockdown for RDD sandboxing.

After re-adding the mscom::ProcessRuntime initialization, mscom::ProcessRuntime::InitializeSecurity fails calling SetEntriesInAclW with error code 127. Disabling the win32k lockdown allows this call to succeed. Aaron suggested that I open a bug at this point.

I'll attach a patch that has Vorbis decoding on RDD enabled along with the debug printfs that shows the error.

The test I'm running is:
./mach mochitest browser/base/content/test/general/browser_contextmenu_childprocess.js

Comment 1

[Michael Froman [:mjf]]

Aaron,

As requested, here is the bug and info I scraped from the email thread between you, Bob, and myself.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Thanks Michael! I'll take a peek at this today.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

The main suspect that I had was the presence of a DelayLoaded import. Indeed, it turns out that advapi32!SetEntriesInAclW redirects to a DelayLoaded import from api-ms-win-security-provider-l1-1-0.dll, which resolves to ntmarta.dll.

My current theory is that the sandbox is blocking this DelayLoad, so the implementation of SetEntriesInAclW cannot be resolved.

OTOH, the placement of mscom::ProcessRuntime in the patch in this bug seems to be implying that the ProcessRuntime is being instantiated before the token is dropped.

Anyway, this is still my best lead at the moment.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

I can confirm the same behaviour if I try the following on a content process in a debugger:

• When the content process starts up, I make the debugger break when ntmarta is loaded via delayload from SetEntriesInAclW;
• In the debugger, I changed the NTSTATUS return code from STATUS_SUCCESS to STATUS_ACCESS_DENIED;
• Walking up the call stack, I end up with error code 127 being returned from SetEntriesInAclW.

So I guess the next question is, what is it about the RDD process sandbox policy that is blocking this delayloaded DLL?

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

For ease of debugging, I modified the GPU process to use the RDD sandbox policy so that I could quickly observe this issue.

In addition to the information that I included in Comment 3, there is this:

At one point during delayload resolution, the loader checks to see whether the library we're trying to load is a KnownDLL. As part of that check, the loader ends up calling NtOpenSection in search of a section named ntmarta.dll with a non-null RootDirectory handle. Here's the handle information that I dumped from the RootDirectory:

  Type         	Directory
  Attributes   	0x10
  GrantedAccess	0x3:
         None
         Query,Traverse
  HandleCount  	262
  PointerCount 	8499676
  Name         	\KnownDlls

This call to NtOpenSection fails with STATUS_ACCESS_DENIED. Amusingly enough, an object named ntmarta.dll doesn't actually exist, so if NtOpenSection had returned a different error code indicating as such, we wouldn't have failed. But since we do fail with an unexpected error code, it is propagated up the stack and the delayload resolver sets SetEntriesInAclW to point to a stub that returns the error code that we're seeing here.

I'm going to move this over to sandboxing, since at this point it's clear that we need to fix how the RDD sandbox deals with delayloads. TL;DR we need these NtOpenSection calls in search of sections named \KnownDlls\* to work, or at least fail without STATUS_ACCESS_DENIED

Bob, thoughts?

Comment 6

[Bob Owen (:bobowen)]

Attachment: rddcom.patch

(In reply to Aaron Klotz [:aklotz] from comment #5)
...

This call to NtOpenSection fails with STATUS_ACCESS_DENIED. Amusingly enough, an object named ntmarta.dll doesn't actually exist, so if NtOpenSection had returned a different error code indicating as such, we wouldn't have failed. But since we do fail with an unexpected error code, it is propagated up the stack and the delayload resolver sets SetEntriesInAclW to point to a stub that returns the error code that we're seeing here.

I'm going to move this over to sandboxing, since at this point it's clear that we need to fix how the RDD sandbox deals with delayloads. TL;DR we need these NtOpenSection calls in search of sections named \KnownDlls\* to work, or at least fail without STATUS_ACCESS_DENIED

Bob, thoughts?

Sorry about the delay, I knew this was going to take a little while to investigate, so the quick answer would have been "No useful ones.". :-)

First thing I did was add a |if (XRE_IsRDDProcess()) { return true; }| to the start of ProcessRuntime::IsWin32kLockedDown, so I could test this without win32k lockdown enabled.

The initial problem here is that this is all being done on a separate thread.
From a token point of view, the raised initial privileges is done using an impersonation token set on the main thread before the suspended process is resumed.
So, while this is done before the sandbox is lowered, it's not on the main thread so things fail.

Initially I tried to warm things up on the main thread by loading rpcss.dll (which CoInitializeEx needed) and then a dummy call to SetEntriesInAcl.
This worked to get me as far as CoInitializeSecurity, but that failed as well.

So, I tried a different tack. In the attached patch it tries to open the thread token on the main thread and if there is one sets this on the MTA thread before doing anything else.
When the sandbox is lowered it also dispatches a task to the MTA thread to RevertToSelf.
You have to do this after the main thread, because the sandbox hooks NtSetInformationThread (which is what RevertToSelf uses), to make sure no reverts happen before the sandbox one on the main thread.

This all worked without win32k lockdown, but failed when I enabled the lockdown. :-(

Tracing through the calls for CoInitializeSecurity, it uses win32 API to get some WinStation and Desktop information:

    5     0 [  4]         win32u!NtUserGetProcessWindowStation
    7     0 [  3]       ntdll!KiUserCallbackDispatch
    8     0 [  4]         ntdll!KiUserCallForwarder
   11     0 [  5]           ntdll!LdrpValidateUserCallTarget rax = 00000fff`83057cb0
   15    11 [  4]         ntdll!KiUserCallForwarder
    2     0 [  4]         user32!_ClientThreadSetup
   11     0 [  5]           user32!ClientThreadSetup
   10     0 [  6]             ntdll!RtlEnterCriticalSection rax = 0
   16    10 [  5]           user32!ClientThreadSetup
   19     0 [  6]             ntdll!RtlLeaveCriticalSection rax = 0
   40    29 [  5]           user32!ClientThreadSetup rax = 1
   10    69 [  4]         user32!_ClientThreadSetup
    5     0 [  4]         ntdll!NtCallbackReturn
    1     0 [  3]       win32u!NtUserGetProcessWindowStation rax = cc
>> More than one level popped 3 -> 3
   28  8661 [  3]       combase!CRpcResolver::GetThreadWinstaDesktop
    3     0 [  4]         KERNEL32!GetCurrentThreadId rax = 57e8
   30  8664 [  3]       combase!CRpcResolver::GetThreadWinstaDesktop
    1     0 [  4]         user32!NtUserGetThreadDesktop
    6     0 [  4]         win32u!NtUserGetThreadDesktop rax = d0
>> More than one level popped 3 -> 3
   44  8671 [  3]       combase!CRpcResolver::GetThreadWinstaDesktop
    1     0 [  4]         user32!NtUserGetObjectInformation
    6     0 [  4]         win32u!NtUserGetObjectInformation rax = 1
>> More than one level popped 3 -> 3
   54  8678 [  3]       combase!CRpcResolver::GetThreadWinstaDesktop
    1     0 [  4]         user32!NtUserGetObjectInformation
    6     0 [  4]         win32u!NtUserGetObjectInformation rax = 1
>> More than one level popped 3 -> 3
  211  8685 [  3]       combase!CRpcResolver::GetThreadWinstaDesktop

aklotz - any ideas if we can call CoInitializeSecurity in a way that might not require this?

It's possible we could hook those and return dummy / known values or broker to the main process.
I think that it might be possible to enable win32k lockdown as a delayed mitigation, when we lower the sandbox.
I'm not sure if not having it enabled from the start, causes any security problems, but I guess it would be better than not having it at all.

We might also find that we need to do other initialization before we lower the sandbox as well.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Ewwww... this is so gross!

CoInitializeSecurity executes some COM goop to resolve and open a connection to the DCOM SCM. Part of that connection info includes winsta and desktop information. There are two code paths that this code follows, depending on whether or not user32 can be resolved. Note that this test ignores the state of Win32k lockdown.

If user32 cannot be resolved, the code branches to a fallback that can resolve that info without requiring user32. But that doesn't happen unless user32 is not loadable!

• I think this is probably a bug that I should report to the COM team; they really should be checking for Win32k lockdown as well as whether user32 is accessible.

• As a workaround, we need to somehow ensure that user32 is not resolvable by COM.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Here are some notes on this:

• We need to get our act together with xul.dll dependencies. We're wasting startup time loading user32, gdi32 and friends in sandboxed processes with win32k lockdown. Plus they're causing problems like the ones in this bug.

• The function involved in all of this is combase!IsGetProcessWindowStationPresent, which calls ApiSetQueryApiSetPresence to determine whether it can invoke user32.

• The parameter passed to ApiSetQueryApiSetPresence is a UNICODE_STRING containing "ext-ms-win-ntuser-windowstation-l1-1-0"

• I don't know how hard it is to fix our XUL dependencies. Trivially delayloading gdi32 and user32 is insufficient due to being loaded transitively. Maybe we can clean this up and add enough assertions such that nobody can add a breaking entry to OS_LIBS without being backed out.

• This explicit user32 load should go away if we're locked down. If we need DLL blocking via CreateRemoteThread to ride the trains to make this happen (it is currently only on Nightly), then so be it.

• Short-term it might be easiest to just take care of the ApiSetQueryApiSetPresence issue when locked down.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

A rough patch to handle ApiSetQueryApiSetPresence does work, however I need to make it work selectively depending on Win32k lockdown.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

• I have emailed our Microsoft discussion list to inform them of the bug.
• I'll move this back to MSCOM and assign myself to put together an interim solution.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1535704: Part 1 - Move IsWin32kLockedDown into mozglue; r=bobowen!

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1535704: Part 2 - Add a hook to sandbox target initialization that catches mscom's attempts to resolve user32 when Win32k lockdown is enabled; r=bobowen!

Depends on D27832

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1535704: Part 4 - When sandboxed, mscom::ProcessRuntime must make the MTA thread impersonate using the main thread's token; r=bobowen!

Depends on D27833

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a80bdf2de54ec71e7f3ac5acbce99a4a5aec08d

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c24582e2ea63b92acd243b171a5991b9547498e9

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1535704: Part 3 - Add an observer to sandboxTarget that fires once the main thread's token has been lowered; r=bobowen!

This allows us to loosen the coupling between the sandbox any code that needs
to run as soon as the token has been lowered.

We use std::list here because the observer service is not yet initialized.

Depends on D27833

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74eb8e5f7143
Part 1 - Move IsWin32kLockedDown into mozglue; r=bobowen

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8962632eb3534018d23b9ea1c6c723c02a2d6f8

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae4efa2152771ec13ec0e28cd41ed38dddd0995e

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12590021712ba86b78bd357995b9adcce93cfbc0

Comment 21

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/74eb8e5f7143

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=834d273b7f2cfeb3330f153805f672163d0f7b61

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d49e716056f8
Part 2 - Add a hook to sandbox target initialization that catches mscom's attempts to resolve user32 when Win32k lockdown is enabled; r=bobowen
https://hg.mozilla.org/integration/autoland/rev/d303f2ce387e
Part 3 - Add an observer to sandboxTarget that fires once the main thread's token has been lowered; r=bobowen
https://hg.mozilla.org/integration/autoland/rev/496213852ce6
Part 4 - When sandboxed, mscom::ProcessRuntime must make the MTA thread impersonate using the main thread's token; r=bobowen

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d49e716056f8
https://hg.mozilla.org/mozilla-central/rev/d303f2ce387e
https://hg.mozilla.org/mozilla-central/rev/496213852ce6