Closed Bug 1553249 Opened 1 year ago Closed 1 year ago

RDD on Win7 fails to detect win32k lockdown in ProcessRuntime

Categories

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

Unspecified
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: mjf, Assigned: aklotz)

References

Details

Attachments

(2 files)

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

See Also: → 1535704

Aaron, any thoughts here?

Flags: needinfo?(aklotz)

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?

Flags: needinfo?(aklotz) → needinfo?(bobowencode)

(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.

Flags: needinfo?(bobowencode)

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

Flags: needinfo?(mfroman)

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

Flags: needinfo?(mfroman)

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

Flags: needinfo?(aklotz)
Blocks: 1524049

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.
Flags: needinfo?(aklotz)

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(aklotz)
Flags: needinfo?(aklotz)
Priority: -- → P3

(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.

Flags: needinfo?(mfroman)

(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?

Flags: needinfo?(mfroman) → needinfo?(aklotz)

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

Assignee: nobody → aklotz
Component: IPC: MSCOM → Security: Process Sandboxing
Flags: needinfo?(aklotz)
OS: Unspecified → Windows 7
Status: NEW → ASSIGNED
Priority: P3 → P1
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95a7d9d33d7b
Only set the Win32k disable policy for the RDD process when running on Win8+; r=bobowen
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.