Closed
Bug 1194890
Opened 9 years ago
Closed 9 years ago
NS_StackWalk in mozglue broke the Windows DLL blocklist
Categories
(Core :: mozglue, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox40 | --- | unaffected |
firefox41 | + | fixed |
firefox42 | --- | fixed |
firefox43 | --- | fixed |
People
(Reporter: bugzilla, Assigned: bugzilla)
References
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
glandium
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Moving NS_StackWalk to mozglue in bug 1172216 inadvertently caused mozglue to import some user32 APIs that are used for synchronization in the Win32 implementation of NS_StackWalk. This effectively causes user32 to be loaded before the DLL blocklist is up.
For now I think it's safe to just ensure that user32 is delayloaded. In the long term we should probably think about moving the synchronization in NS_StackWalk away from user32 APIs.
Assignee | ||
Updated•9 years ago
|
Summary: NS_StaclkWalk in mozglue broke DLL blocklist → NS_StackWalk in mozglue broke the Windows DLL blocklist
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8648290 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 2•9 years ago
|
||
dmajor pointed out that it was probably the combination of bug 1172216 and bug 858928 that caused this.
[Tracking Requested - why for this release]: DLL blocklist is partially broken. Easy fix.
Blocks: 1172216
status-firefox40:
--- → unaffected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox43:
--- → affected
tracking-firefox41:
--- → ?
Bug 858928 alters the sequence a little bit, but ultimately it's bug 1172216 that pulls in user32, with or without the other change.
Comment 5•9 years ago
|
||
We really need a dll blocklist automated test.
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #5)
> We really need a dll blocklist automated test.
4:34:05 PM - dmajor: aklotz: it would be nice to assert that there's no user32
4:34:19 PM - dmajor: but I tried that and too many developers had weird configurations that tripped over it
4:34:28 PM - aklotz: dmajor: wow. that sucks!
4:34:36 PM - dmajor: console2 and whatnot
We could enable the assertion on our build infra at least. Is there something defined in the build system that we could use to selectively enable this?
Flags: needinfo?(mh+mozilla)
Comment 7•9 years ago
|
||
Checking that DEVELOPER_OPTIONS is not set, maybe?
Flags: needinfo?(mh+mozilla)
I suspect people would still have problems with Nightly.
Maybe we could do something that causes tests to fail? I wonder if it would be sufficient to just add
do_check_false("User32BeforeBlocklist" in extra);
to https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/unit/test_crash_moz_crash.js#10
> Maybe we could do something that causes tests to fail? I wonder if it would
> be sufficient to just add
> do_check_false("User32BeforeBlocklist" in extra);
> to
> https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/
> unit/test_crash_moz_crash.js#10
Apparently not; xpcshell.exe already has user32 loaded at startup, even with this patch.
Adding the keyword "regression" based on comment 3. Also tracked.
Keywords: regression
Assignee | ||
Comment 11•9 years ago
|
||
One way we could test this is to have a test that inspects the import tables of firefox.exe and mozglue.dll for static linking of user32.dll. That won't cover the LoadLibrary case but it's reasonably thorough and most importantly doesn't depend on the user's environment at runtime.
I can tackle this but it won't be right away as I'm caught up with some plugin regressions in 41.
Comment 12•9 years ago
|
||
Testing that the import tables do the right thing wrt user32.dll doesn't cut it to know whether the blocklist works or not. It would detect if the fix here does what it's advertising to be doing, which is to delay load user32.dll, but would't assert a) whether delayloading user32.dll actually fixes the blocklist and b) that something else won't break it in the future.
While I can live with b) not being addressed right now, a) not being addressed doesn't help make me confident in the fix.
Presumably, the blocklist applies to plugins, right? How about we add a dummy plugin with the name of something in the blocklist and add a browser test that the plugin is never loaded?
Comment 13•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #12)
> Presumably, the blocklist applies to plugins, right? How about we add a
> dummy plugin with the name of something in the blocklist and add a browser
> test that the plugin is never loaded?
Or is it maybe too late and by then the blocklist already works?
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #13)
> (In reply to Mike Hommey [:glandium] from comment #12)
> > Presumably, the blocklist applies to plugins, right? How about we add a
> > dummy plugin with the name of something in the blocklist and add a browser
> > test that the plugin is never loaded?
>
> Or is it maybe too late and by then the blocklist already works?
By that point the blocklist will be initialized. One of the biggest issues IMO is ensuring that it is initialized early enough such that it will be ready by the time that the common DLL injection routes are available.
At any rate, I just don't have the time right now to work on a full regression test for this code. I've got a bunch of regressions that need to be fixed in beta. I've filed bug 1198431 for the test work.
Comment 15•9 years ago
|
||
FWIW the blocklist does include the name of a dummy DLL that we test in CI. That makes sure the blocklist works _at all_. However, as Aaron points out, it's also important to know whether the blocklist works _early enough_.
Comment 16•9 years ago
|
||
(In reply to David Major [:dmajor] from comment #15)
> FWIW the blocklist does include the name of a dummy DLL that we test in CI.
TIL that's not true. :( https://bugzilla.mozilla.org/show_bug.cgi?id=524904#c19
Assignee | ||
Comment 17•9 years ago
|
||
Can we review the current patch now and worry about the test in bug 1198431? We need to get this patch uplifted to beta.
Flags: needinfo?(mh+mozilla)
Comment 18•9 years ago
|
||
Comment on attachment 8648290 [details] [diff] [review]
Patch
Review of attachment 8648290 [details] [diff] [review]:
-----------------------------------------------------------------
The patch is fine, can be safely uplifted, but there is no guarantee that it works and will keep working.
Attachment #8648290 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mh+mozilla)
Comment 19•9 years ago
|
||
Keywords: checkin-needed
Comment 20•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8648290 [details] [diff] [review]
Patch
Approval Request Comment
[Feature/regressing bug #]: bug 1172216
[User impact if declined]: Potential window for DLL injection during early startup, bypassing our blocklist.
[Describe test coverage new/current, TreeHerder]: On m-c, tested locally
[Risks and why]: Low, changes a build flag. No code changes.
[String/UUID change made/needed]: None
Attachment #8648290 -
Flags: approval-mozilla-beta?
Attachment #8648290 -
Flags: approval-mozilla-aurora?
Aaron, this comment https://bugzilla.mozilla.org/show_bug.cgi?id=1194890#c18 makes me wonder whether this should be uplifted to Beta41 or not. Are you certain that this patch works? Sorry but want to double check.
Flags: needinfo?(aklotz)
Assignee | ||
Comment 23•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #22)
> Aaron, this comment https://bugzilla.mozilla.org/show_bug.cgi?id=1194890#c18
> makes me wonder whether this should be uplifted to Beta41 or not. Are you
> certain that this patch works? Sorry but want to double check.
Yes, it fixes the regression that was caused by bug 1172216. When run on a debug build in my environment, an error message is displayed when the blocklist isn't loaded in time. With this patch in the tree, that error message is no longer displayed.
It is still possible (see comment 6) for certain configurations to cause the blocklist initialization to not work correctly, but that is really a separate issue from this regression and should be dealt with in other bugs. (And frankly, we should never assume that the blocklist is infallible. Sufficiently sophisticated malware with sufficient privileges could easily bypass it.)
Flags: needinfo?(aklotz)
Comment 24•9 years ago
|
||
I'll add another voice of support that we do need this on beta. The patch itself is good; comment 18 is referring to the lack of test coverage. Bug 1198431 is filed for adding tests but it's not easy and Aaron has more urgent work at the moment.
Thanks for the confirmation Aaron and the additional up-vote by David. Will Beta+ shortly.
Comment on attachment 8648290 [details] [diff] [review]
Patch
DLL blocklisting should work. Beta41+, Aurora42+
Attachment #8648290 -
Flags: approval-mozilla-beta?
Attachment #8648290 -
Flags: approval-mozilla-beta+
Attachment #8648290 -
Flags: approval-mozilla-aurora?
Attachment #8648290 -
Flags: approval-mozilla-aurora+
Comment 27•9 years ago
|
||
Comment 28•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•