1285356 - Windows DLL blocklist is broken due to sandboxing

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

[Tracking Requested - why for this release]:
This will regress product stability.

I think this is due to the changes in bug 1035125. When I dump firefox.exe imports, I see the following:

   USER32.dll
                4DD4BC Import Address Table
                4DDEB0 Import Name Table
                     0 time date stamp
                     0 Index of first forwarder reference

                  315 SetProcessWindowStation
                  139 GetDesktopWindow
                   4C CloseDesktop
                   50 CloseWindowStation
                  1BC GetUserObjectInformationW
                  197 GetProcessWindowStation
                  383 UserHandleGrantAccess
                   76 CreateWindowStationW
                  1B2 GetThreadDesktop
                   61 CreateDesktopW

Which looks like sandboxing code to me.

user32.dll *must* be delayloaded.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Fix plus add assertion to blocklist

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Fix plus add assertion to blocklist (r2)

It also added winmm.dll to imports which has user32.dll as a dependency.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Child processes are also broken due to bug 1114647. We need to initialize the blocklist before initializing XPCOM Glue.

https://dxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.cpp#341

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Fixes plus add assertion to blocklist (r3)

This revision also fixes the issues with the content process. When starting firefox.exe for content, we were loading XPCOM before initializing the blocklist.

Note that there already is a call to initialize the dll blocklist in XRE_InitChildProcess, which is still used in the plugin-container case. Unfortunately XRE_InitChildProcess is also called in the firefox.exe content case, which would mean that in the latter situation DllBlocklist_Initialize could be called more than once.

I added a check to the blocklist to ensure that initialization only happens once.

I know that the assertion is an imperfect way of catching this kind of bustage, but I think we really need to catch this in automation.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Fixes plus add assertion to blocklist (r4)

*sigh* xpcshell and plugin-container were also a mess, even notwithstanding the sandboxing changes.

This version ensures that the blocklist fires up before user32 for all three binaries in four scenarios: firefox (chrome), firefox (content), plugin-container, and xpcshell.

Comment 7

[Marcia Knous [:marcia]]

Tracking 49/50+ since this will regress product stability.

Comment 8

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

This should probably be a release blocker for 49. My impression is that the blocklist is more important for stability on release than it is on pre-release.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d06c9ff66cf7ef8c1a563ba4860bb12ac89c85e
Bug 1285356: Fix blocklist initialization regressions; r=bsmedberg

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d06c9ff66cf

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8769057 [details] [diff] [review]
Fixes plus add assertion to blocklist (r4)

Approval Request Comment
[Feature/regressing bug #]: bug 1035125
[User impact if declined]: Increased crash rate due to disabled DLL blocklist
[Describe test coverage new/current, TreeHerder]: This patch adds an assertion that will cause test failures if blocklist startup has been broken.
[Risks and why]: Low, changes are to build system and trivial moving around of some initialization code.
[String/UUID change made/needed]: None

Comment 13

[Oriol Brufau [:Oriol]]

MOZ_ASSERT(!sUser32BeforeBlocklist); prevents me from running my just-compiled build.

Not sure if I have something misconfigured, but I don't recommend to uplift this.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Oriol from comment #13)
> MOZ_ASSERT(!sUser32BeforeBlocklist); prevents me from running my
> just-compiled build.
> 
> Not sure if I have something misconfigured, but I don't recommend to uplift
> this.

If you're having problems then I'd suggest that you comment it out for the purposes of running a local build. We plan to land a better fix for detecting blocklist regressions in bug 1286306, but at the moment this assertion is the best way for us to detect such regressions in automation. Since assertions are debug-only, this won't affect most users running release builds.

That assertion may fire for some people whose PCs are configured in such a way that user32.dll is for some reason loaded early. I have heard anecdotes in the past about certain programs causing this. Do you use console2, by any chance?

Comment 15

[Oriol Brufau [:Oriol]]

(In reply to Aaron Klotz [:aklotz] from comment #14)
> Do you use console2, by any chance?

No, I don't have that. I tried restarting my PC but it still complained, so I commented the assert out like you suggested.

Comment 16

[Jim Mathies [:jimm]]

(In reply to Oriol from comment #15)
> (In reply to Aaron Klotz [:aklotz] from comment #14)
> > Do you use console2, by any chance?
> 
> No, I don't have that. I tried restarting my PC but it still complained, so
> I commented the assert out like you suggested.

Afew questions come to mind - 

Are you using any odd desktop extensions or skinning programs?

Are you using any accessibility clients?

Do you have any crash reports in about:crashes you can submit and post a link to?

Comment 17

[Oriol Brufau [:Oriol]]

(In reply to Jim Mathies [:jimm] from comment #16)
> Afew questions come to mind - 
> 
> Are you using any odd desktop extensions or skinning programs?
> 
> Are you using any accessibility clients?
> 
> Do you have any crash reports in about:crashes you can submit and post a
> link to?

No desktop extensions and no accessibility clients.

I don't have reports of this crash because I compiled without the crash reporter.

But if you only wanted them to get more info about my system, see for example
https://crash-stats.mozilla.com/report/index/a4c5c15e-2a19-4107-86f5-58b6f2160717
https://crash-stats.mozilla.com/report/index/7d4528e6-e2c8-4e95-9f85-e3b812160717
https://crash-stats.mozilla.com/report/index/9d418389-fe86-4206-864d-48d402160717

Comment 18

[Molly Howell (she/her) (no longer active)]

I'm also hitting the !sUser32BeforeBlocklist assertion on my own builds, and I can add on to aklotz's list of anecdotes. I don't have Console2, but I do use a similar thing called ConEmu. It has its own ConEmuHk.dll that it likes to inject into everything you run through it, and disabling that feature seems to have put a stop to the problem.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8769057 [details] [diff] [review]
Fixes plus add assertion to blocklist (r4)

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

This patch fixes a stability issue. Let's take it aurora.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing for Aurora uplift.

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa1f8366541e

Comment 22

[Gabriele Svelto [:gsvelto]]

I've just run into a problem with this: I've made a regular debug build on Windows, when I tried to run it with ./mach run it tripped the assertion introduced in this patch. Is this normal behavior? Should I change something in my setup and/or how I launch my build?

Comment 23

[Jim Mathies [:jimm]]

(In reply to Gabriele Svelto [:gsvelto] from comment #22)
> I've just run into a problem with this: I've made a regular debug build on
> Windows, when I tried to run it with ./mach run it tripped the assertion
> introduced in this patch. Is this normal behavior? Should I change something
> in my setup and/or how I launch my build?

apparently it's pretty common to have user32 injected into the process early. Until we get bug 1286306 landed just comment out the assert.

Comment 24

[Gabriele Svelto [:gsvelto]]

(In reply to Jim Mathies [:jimm] from comment #23)
> apparently it's pretty common to have user32 injected into the process
> early. Until we get bug 1286306 landed just comment out the assert.

Alright, thanks.

Comment 25

[Johan Verrept]

Hi, 

I ran into this problem too. If i interpret the logs correctly, user32.dll gets loaded by McAfee Endpoint Security:

'firefox.exe' (Win32): Loaded 'C:\Program Files\McAfee\Endpoint Security\Threat Prevention\IPS\EpMPApi.dll'. Cannot find or open the PDB file.
'firefox.exe' (Win32): Loaded 'C:\Windows\System32\user32.dll'. Cannot find or open the PDB file.

Comment 26

[Jerry Shih[:jerry] (UTC+8) (inactive)]

With today's mozilla_central, I still hit the assert !sUser32BeforeBlocklist on my build.
I'm also use the "ConEmu" console.

In comment 23, the bug 1286306 was landed, do you mean we could remove the "!sUser32BeforeBlocklist" assert now?

Comment 27

[Jim Mathies [:jimm]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #26)
> With today's mozilla_central, I still hit the assert !sUser32BeforeBlocklist
> on my build.
> I'm also use the "ConEmu" console.
> 
> In comment 23, the bug 1286306 was landed, do you mean we could remove the
> "!sUser32BeforeBlocklist" assert now?

this will land in bug 1286306.