1553249 - RDD on Win7 fails to detect win32k lockdown in ProcessRuntime

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[Michael Froman [:mjf]]

Attachment: enable vorbis on rdd and re-add ProcessRuntime in RDDProcessImpl.h

When enabling vorbis decoding on RDD (and re-adding mozilla::mscom::ProcessRuntime to RDDProcessImpl.h), Win7 debug mochitests are crashing with:
Assertion failure: IsValid(), at z:/build/build/src/obj-firefox/dist/include\mozilla/mscom/ApartmentRegion.h:48

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=699622757394756381077b9ce9211fadad82a0df&selectedJob=247476977
An example of the failure is here: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247476977&repo=try&lineNumber=1369

To run a specific mochitest that fails:
./mach mochitest dom/media/mediasource/test/test_ChangeType.html

After adding some debug logging for a try push, I wonder if we're not running the proper code in the ProcessRuntime constructor because the RDD process fails the check for IsWin32kLockedDown here[1]. My logging says we are not getting a valid pGetProcessMitigationPolicy inside IsWin32kLockedDown[2].

[1] https://searchfox.org/mozilla-central/source/ipc/mscom/ProcessRuntime.cpp#53
[2] https://searchfox.org/mozilla-central/source/mozglue/misc/WindowsProcessMitigations.cpp#32

Comment 1

[Michael Froman [:mjf]]

Aaron, any thoughts here?

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

GetProcessMitigationPolicy and Win32k lockdown is not supported by Windows 7, so you'll always get false from IsWin32kLockedDown.

As to why COM isn't initializing right, my first question is for Bob: Does the Chromium sandbox try to "simulate" a Win32k lockdown on Windows 7? Could that be why we're failing to initialize a single-threaded apartment in this case?

Comment 3

[Bob Owen (:bobowen)]

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

GetProcessMitigationPolicy and Win32k lockdown is not supported by Windows 7, so you'll always get false from IsWin32kLockedDown.

As to why COM isn't initializing right, my first question is for Bob: Does the Chromium sandbox try to "simulate" a Win32k lockdown on Windows 7? Could that be why we're failing to initialize a single-threaded apartment in this case?

No, not that I'm aware of.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Michael, can you tell me which HRESULT code is being returned from CoInitializeEx?

Comment 5

[Michael Froman [:mjf]]

I added printfs there and started a try run (sorry I can't test locally):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=846ab64324b14b3958379cd270dba81311bc5399

Comment 6

[Michael Froman [:mjf]]

Aaron,

Here is the printf logging for CoInitializeEx:
21:08:29 INFO - GECKO(4028) | MJF: ApartmentRegion - CoInitializeEx result: 8007000e
21:08:29 INFO - GECKO(4028) | MJF: E_OUTOFMEMORY

from:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247628632&repo=try&lineNumber=1368

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

From what I have discovered, it appears that the failure happens because user32 is trying to register the window class for its built-in IME window. This fails in the sandboxed RDD process for some reason (though oddly enough, if I apply the RDD sandbox policy to the GPU process and try it there, everything does work).

I tried alternatives like calling ImmDisableIME first, but this fails in the RDD process too.

I think there are two routes forward from here:

• Only set sRddWin32kDisable in the sandbox broker when >= Windows 8 (I tried this in a test build on my Windows 7 box and it worked); or
• We need some kind of mechanism to determine whether this Windows 7 quasi-lockdown is in effect from within the sandboxed process. I'd prefer to avoid hacky solutions that aren't future-proof, such as checking the gecko process type specifically for the RDD process.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:aklotz, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

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

• Only set sRddWin32kDisable in the sandbox broker when >= Windows 8 (I tried this in a test build on my Windows 7 box and it worked); or
• We need some kind of mechanism to determine whether this Windows 7 quasi-lockdown is in effect from within the sandboxed process. I'd prefer to avoid hacky solutions that aren't future-proof, such as checking the gecko process type specifically for the RDD process.

Michael, can you please decide whether you'd prefer doing the first option, or whether you'd like us to implement the second one? Honestly, the first option is simpler and probably makes the most sense -- since Windows 7 does not actually support true Win32k lockdown, we should not be giving ourselves a false sense of security in that case.

Comment 10

[Michael Froman [:mjf]]

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

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

• Only set sRddWin32kDisable in the sandbox broker when >= Windows 8 (I tried this in a test build on my Windows 7 box and it worked); or
• We need some kind of mechanism to determine whether this Windows 7 quasi-lockdown is in effect from within the sandboxed process. I'd prefer to avoid hacky solutions that aren't future-proof, such as checking the gecko process type specifically for the RDD process.

Michael, can you please decide whether you'd prefer doing the first option, or whether you'd like us to implement the second one? Honestly, the first option is simpler and probably makes the most sense -- since Windows 7 does not actually support true Win32k lockdown, we should not be giving ourselves a false sense of security in that case.

Ah, I didn't realize that was a choice that was mine to make since this is all sandbox code. Since I missed the original question, I'll ask whether option 1 is a change I make, and if so are you envisioning me changing code in sandboxBroker.cpp to do this?

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Hmm... Looks like Bob originally wrote it, so I guess we can take care of it. I'll send a patch to Bob.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1553249: Only set the Win32k disable policy for the RDD process when running on Win8+; r=bobowen!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95a7d9d33d7b
Only set the Win32k disable policy for the RDD process when running on Win8+; r=bobowen

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/95a7d9d33d7b