1519368 - Set MaxLoaderThreads in registry for arm64 Windows install.

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen) PTO until 29th Oct]

On arm64 Windows installs we need to set the following registry entries for the sandbox to work properly:
[HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Image File Execution Options\firefox.exe]
"MaxLoaderThreads"=dword:00000001
[HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Image File Execution Options\plugin-container.exe]
"MaxLoaderThreads"=dword:00000001

This is required because the multi-threaded DLL loading doesn't work with the chromium sandbox when the access token level is strong enough to prevent the reading of DLLs at start-up on anything other than the main thread.

I'm doing this in a separate bug to bug 1515088, because this doesn't actually fix things for the case where you've just unzipped a build somewhere under your Users folder.
It also doesn't fix it when you install without Administrator permissions, where we install into AppData\Local. This is because we don't have permission to write into HKEY_LOCAL_MACHINE and creating an equivalent entry in HKEY_CURRENT_USER doesn't seem to work for Image File Execution Options.

(See bug 1515088 and bug 1515826 for more details)

Comment 1

[Bob Owen (:bobowen) PTO until 29th Oct]

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=221278061&revision=8bd149b1346f985700c1480c05fb7696206ba076

Comment 2

[Bob Owen (:bobowen) PTO until 29th Oct]

Attachment: Set MaxLoaderThreads to 1 in the registry for arm64 Windows installs

This Image File Execution Option is required for firefox.exe and
plugin-container.exe because the multi-threaded loading interferes
with the chromium sandbox.

Comment 3

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 9035962 [details] [diff] [review]
Set MaxLoaderThreads to 1 in the registry for arm64 Windows installs

Review of attachment 9035962 [details] [diff] [review]:
-----------------------------------------------------------------

I'd have concerns about this as a permanent solution, but it's fine as a stopgap.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Matt Howell (he/him) [:mhowell] from comment #3)

I'd have concerns about this as a permanent solution, but it's fine as a
stopgap.

Presumably the sandbox can hook registry functions, right? Could it be possible to force the sandbox to emit a fake MaxLoaderThreads value when it it is queried?

Comment 5

[Bob Owen (:bobowen) PTO until 29th Oct]

(In reply to Matt Howell (he/him) [:mhowell] from comment #3)
...

I'd have concerns about this as a permanent solution, but it's fine as a
stopgap.

Thanks.
Out of interest, is it the use of the registry keys to achieve the single threaded loading or the actual setting of these keys through the installer that you are concerned about?

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

(In reply to Matt Howell (he/him) [:mhowell] from comment #3)

I'd have concerns about this as a permanent solution, but it's fine as a
stopgap.

Presumably the sandbox can hook registry functions, right? Could it be possible to force the sandbox to emit a fake MaxLoaderThreads value when it it is queried?

I thought maybe this wouldn't work (as in we wouldn't see the look-up user side), but a quick check on x64 seems to suggest it would, thanks!
In fact the sandbox already hooks NtOpenKey (and NtCreateKey) for brokering access.
Looks like we'd need to hook NtQueryValueKey as well, but that doesn't seem like a big problem.
I imagine it would make sense to keep this in the sandbox code as it relates to that and it already hooks NtOpenKey.

I think I'd normally slightly prefer using the official solution of setting the registry, but given that doesn't work in all cases, I think this may well be the way to go.

I'll check on arm64.

Comment 6

[Molly Howell (she/her) (mostly inactive)]

My reservations are about the use of the registry key at all; if we're going to do that, doing it through the installer is pretty much the only way, so that's alright.

My preference is for the registry function hook solution though. It would work for ZIP builds and non-admin installs and it doesn't leave anything permanent that might complicate uninstall. I'm also a little worried that overzealous security software (which is the only kind of security software) might take issue with us writing to the Image File Execution Options key.

Comment 7

[Bob Owen (:bobowen) PTO until 29th Oct]

I've managed to get something working using the intercepted NtOpenKey (TargetNtOpenKey) that is already in the sandbox.
However the additional hoop that I've had to jump through to get that working, makes me think we should still set the proper registry entries when we can and only use the interception when we can't.

So, I'm going to land the installer patch and do the other fix in bug 1515088.

Comment 8

[Bob Owen (:bobowen) PTO until 29th Oct]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86ef2274deffbe467bc466e2d33de895177d8567

Comment 9

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
ARM 66 uplift candidate

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/86ef2274deff

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift when you're ready to try bringing this to beta? Would you need the work from the other patch too?

Comment 12

[Bob Owen (:bobowen) PTO until 29th Oct]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #11)

Can you request uplift when you're ready to try bringing this to beta? Would you need the work from the other patch too?

I've decided I'm actually going to back out this patch in bug 1515088, because in conversation with dmajor, we came up with a better and much simpler approach, which on balance I think we can use all the time instead of the registry keys.
I'll update the other bug with the details.