939043 - MOZ_ASSERT(!GetModuleHandleA("user32.dll")) when launched by a separate process with ELEVATECREATEPROCESS appcompatibility set

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[(Away)]

bent reported hitting this assert when EMET is installed.

In his process, user32.dll was pulled in as a dependency of aclayers.dll, which was explicitly loaded by apphelp.dll, the Windows appcompat shim engine. Typically apphelp is loaded when you have compat flags on a binary. I was able to reproduce the assert by setting the Windows Vista mode flag on the exe. But on Ben's machine, apphelp gets loaded even without any flags, we suspect because of EMET.

I'm not entirely sure of the mechanism by which EMET causes apphelp to be loaded into a non-compat-flagged binary -- and whether we can do anything about it. I need to do more digging on that.

We should probably relax the assert to avoid troubling EMET users. That doesn't bother me so much. I'm more worried about the implications of this discovery: that there still exist cases where DLLs can get in before the blocklist.

Comment 1

[(Away)]

I'm not able to reproduce this using EMET 4.1 with all protections enabled.

Ben, what EMET version and settings are you using? (It seems the "Export" button doesn't actually capture all the settings, so it might be better to just list them) 

Would you mind briefly uninstalling EMET to confirm that it's the cause? It's a relatively painless uninstall and reinstall, so hopefully it won't be too much trouble.

And in case you still reproduce after the uninstall -- are there any other utility-type applications running? Antivirus maybe?

Comment 2

[(Away)]

Hm... with EMET, I do get apphelp forced into my process before the blocklist is initialized, but it doesn't load aclayers or user32.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Looks like I have EMET 3.5 installed. I'll be happy to try uninstalling tomorrow.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Uninstalling EMET did not fix my issue :(

Comment 5

[(Away)]

Ouch :(

I think you may have some appcompat database still on your machine. It may be leftover from EMET or maybe some other source. (EMET's uninstaller doesn't appear to remove everything, since I've seen it remember my app settings between installs)

Can you get a hold of "Compatibility Administrator" as part of "Microsoft Application Compatibility Toolkit" and see if you have anything suspicious in the system database?

Another quick test might be to try renaming the exe to something else. I imagine that would invalidate any appcompat triggers.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

I hit this also on my local build. I had to comment out the assert in order to continue with my work. I don't have EMET installed. I'm on Win7.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://blogs.msdn.com/b/cjacks/archive/2008/05/20/enabling-diagnostic-output-from-shims.aspx might help find the problem in the future.

Basically I had a registry entry in HKEY_CURRENT_USER\Software\Microsoft\Windows NT\CurrentVersion\AppCompatFlags\Layers that had devenv.exe receiving a special "ELEVATECREATEPROCESS" flag... Yikes. Apparently this can be granted *automatically* by the Program Compatibility Assistant (http://msdn.microsoft.com/en-us/library/bb756937.aspx).

All I had to do was remove the devenv.exe line and I was able to run ff again.

Comment 8

[(Away)]

It turns out EMET wasn't a factor here. Ben and Jared were both launching from other apps, VS and Console2 respectively. At one point in the past, their apps must have hit an issue that Windows tried to fix via compat flags: http://www.itwriting.com/blog/1119-vistas-mysterious-compatibility-settings-what-do-they-do.html 

As a child of these compat-flagged processes, the browser also received the compat shims, so it loaded aclayers at startup, which pulled in user32.

Given what we now know, I'm no longer concerned about the broader implications for blocklist effectiveness. These situations where we lose the DLL race are likely to be rare. If this assumption is wrong then crash data will show it.

Comment 9

[(Away)]

Attachment: Relax the assert

Patch if we want it: disables the assert if aclayers.dll is present.

Alternatively, we could choose to leave the code as is, and tell all affected users to debug using comment 7 and un-flag themselves.

Comment 10

[(Away)]

The more I think about this, the more I lean towards wontfix. If these two cases were one-offs, then it's probably for the better that they've cleaned up their registries. But if it becomes a recurring problem (for example if Windows frequently keeps re-adding the appcompat flags) then we can revisit. Benjamin, what do you think?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

David, maybe you could update the assertion to mention this bug so people who encounter this can check to see if their assertion has the same cause?

Comment 12

[(Away)]

The assert text is in the bug title, so this ought to be pretty discoverable, no?

Comment 13

[(Away)]

I meant to clear this with comment 12

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

It is in the bug title, but it would be even better if it were in the assert text. See http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1864 for an example where we do this in another place.

Comment 15

[Benjamin Smedberg]

Comment on attachment 8335670 [details] [diff] [review]
Relax the assert

I think we should just make this a NS_WARNING. We'll still get data from crash-stats on it, but otherwise we're just making developer lives harder for not much benefit.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yes, please do (NS_WARNING).  I'm hitting this with no idea why; I'm launching from ConEmu, and as best I can see I don't have ELEVATECREATEPROCESS in any app compat stuff in the registry for any involved process.

Comment 17

[:Gijs (out for now; he/him)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #16)
> Yes, please do (NS_WARNING).  I'm hitting this with no idea why; I'm
> launching from ConEmu, and as best I can see I don't have
> ELEVATECREATEPROCESS in any app compat stuff in the registry for any
> involved process.

Same here.

Comment 18

[(Away)]

Ok. There's now enough volume for a code change.

But since I can't stand unsolved mysteries, Vlad and Gijs can you confirm that you have aclayers loaded? (in windbg, "lmm aclayers" when it asserts -- or just see whether the comment 9 patch works for you)

If you do, then I'll just accept that appcompat can sneak into the process, and we'll move on. But if you don't have aclayers, then there might still be a piece of this that we don't yet understand.

Comment 19

[(Away)]

Turns out we can't use NS_WARNING this early because the XPCOM debug stuff isn't hooked up yet. Is there some other facility that I should use?

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I do not have aclayers loaded.

Comment 21

[:Gijs (out for now; he/him)]

(In reply to David Major [:dmajor] from comment #18)
> Ok. There's now enough volume for a code change.
> 
> But since I can't stand unsolved mysteries, Vlad and Gijs can you confirm
> that you have aclayers loaded? (in windbg, "lmm aclayers" when it asserts --
> or just see whether the comment 9 patch works for you)
> 
> If you do, then I'll just accept that appcompat can sneak into the process,
> and we'll move on. But if you don't have aclayers, then there might still be
> a piece of this that we don't yet understand.

I don't understand these instructions. If I run with ./mach debug, it complains I don't have gdb (wat). If I run directly from windbg by opening the executable and using 'go' from the menu, I don't see an assertion.

How should I verify whether aclayers is loaded? Or is Vlad's comment #20 enough here? Clearing needinfo for now.

Comment 22

[Benjamin Smedberg]

Comment on attachment 8335670 [details] [diff] [review]
Relax the assert

Enh, use #ifdef DEBUG printf(stderr)

Comment 23

[(Away)]

Debugged with Gijs. ConEmu has an option to inject ConEmuHk.dll into all the processes that you launch, and that DLL has a dependency on user32.

So even the aclayers patch wouldn't work here. More reason to go with debug prints.

Comment 24

[(Away)]

Attachment: Downgrade the MOZ_ASSERT to fprintf(stderr)

Also shortened the comment prose since it appears to have been inaccurate more often than not.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c149ae799213

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c149ae799213

Comment 27

[[e-mail]]

Comment on attachment 8342930 [details] [diff] [review]
Downgrade the MOZ_ASSERT to fprintf(stderr)

> #ifdef HAS_DLL_BLOCKLIST
>   DllBlocklist_Initialize();
> 
>+#ifdef DEBUG
>   // In order to be effective against AppInit DLLs, the blocklist must be
>-  // initialized before user32.dll is loaded into the process. If this assert
>-  // ever fires, then the fix for bug 932100 has been defeated and the
>-  // blocklist will miss AppInit DLLs. You should use a delayload or reorder
>-  // the code to prevent user32.dll from loading during early startup.
>-  MOZ_ASSERT(!GetModuleHandleA("user32.dll"));
>+  // initialized before user32.dll is loaded into the process (bug 932100).
>+  if (GetModuleHandleA("user32.dll")) {
>+    fprintf(stderr, "DLL blocklist was unable to intercept AppInit DLLs.\n");
>+  }
>+#endif
> #endif
I notice that DllBlocklist_Initialize already checks for user32. Would it be worth me filing a big to move the warning message there?

Comment 28

[(Away)]

(In reply to [e-mail] from comment #27)
> I notice that DllBlocklist_Initialize already checks for user32. Would it be
> worth me filing a big to move the warning message there?

I had to keep this message outside of mozglue because other DllBlocklist callers (xpcshell) can't make the same guarantees about user32.

Comment 29

[u279076]

Ben, are you able to reproduce this in the latest Beta?

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This was an assertion that turned into a warning. I haven't been paying attention now that it's nonfatal.