Move DLL blocklist setup to a global constructor

NEW
Unassigned

Status

()

Toolkit
General
4 years ago
8 months ago

People

(Reporter: dmajor (away and/or busy), Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

4 years ago
We've seen cases where applications implant binaries into the firefox process very early during startup.

On a clean Firefox 32, startup goes like this:
1) Load firefox.exe, mozglue.dll, other load-time dependencies
2) If running under a debugger, you get the "initial breakpoint" here
3) Global constructors for mozglue.dll (and other dependencies of firefox.exe)
4) firefox!wmainCRTStartup
5) Global constructors for firefox.exe
6) firefox!wmain
7) mozglue!DllBlocklist_Initialize

The application in bug 1065104 adds a DLL at (1) and hooks the process entry point at (4) to call into that DLL and load even more modules. In past debugging I've seen other apps get into (1) as well, but I don't remember the details.

We could set up the blocklist sooner by moving it to a global constructor in mozglue. This would let us run before the hooks at (4). We'll need to carefully check that all the codepaths are safe for use in a partially-initialized process.

For the 33 beta this ought to be straightforward. For 34 we'll need to undo the delayload from bug 1023941, which is fine because 34 isn't depending on it. For the long term it would be nice if we could do some meddling of our own at step (1). If that doesn't happen by 35, we would need to create a new DLL to host the blocklist, since mozglue will be delay-loaded.

bsmedberg does this sound reasonable? I'm not happy about creating a DLL just for this one thing, but I don't have any better ideas.
Flags: needinfo?(benjamin)
(Reporter)

Updated

4 years ago
Blocks: 1065104
Consideration must also be given to the fact that assistive technologies such as screen readers for the blind inject a DLL into the Firefox process to get at web content in-process, which makes COM calls up to 12 times faster than out of process[1] and is needed for efficient browsing. So whatever we do here, we must make sure that screen readers and other assistive technologies don't break. But I guess as long as those DLLs don't land on the black list, it should be fine.

[1] http://community.nvda-project.org/blog/First_Work_on_Web_Access_Grant
> The application in bug 1065104 adds a DLL at (1) and hooks the process entry
> point at (4) to call into that DLL and load even more modules.

So, instead of doing that, it will just add those other modules at (1), or load them at (3) if the static initializers for those DLLs run before mozglue's. So, apart from making those people have to change their game, is your proposal really preventing something?
(Reporter)

Comment 3

4 years ago
The purpose of the blocklist is to stop crashes from current and past DLLs, not future ones. The proposed changes would allow us to react to a larger set of applications. The blocklist will never be a defense against a deliberate attacker.

Whenever possible we restrict blocks to particular versions or timestamps, so that a 'good' vendor could release a fix and continue working without having to one-up our tricks. I concede that a really determined vendor could pre-emptively change their game, and there isn't much we can do about that.

Comment 4

4 years ago
David, I don't think I have useful opinions on how you should set this up. Having a separate DLL sounds reasonable to me. I guess my only question is whether this kind of warfare is worth the effort.
Flags: needinfo?(benjamin)
(Reporter)

Comment 5

4 years ago
Created attachment 8493535 [details] [diff] [review]
Patch
Attachment #8493535 - Flags: review?(benjamin)
Comment on attachment 8493535 [details] [diff] [review]
Patch

The problem here is that the blocklist is currently an opt-in in application code, and you're making it a must-have for all apps. For instance, currently, at the very least, the im app in comm-central (whatever it's called) and xulrunner don't use it. I'd rather not touch them.
Attachment #8493535 - Flags: feedback-
> I'd rather not touch them.

especially if you want to uplift this.
(Reporter)

Comment 8

4 years ago
Good catch, thanks. Is there a way to figure out who's loading mozglue? I can only think of GetModuleHandle("firefox.exe") which is pretty yucky.
How about a check of CONFIG['MOZ_BUILD_APP'] in moz.build, setting a relevant DEFINES?
(Reporter)

Comment 10

4 years ago
It wouldn't keep this change out of binaries like xpcshell, but I guess we don't care. OK.
(In reply to David Major [:dmajor] from comment #10)
> It wouldn't keep this change out of binaries like xpcshell, but I guess we
> don't care. OK.

Your patch doesn't do that either.
(Reporter)

Updated

4 years ago
Attachment #8493535 - Attachment is obsolete: true
Attachment #8493535 - Flags: review?(benjamin)
(Reporter)

Comment 12

4 years ago
With all the 32 release stuff, I didn't realize how close we were to 33. This change is too risky to go into a late beta. We've had too many regressions from hook changes in the past.

Frankly I don't feel good about the long-term fix (new DLL) anymore either. The current wave of crashes is unfortunate, but tolerable. I don't think it merits another round of arms race. I'd prefer to keep this change in our back pocket until we *really* need it.
You need to log in before you can comment on or make changes to this bug.