Closed Bug 1285356 Opened 4 years ago Closed 4 years ago

Windows DLL blocklist is broken due to sandboxing

Categories

(Core :: Security: Process Sandboxing, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- unaffected
firefox49 blocking fixed
firefox50 + fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

Details

(Whiteboard: sb+)

Attachments

(1 file, 3 obsolete files)

[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: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8768933 - Flags: review?(benjamin)
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)
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
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)
*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.
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.
Blocks: 1286306
Attachment #8769057 - Flags: review?(benjamin) → review+
Whiteboard: sb? → sb+
https://hg.mozilla.org/mozilla-central/rev/4d06c9ff66cf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.
(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)
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.
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.