Windows DLL blocklist is broken due to sandboxing

RESOLVED FIXED in Firefox 49

Status

()

Core
Security: Process Sandboxing
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

Trunk
mozilla50
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49blocking fixed, firefox50+ fixed)

Details

(Whiteboard: sb+)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
[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.
(Assignee)

Updated

2 years ago
status-firefox48: --- → unaffected
(Assignee)

Comment 1

2 years ago
Created attachment 8768933 [details] [diff] [review]
Fix plus add assertion to blocklist
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8768933 - Flags: review?(benjamin)
(Assignee)

Comment 2

2 years ago
Created attachment 8768950 [details] [diff] [review]
Fix plus add assertion to blocklist (r2)

It also added winmm.dll to imports which has user32.dll as a dependency.
Attachment #8768933 - Attachment is obsolete: true
Attachment #8768933 - Flags: review?(benjamin)
Attachment #8768950 - Flags: review?(benjamin)
(Assignee)

Comment 3

2 years ago
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
(Assignee)

Comment 4

2 years ago
Created attachment 8768984 [details] [diff] [review]
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.
Attachment #8768950 - Attachment is obsolete: true
Attachment #8768950 - Flags: review?(benjamin)
Attachment #8768984 - Flags: review?(benjamin)
(Assignee)

Comment 5

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dff1037ea8f6
(Assignee)

Comment 6

2 years ago
Created attachment 8769057 [details] [diff] [review]
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.
Attachment #8768984 - Attachment is obsolete: true
Attachment #8768984 - Flags: review?(benjamin)
Attachment #8769057 - Flags: review?(benjamin)
Tracking 49/50+ since this will regress product stability.
tracking-firefox49: ? → +
tracking-firefox50: ? → +
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.
tracking-firefox49: + → blocking
(Assignee)

Comment 9

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56803decb497
Blocks: 1286306

Updated

a year ago
Attachment #8769057 - Flags: review?(benjamin) → review+

Updated

a year ago
Whiteboard: sb? → sb+
(Assignee)

Comment 10

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

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d06c9ff66cf
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Comment 12

a year ago
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
Attachment #8769057 - Flags: approval-mozilla-aurora?
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.
(Assignee)

Comment 14

a year ago
(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?
(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.
(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?
(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
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 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.
Attachment #8769057 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This needs rebasing for Aurora uplift.
Flags: needinfo?(aklotz)
(Assignee)

Comment 21

a year ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/aa1f8366541e
Flags: needinfo?(aklotz)
(Assignee)

Updated

a year ago
status-firefox49: affected → fixed
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?
(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.
(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

a year ago
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.
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?
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
You need to log in before you can comment on or make changes to this bug.