Closed
Bug 1285356
Opened 8 years ago
Closed 8 years ago
Windows DLL blocklist is broken due to sandboxing
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox48 | --- | unaffected |
firefox49 | blocking | fixed |
firefox50 | + | fixed |
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
(Whiteboard: sb+)
Attachments
(1 file, 3 obsolete files)
9.20 KB,
patch
|
benjamin
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
[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•8 years ago
|
status-firefox48:
--- → unaffected
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
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•8 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•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dff1037ea8f6
Assignee | ||
Comment 6•8 years ago
|
||
*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)
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56803decb497
Updated•8 years ago
|
Attachment #8769057 -
Flags: review?(benjamin) → review+
Updated•8 years ago
|
Whiteboard: sb? → sb+
Assignee | ||
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d06c9ff66cf7ef8c1a563ba4860bb12ac89c85e Bug 1285356: Fix blocklist initialization regressions; r=bsmedberg
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4d06c9ff66cf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 12•8 years 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?
Comment 13•8 years ago
|
||
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•8 years 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?
Comment 15•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
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+
Assignee | ||
Comment 21•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/aa1f8366541e
Flags: needinfo?(aklotz)
Assignee | ||
Updated•8 years ago
|
Comment 22•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
(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•8 years 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.
Comment 26•8 years ago
|
||
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)
Comment 27•8 years ago
|
||
(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.
Description
•