1194890 - NS_StackWalk in mozglue broke the Windows DLL blocklist

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

dmajor pointed out that it was probably the combination of bug 1172216 and bug 858928 that caused this.

Comment 3

[(Away)]

[Tracking Requested - why for this release]: DLL blocklist is partially broken. Easy fix.

Comment 4

[(Away)]

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

[Mike Hommey [:glandium]]

We really need a dll blocklist automated test.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

(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?

Comment 7

[Mike Hommey [:glandium]]

Checking that DEVELOPER_OPTIONS is not set, maybe?

Comment 8

[(Away)]

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

Comment 9

[(Away)]

> 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.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding the keyword "regression" based on comment 3. Also tracked.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

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

[Mike Hommey [:glandium]]

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

[Mike Hommey [:glandium]]

(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?

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

(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

[(Away)]

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

[(Away)]

(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

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Can we review the current patch now and worry about the test in bug 1198431? We need to get this patch uplifted to beta.

Comment 18

[Mike Hommey [:glandium]]

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.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/087ba64d44ed

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/087ba64d44ed

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

(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.)

Comment 24

[(Away)]

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.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks for the confirmation Aaron and the additional up-vote by David. Will Beta+ shortly.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8648290 [details] [diff] [review]
Patch

DLL blocklisting should work. Beta41+, Aurora42+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8bae9ec37b78

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/16883d4a2364