792106 - (CVE-2012-4206) DLL Hijacking - Firefox installer

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Robert Kugler]

Attachment: trojan DLL loads cmd.exe

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

I downloaded the current Firefox installer in the Windows downloads directory. 
C:\Users\User\Downloads
Then I put a trojan dwmapi.dll in the same directory.


Actual results:

If a user wants to install the Firefox browser, the setup loads the trojan dll with administrative privileges.
As described here http://seclists.org/fulldisclosure/2012/Aug/134 , you can compromise the victim with a social engineering attack like this.



Expected results:

The installer should not load the trojan dll.

Comment 1

[Benjamin Smedberg]

It's easy to get files into the downloads directory with little interaction (using IE and Firefox); I'm going to mark this critical.

Comment 2

[Brian R. Bondy [:bbondy]]

rstrong and I were just discussing the possibility of such an issue a couple days ago.

Comment 3

[Robert Kugler]

correction:
If a user wants to install the Firefox browser, the setup loads the trojan dll with administrative privileges when the installer is executed as administrator.

If the user executes the installer with normal privileges the DLL will be loaded without administrative privileges.

Sorry!

Best regards

Robert Kugler

Comment 5

[Robert Strong (they/them - no direct email)]

Appears to affect the 7-Zip self-extracting stub and not NSIS itself.

Comment 6

[Brian R. Bondy [:bbondy]]

I also didn't see this dll listed when using depends.exe on the installer file.  With this info I think the sec-critical should be downgraded.

Comment 7

[Robert Strong (they/them - no direct email)]

I see it under COMCTL32.DLL for the installer though it doesn't load it.

Comment 8

[Brian R. Bondy [:bbondy]]

Ya, I was looking in the "Module List View" which displays a list of all unique modules that are dependencies for the root module you opened. 

I guess since dwmapi.dll is delay load, it doesn't show up in the unique list?

Comment 9

[Brian R. Bondy [:bbondy]]

OK after looking at this more:
1) You can reproduce that it launches high integrity cmd processes by right clicking and running as administrator
2) If you just double click and let NSIS do the UAC elevation though, you cannot reproduce.  In this case cmd.exe is started, but they are started at medium integrity and if the user has access to the downloads directory, they are already running as that.

Comment 10

[Brian R. Bondy [:bbondy]]

And I see this clarification was given in Comment 3 :D
Sorry for the confusion. I missed it.

Comment 11

[Brian R. Bondy [:bbondy]]

In fact I seen Comment 3 but I thought the comment meant you press no to the UAC prompt, and in which case limited user accounts install into app data.

Comment 12

[Robert Kugler]

@Brian What do you think? 
Is it a critical vulnerability?
You can gain administrative privileges if the user starts the installer with a right click.

If the user doesn't click right, you can execute a DLL with user privileges.
Isn't this enough to compromise the victim?

For example you can get a meterpreter reverse shell with powershell.exe "Shellcode".

I think this is a critical vulnerability.

Best regards

Robert Kugler

Comment 13

[Robert Strong (they/them - no direct email)]

Brian wasn't giving an opinion as to whether this is a critical vulnerability. Brian and I were trying to determine the behavior when launching the installer via the 7-Zip self-extractor and when launching the actual NSIS installer without the 7-Zip self-extractor.

Comment 14

[Brian R. Bondy [:bbondy]]

Yup re: Comment 13. 

So to summarize:
- I think manually loading this dll in init from an explicit path would fix
- If you use our main installer, and simply double click to run it, and a UAC prompt comes up, then I think this is not a security issue, because the cmd.exe processes that are started are medium integrity.
- If you use our main installer, and right click to run as admin, then I think this is a security flaw since an attacker can go from medium integrity access to high integrity access.

Comment 15

[Daniel Veditz [:dveditz]]

Why are we not using SetDllDirectory("") in the installer/self-extractor/everywhere we can? I know we've had problems in the main Firefox due to plugin breakage but our contained install-related programs should all do this!

http://msdn.microsoft.com/en-us/library/ms686203%28VS.85%29.aspx

Comment 16

[Daniel Veditz [:dveditz]]

Should this get moved to the Toolkit:NSIS Installer component? It's going to affect all our products, not just Firefox. Maybe it doesn't matter that much.

Comment 17

[Brian R. Bondy [:bbondy]]

I think that's a good idea but I don't think SetDllDirectory("") would help for this class of problems.  Even after you call SetDllDirectory(""), the first place it'll search is the current module's directory. The second place is the current working directory which that call removes.

Comment 18

[Robert Strong (they/them - no direct email)]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> Why are we not using SetDllDirectory("") in the
> installer/self-extractor/everywhere we can? I know we've had problems in the
> main Firefox due to plugin breakage but our contained install-related
> programs should all do this!
This affects the 7-Zip self-extractor which Firefox has been using since it was in the womb and we are not using SetDllDirectory because it is a 3rd party tool that we did not write or know about this issue until recently! Agreed that all programs should do this! :)

Comment 19

[Daniel Veditz [:dveditz]]

Might be too close to release to get a fix into Fx16, but we should get this in to 17 at least.

Comment 20

[Robert Strong (they/them - no direct email)]

I think a good first step would be to check if the latest 7-Zip self-extractor still has this issue especially since we would want to upstream any fix we come up with.

Comment 21

[Robert Strong (they/them - no direct email)]

Forgot to mention that Brian, myself, and the platform integration team is rather busy with project / feature work so we'd need to get Firefox product management on board with dropping that work if any of us are to work on this immediately.

Comment 22

[Brian R. Bondy [:bbondy]]

I can pickup this work for a v17 beta uplift but I don't think I'll have time for v16 beta

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Is this a sec-high or sec-critical?

Comment 24

[Daniel Veditz [:dveditz]]

It would take some social engineering to get someone to download dwmapi.dll into their download directory and not say "WTF is this?" and cancel the download attempt. But... once you do the user is eventually in trouble. I can't justify sec-critical because this is not a drive-by type bug. I'm tempted to lower to sec-moderate on the user interaction involved, but the results are bad enough to call it sec-high.

Comment 27

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Remove old 7zip reference only src

Comment 28

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - New self extracting file

Comment 29

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Actual Fix - 7zip changes from base 7zip src patch

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Precaution only - NSIS disable DLL loading from CWD

Comment 31

[Brian R. Bondy [:bbondy]]

agh source I built from is a few MB so finding solution to get that src into the tree.  Or I'll try to find a way to build the old source and apply my fix to that instead.

Comment 32

[Brian R. Bondy [:bbondy]]

Comment on attachment 671170 [details] [diff] [review]
Patch v1 - Remove old 7zip reference only src

Not going to remove this source because the new source is too big, will apply the fix to the existing source and build that.

Comment 33

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - New self extracting file (SFX)

Comment 34

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Actual Fix - 7zip changes for DLL preload

Also had to add the manifest thing to get it to work with XP+ Common Controls themes

Comment 35

[Brian R. Bondy [:bbondy]]

Sorry about the canceled reviews, I had this working first from my own src for 7zip so I just removed the old code in other-licenses and included the new one.  Then when I was posting patches I noticed it was 4MB which is too big to even attach to bugzilla let alone add to the tree :)

So I just applied the fix to the src that was already in the other-licenses folder, and rebuilt the SFX with that src with the ReleaseD config.

Comment 36

[Brian R. Bondy [:bbondy]]

Interesting bit of info from the comments section of SetDllDirectory:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686203%28v=vs.85%29.aspx

> When you launch a child process and Windows searches for its implicitly-linked
>  DLL dependencies, whether or not the current directory is searched depends on 
> whether or not your process (i.e. the parent of the one just launched) has called 
> SetDllDirectory("").

So the reason why SetDllDirectory("") doesn't work as I mentioned in Comment 17, is not because of SetDllDirectory removes the CWD (the CWD is not in the search path at all).  But the reason is because it only matters if the parent called SetDllDirectory or not.  In our case explorer.exe didn't call it.

I tested without the extra "loadDlls" code, with only the SetDllDirectory("") and the bug still reproduces by the way.

Please proceed with the reviews when you get a chance, the only thing I'll do before landing (other than review comments) is update the comments in the code to indicate this.

Comment 37

[Brian R. Bondy [:bbondy]]

And the above applies not to LoadLibrary but to implicitly linked and delayed loaded implicitly linked dlls.  

In the case of dwmapi.dll, it is implicitly linked and a delayed loaded dll.

Comment 38

[Robert Strong (they/them - no direct email)]

Comment on attachment 671173 [details] [diff] [review]
Patch v1 - Precaution only - NSIS disable DLL loading from CWD

This should also be done for stub.nsi, maintenanceservice_installer.nsi, and webapprt/win/webapp-uninstaller.nsi.in which doesn't use the common init macros.

Did you verify this fixes the problem for this bug?

Comment 39

[Robert Strong (they/them - no direct email)]

Comment on attachment 671177 [details] [diff] [review]
Patch v1 - Actual Fix - 7zip changes for DLL preload

Is this fixed in the latest 7-Zip source?

Comment 40

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #38)
> Comment on attachment 671173 [details] [diff] [review]
> Patch v1 - Precaution only - NSIS disable DLL loading from CWD
> 
> This should also be done for stub.nsi, maintenanceservice_installer.nsi, and
> webapprt/win/webapp-uninstaller.nsi.in which doesn't use the common init
> macros.

OK, I'm actually going to move the NSIS part of this patch into it's own non security sensitive bug.  It is not a fix to this bug and it is just a precaution. I'll do it for those extra 3 nsi as well in that bug.  I'll obsolete that patch which is r- in this bug now.


> Did you verify this fixes the problem for this bug?

Yes, I verified 2 things:
1) The patch as is (Patch v1 - Actual Fix - 7zip changes for DLL preload) fixes the problem.
2) That same patch modified to only call SetDllDirectory("") can still reproduce the problem.  The reason being that the call to SetDllDirectory("") does not help with delay loaded DLLs,  the only way I've ever found to fix this is to load it before the delayed load happens from an absolute path.  See Comment 36 for further explanation. 

> Is this fixed in the latest 7-Zip source?

I tried with the most recent stable version, and no it is not fixed there.
I can try to send in a patch for that once this lands.

Comment 41

[Brian R. Bondy [:bbondy]]

Posted the precaution NSIS SetDllDirectory("") bug here: bug 801853

Comment 42

[Robert Strong (they/them - no direct email)]

Comment on attachment 671177 [details] [diff] [review]
Patch v1 - Actual Fix - 7zip changes for DLL preload

Thanks and please try to upstream the fix

Comment 43

[Robert Strong (they/them - no direct email)]

Comment on attachment 671176 [details] [diff] [review]
Patch v1 - New self extracting file (SFX)

I think this was compiled with VC 6 to keep the size down and the new manifest doesn't have supportedOS

Original manifest
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
<assemblyIdentity version="1.0.0.0" processorArchitecture="X86" name="7zS.sfx.exe" type="win32"/>
<description>7-Zip Self-extracting Archive v4.42</description>
<dependency>
<dependentAssembly>
<assemblyIdentity type="win32" name="Microsoft.Windows.Common-Controls" version="6.0.0.0" processorArchitecture="X86" publicKeyToken="6595b64144ccf1df" language="*"/>
</dependentAssembly>
</dependency>
<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
<security>
<requestedPrivileges><requestedExecutionLevel level="asInvoker" uiAccess="false"/>
</requestedPrivileges>
</security>
</trustInfo>
<compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
<application>
<supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}"/>
<supportedOS Id="{e2011457-1546-43c5-a5fe-008deee3d3f0}"/>
</application>
</compatibility>
</assembly>

New manifest
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
  <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
    <security>
      <requestedPrivileges>
        <requestedExecutionLevel level="asInvoker" uiAccess="false"></requestedExecutionLevel>
      </requestedPrivileges>
    </security>
  </trustInfo>
  <dependency>
    <dependentAssembly>
      <assemblyIdentity type="win32" name="Microsoft.VC90.CRT" version="9.0.21022.8" processorArchitecture="x86" publicKeyToken="1fc8b3b9a1e18e3b"></assemblyIdentity>
    </dependentAssembly>
  </dependency>
  <dependency>
    <dependentAssembly>
      <assemblyIdentity type="win32" name="Microsoft.Windows.Common-Controls" version="6.0.0.0" processorArchitecture="*" publicKeyToken="6595b64144ccf1df" language="*"></assemblyIdentity>
    </dependentAssembly>
  </dependency>
</assembly>

Comment 44

[Robert Strong (they/them - no direct email)]

Actually, it was compiled that way since we used it on Win9x as well.

Comment 45

[Robert Strong (they/them - no direct email)]

Also, we want to be able to show the your OS is not supported message if at all possible.

Comment 46

[Brian R. Bondy [:bbondy]]

The new sfx is 126,464 bytes
The existing sfx is 122,368 bytes

So there's a few KB difference.
I don't have have VC6 handy though to compile that way, do you know of somewhere I can grab it from?

Comment 47

[Robert Strong (they/them - no direct email)]

I have it and will compile it.

Comment 48

[Brian R. Bondy [:bbondy]]

OK thanks, make sure to apply the patch first :D

Comment 49

[Robert Strong (they/them - no direct email)]

SetDLLDirectory isn't available until XP SP1 so it won't get to the point of notifying the user.

Comment 50

[Brian R. Bondy [:bbondy]]

Did you want me to load it dynamically?

Comment 51

[Robert Strong (they/them - no direct email)]

I think we should just bite the bullet and go with it as is but I'm going to talk with a couple of people first.

Comment 52

[Brian R. Bondy [:bbondy]]

That call is only a precaution, I could remove it too if you want.  The fix is loading the dll from an absolute path before it is implicitly loaded on demand.

Comment 53

[Robert Strong (they/them - no direct email)]

Brian, we are likely going to go with the ugly error so can you fix up the manifest with reshacker?

Comment 54

[Brian R. Bondy [:bbondy]]

I can but a couple questions:
- Were you going to compile the sfx with vc6 first?
- Why not just change the source code for the manifest then recompile that way for future use?

Comment 55

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #54)
> I can but a couple questions:
> - Were you going to compile the sfx with vc6 first?
If a default Win XP SP1 meets all of the dependencies then there is no need to.

> - Why not just change the source code for the manifest then recompile that
> way for future use?
That would be fine. I found it was easier to just use reshacker to change these values which change more often than the code especially when compiling with VC 6.

Comment 56

[Brian R. Bondy [:bbondy]]

Attachment: Patc v2 - New self extracting file (SFX)

Same as before but did these steps:
1. Renamed to .dll
2. Opened with VS resource editor
3. Modified manifest and saved it
4. Renamed back to .sfx

Changed back to:
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
<assemblyIdentity version="1.0.0.0" processorArchitecture="X86" name="7zS.sfx.exe" type="win32"/>
<description>7-Zip Self-extracting Archive v4.42</description>
<dependency>
<dependentAssembly>
<assemblyIdentity type="win32" name="Microsoft.Windows.Common-Controls" version="6.0.0.0" processorArchitecture="X86" publicKeyToken="6595b64144ccf1df" language="*"/>
</dependentAssembly>
</dependency>
<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
<security>
<requestedPrivileges><requestedExecutionLevel level="asInvoker" uiAccess="false"/>
</requestedPrivileges>
</security>
</trustInfo>
<compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
<application>
<supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}"/>
<supportedOS Id="{e2011457-1546-43c5-a5fe-008deee3d3f0}"/>
</application>
</compatibility>
</assembly>

Comment 57

[Brian R. Bondy [:bbondy]]

doh, tried a try build and getting: MSVCR90.dll is missing from your computer.
So I either have to statically link to the C runtime which will probably increase the installer size even further, or install VC6 and build that way.  I'll try to find VC6 and build that way.  I seem to need it every now and again anyway.

Comment 58

[Brian R. Bondy [:bbondy]]

Comment on attachment 671743 [details] [diff] [review]
Patc v2 - New self extracting file (SFX)

I'll carry forward this r+ once I build with VC6, I'll re-update the manifest at that time as well.

Comment 59

[Brian R. Bondy [:bbondy]]

I found my old MSDN VC6 but the 7zipstub project doesn't compile cleanly. I even tried with VC6SP6.  I suspect the last SFX build wasn't done with VC6.  For example, there are 151 errors including MEM_LARGE_PAGES not being defined. I checked the headers and there is no conditional inclusion on a WINVER, it just doesn't exist.  I'll try with VC2010 and statically link to the runtime.

Comment 60

[Brian R. Bondy [:bbondy]]

ok so more info...

Statically linking results in a 2MB file, so that's not acceptable.
I did find out though that the problem is with the modified manifest. 
Apparently you can't safely change a manifest file.

I tried with both Visual Studio and reshacker with the same results.
I'm going to have to compile those OS strings in directly from source.

Comment 61

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Actual Fix - 7zip changes for DLL preload

OK this bug was a bit of a nightmare but I think I have it working correctly now and it will embed the manifest for any future changes we have to make.

I even exported the manifest from the old sfx, and it gives me that dll error. 
So I just used the manifest I had, and added a supportedOS block to it.

Maybe the manifest gets changed at the time of embedding so importing directly doesn't work.

Comment 62

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - New self extracting file (SFX)

Same as before with same manifest as originally but with an ossupported block

Comment 63

[Robert Strong (they/them - no direct email)]

Comment on attachment 671930 [details] [diff] [review]
Patch v2 - Actual Fix - 7zip changes for DLL preload

I talked with Brian and there are still a couple of problems with this. He will be submitting a new patch in a day or two.

Comment 64

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - 7zip changes for safe DLL loading

As discussed, we're going to use VC6 to compile to keep the file size low and not have any missing CRT errors on startup.

If this is r+ed could you compile that SFX to avoid me having to setup the WinVista SDK with VC6?

Comment 65

[Brian R. Bondy [:bbondy]]

Comment on attachment 672201 [details] [diff] [review]
Patch v3 - 7zip changes for safe DLL loading

ack! Sorry I totally forgot about the loading from a resource string when I coded this in the middle of the night.

Comment 66

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - 7zip changes for save DLL loading

Same as last patch but I load the strings from the resource file now.

Comment 67

[Robert Strong (they/them - no direct email)]

Attachment: Patch v4 - New self extracting file (SFX) compiled with VC 6

Brian, could you verify the resources look correct?

Comment 68

[Brian R. Bondy [:bbondy]]

Comment on attachment 673075 [details] [diff] [review]
Patch v4 - New self extracting file (SFX) compiled with VC 6

The manifest and resource string looks correct, I'll push to try for builds only and try on a couple of VMs I have.

I'll update my patch to have "Setup Error" to match the updated string.

Comment 69

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5 - 7zip changes for safe DLL loading

Updated resource title string to match sfx.
Carrying forward r+.

Comment 70

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/987d2f22f83f
https://hg.mozilla.org/mozilla-central/rev/2f83907a7087

Comment 71

[Brian R. Bondy [:bbondy]]

Comment on attachment 673075 [details] [diff] [review]
Patch v4 - New self extracting file (SFX) compiled with VC 6

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Escalation of privileges attack. A lower integrity process can become a high integrity process by placing a dll with arbitrary code by the given name in the downloads folder and waiting for Firefox to be executed.
Testing completed (on m-c, etc.): I tested with a try build
Fix Landed on Version: m-c v19
Risk to taking this patch (and alternatives if risky): Low 
String or UUID changes made by this patch: There is a minimum version required error inside the 7zip stub which cannot be localized at this time.  Jonath and Asa are OK with this given that the only time it can be showed is on unsupported OS.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 72

[Brian R. Bondy [:bbondy]]

Comment on attachment 673121 [details] [diff] [review]
Patch v5 - 7zip changes for safe DLL loading

Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

User impact if declined: Escalation of privileges attack. A lower integrity process can become a high integrity process by placing a dll with arbitrary code by the given name in the downloads folder and waiting for Firefox to be executed.
Testing completed (on m-c, etc.): I tested with a try build

Fix Landed on Version: m-c v19

Risk to taking this patch (and alternatives if risky): Low 
String or UUID changes made by this patch: There is a minimum version required error inside the 7zip stub which cannot be localized at this time.  Jonath and Asa are OK with this given that the only time it can be showed is on unsupported OS.  This patch only contains the source code, and is not used directly though.

Comment 73

[Lukas Blakk [:lsblakk] use ?needinfo]

Please also mark sec-approval? on this patch since it's sec-high and looking for uplift we'll need to get that approval to be able to time landing.

Comment 74

[Robert Strong (they/them - no direct email)]

Should also let SeaMonkey know.

Comment 75

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 673075 [details] [diff] [review]
Patch v4 - New self extracting file (SFX) compiled with VC 6

Sorry, my mistake - this is already on trunk so doesn't need sec-approval, please uplift.

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/82121bb96620
https://hg.mozilla.org/releases/mozilla-aurora/rev/8e8e3952b11a

https://hg.mozilla.org/releases/mozilla-beta/rev/26cab0311a80
https://hg.mozilla.org/releases/mozilla-beta/rev/a0073727b124

https://hg.mozilla.org/releases/mozilla-esr10/rev/82648c9365f8
https://hg.mozilla.org/releases/mozilla-esr10/rev/068daaf3849d

Comment 77

[Justin Wood (:Callek)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #74)
> Should also let SeaMonkey know.

Also CC'ing mcsmurf as our installer owner, and ewong who is known to be trustworthy and quite capable with porting needed bugs

Comment 78

[u279076]

This has regressed the installer for ESR10 builds. We can no longer run ESR10 on Windows 2000 because "Windows XP SP2 or newer is required". I don't believe we wanted to desupport Windows 2000 for ESR10 until we desupported ESR10.

See bug 805032.

Comment 79

[Brian R. Bondy [:bbondy]]

Ya I suggest if we want to keep win2k we just revert these changesets:
https://hg.mozilla.org/releases/mozilla-esr10/rev/82648c9365f8
https://hg.mozilla.org/releases/mozilla-esr10/rev/068daaf3849d

I'm not sure which bug you want to do that in.

Comment 80

[Alex Keybl [:akeybl]]

(In reply to Brian R. Bondy [:bbondy] from comment #79)
> Ya I suggest if we want to keep win2k we just revert these changesets:
> https://hg.mozilla.org/releases/mozilla-esr10/rev/82648c9365f8
> https://hg.mozilla.org/releases/mozilla-esr10/rev/068daaf3849d
> 
> I'm not sure which bug you want to do that in.

Let's do this in bug 805032.

Comment 81

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So I have just tested the candidate builds for the upcoming Firefox 17.0b3 release and we are fine. Those cannot be installed on Windows 2000.

Comment 82

[u279076]

Thanks Henrik.

Comment 83

[Ryan VanderMeulen [:RyanVM]]

Backed out of ESR10.
https://hg.mozilla.org/releases/mozilla-esr10/rev/0626ed379de3

Comment 84

[Daniel Veditz [:dveditz]]

There's no reason the ESR patch has to fail < WinXP SP2, instead of putting on the MessageBox and bailing out just keep going...  People can still install ESR on those old OS's, they just won't get this security fix. Much better than having the ESR installer insecure on ALL versions of Windows, especially since most of our users will be on systems that can benefit from this patch.

Putting this back on the ESR-10 radar.

Comment 85

[Brian R. Bondy [:bbondy]]

How much of a priority is this for ESR10?  Our installer has always been like this.

I think ESR17 is going to be released on November 20th and I think that would automatically include this fix.

I don't currently have an environment setup to build with VC6 with the Vista SDK and it would take a non trivial amount of time to set that up to make these changes.  rstrong is away next week as well.

Comment 86

[Robert Strong (they/them - no direct email)]

If you can submit a patch within an hour I can compile it within the next hour.

Comment 87

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - ESR10 fix

Comment 88

[Robert Strong (they/them - no direct email)]

Attachment: (ESR10 SFX) compiled with VC 6

This is the binary. Verify that it the resources are correct and r=me on the patch.

Comment 89

[Brian R. Bondy [:bbondy]]

Thanks I downloaded the file and verified it, I'll package it in a patch and re-attach.

Comment 90

[Brian R. Bondy [:bbondy]]

Attachment: (ESR10 SFX) compiled with VC 6

Comment 91

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/0c9f82070b95
https://hg.mozilla.org/releases/mozilla-esr10/rev/1ef38198c810

Comment 93

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

reopening based on 809373 for re0investigation

Comment 94

[Frank Wein [:mcsmurf]]

To make life for the devs easier :): The same issue happens when using "cryptbase.dll" as name for the trojan dll (tested with Windows 8, see dupe Bug 809373 for details)

Comment 95

[Brian R. Bondy [:bbondy]]

Is there an easy way to search for all such DLLs? I found some rootkit packages on the Internet but I'm not sure if they'll work.

Comment 96

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Shall we remove the fixed value on the tracking flags?

Comment 97

[Brian R. Bondy [:bbondy]]

I tried to reproduce by renaming the POC to cryptbase.dll and running as administrator this installer (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-11-07-03-07-13-mozilla-central/firefox-19.0a1.en-US.win32.installer.exe) but no cmd.exe processes are launched.

Can someone else try to reproduce?

Comment 98

[Frank Wein [:mcsmurf]]

Brian: What OS did you use trying to reproduce this? Maybe this is Win7 (or even 8?) specific (also: 32-bit vs. 64-bit maybe?). I've found out about cryptbase.dll by looking at the Process Monitor log when launching the installer. I've also observed that the installer tries to load bcryptPrimitives.dll from the same folder as the installer, but renaming the POC to that name did not launch any cmd.exe processes.

Comment 99

[Brian R. Bondy [:bbondy]]

I'm using win7 x64 with a 32bit Mozilla build. 
Well all installers are themselves 32bit so that shouldn't even matter.

Comment 100

[Brian R. Bondy [:bbondy]]

procmon filtering for dlls is a good idea, thanks.

Comment 101

[Frank Wein [:mcsmurf]]

I used Windows 8 32-bit to test this, maybe loading that DLL works differently there. Also see http://www.pretentiousname.com/misc/W7E_Source/win7_uac_poc_details.html on this, there someone used the cryptbase.dll trick to inject some UAC whitelist code into a Windows system program to elevate other programs.

Comment 102

[Brian R. Bondy [:bbondy]]

Are you maybe using the stub installer?

Comment 103

[Brian R. Bondy [:bbondy]]

So using both procmon and depends.exe here's an analysis of the loaded DLLs.
I can't reproduce the problem with any of the DLLs.

I think that any DLL not in the KnownDLLs list puts us at risk of such an attack.
If a DLL is delay loaded, then it is fixable by manually loading the DLL from the system32 directory at startup time before the actual function call is made. (That is the fix in this bug).  Also I think that delay loaded DLLs are only a concern if a function is actually called on them.

Dlls that are loaded from procmon that are not in the KnownDLLs list and that are delay loaded:
C:\Windows\SysWOW64\uxtheme.dll
C:\Windows\SysWOW64\apphelp.dll
C:\Windows\SysWOW64\cryptbase.dll

Dlls that are loaded from procmon that are not in the KnownDLLs list and that I don't see in depends.exe:
C:\Windows\System32\wow64.dll
C:\Windows\System32\wow64win.dll
C:\Windows\System32\wow64cpu.dll

Dlls that are loaded from procmon that are not in the KnownDLLs list but that are Implicit Dependencies [1]:
C:\Windows\SysWOW64\sspicli.dll
C:\Windows\winsxs\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.7601.17514_none_41e6975e2bd6f2b2\comctl32.dll
C:\Windows\SysWOW64\KernelBase.dll
C:\Windows\SysWOW64\ntdll.dll

Dlls that are loaded from procmon and are not in the KnownDLLs but that we already fixed:
C:\Windows\SysWOW64\dwmapi.dll

Dlls that are loaded from procmon that are in the KnownDLLs list:
C:\Windows\SysWOW64\kernel32.dll
C:\Windows\System32\user32.dll
C:\Windows\SysWOW64\msvcrt.dll
C:\Windows\SysWOW64\gdi32.dll
C:\Windows\SysWOW64\sechost.dll
C:\Windows\SysWOW64\rpcrt4.dll
C:\Windows\SysWOW64\lpk.dll
C:\Windows\SysWOW64\usp10.dll
C:\Windows\SysWOW64\shlwapi.dll
C:\Windows\SysWOW64\oleaut32.dll
C:\Windows\SysWOW64\ole32.dll
C:\Windows\SysWOW64\shell32.dll
C:\Windows\SysWOW64\imm32.dll
C:\Windows\SysWOW64\msctf.dll
C:\Windows\SysWOW64\advapi32.dll

It's not clear to me why some DLLs that are not in the KnownDLLs list are loaded from system32 even know there exists a file in the current module directory by the same name.

[1]: Implicit Dependencies
Implicit Dependency (also known as a load-time dependency or sometimes incorrectly referred to as static dependency): Module A is implicitly linked with a LIB file for Module B at compile/link time, and Module A's source code actually calls one or more functions in Module B. Module B is a load time dependency of Module A and will be loaded into memory regardless if Module A actually makes a call to Module B at run-time. Module B will be listed in Module A's import table.

[2]: Delay-load Dependency
Module A is delay-load linked with a LIB file for Module B at compile/link time, and Module A's source code actually calls one or more functions in Module B. Module B is a dynamic dependency and will only be loaded if Module A actually makes a call to Module B at run-time. Module B will be listed in Module A's delay-load import table.

Comment 104

[Brian R. Bondy [:bbondy]]

Another note: If I use a Nightly installer from June I can reproduce the original issue with dwmapi.dll but I cannot reproduce the issue with cryptbase.dll even with the June installer.

Comment 105

[Frank Wein [:mcsmurf]]

Brian: I suspect the cryptbase.dll thing is a 32 vs. 64 bit issue. But I'll check in VMWare later (I only have the 32 bit version of Windows 8 in VMWare, currently downloading the 64 bit one).

Comment 106

[Brian R. Bondy [:bbondy]]

I also tried to reproduce with Windows 8 (x64 Windows again) but I cannot reproduce.

Perhaps this is a system hook dependency where some other software, like maybe an anti-virus package injects a dll.

System Hook Dependency (also known as an injected dependency): This type of dependency occurs when another application hooks a specific event (like a mouse event) in a process. When that process produces that event, the OS can inject a module into the process to handle the event. The module that is injected into the process is not really a dependent of any other module, but does resides in that process' address space.

Comment 107

[Brian R. Bondy [:bbondy]]

Good news I can reproduce on Win8 x86. 
I cannot reproduce on Win8x64, nor Vistax86, nor Win7x64.

I also tried on each OS with the other 2 DLLs that are not in the KnownDLLs list and are delay loaded, (uxtheme.dll, apphelp.dll) but cannot reproduce with those.

Comment 108

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Load cryptbase.dll from system32 explicitly at startup of 7zipstub

We'll also need a new SFX, pls submit a patch with a new SFX built with VC6 with the modified manifest again if you don't mind and I'll verify/r+ it.

Comment 109

[Brian R. Bondy [:bbondy]]

It would be good for someone to go through the process like I did in Comment 103 on each of these OS and check for similar problems.  I don't have the time nor setup myself to verify all of the following.  Since this is an escalation of privileges attack we only need to check on Vista and up:
- Vista x86
- Vista x64
- Windows 7 x86
- Windows 7 x64
- Windows 8 x86
- Windows 8 x64

Also cryptbase.dll problem only shows up on Win8x86 and possibly win7x86 out of the 6 mentioned above, so this shows that problems like this can be per OS or per architecture.

Comment 110

[Brian R. Bondy [:bbondy]]

Anthony I'm adding qawanted for Comment 109.

Comment 111

[u279076]

(In reply to Brian R. Bondy [:bbondy] from comment #110)
> Anthony I'm adding qawanted for Comment 109.

I'll need some time to coordinate this which I'll try to do via email or IRC. I think we should be flipping the status flags until it's really fixed.

Comment 112

[Brian R. Bondy [:bbondy]]

More info on testing:
I'd only test with the latest Nightly.
And I'd use these OS:
- Vista x86
- Vista x64
- Windows 7 x86
- Windows 7 x64
- Windows 8 x86
- Windows 8 x64

1. Rename the installer to something unique like firefoxinstaller.exe
2. Run procmon
3. Add a filter to procmon for process name is firefoxinstaller.exe
4. Add a filter for Path contains .dll
5. Add a filter for Operation is Load Image
6. Press OK
7. Run the installer to completion.
8. Copy out all of the output from procmon by selecting all of it and Ctrl+C, paste it in notepad or gvim
9. Close procmon
9. Filter out the list of all unique dlls.  Start at the first dll and do a ctrl+f to find if there's any dupes, remove the dupes. Go to the next line.
10. Determine from this list which ones are listed here:
HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session Manager\KnownDLLs
Make sure to use the same registry location as the OS the test is running on. This list may differ per OS.
11. Filter down your list to only the Dlls that are not in the KnownDLLs list.  Anything that is in the registry in the KnownDLLs list can be ignored.

12. Download the latest nightly installer in a new folder
13. Download the POC dll and put it in that same folder, 
14. Download process explorer and DO NOT put it inside that folder, the Poc dll is inside the zip attached to this bug. Make sure you extract first.
13. For each DLL in the list in #11 above, Copy the PoC DLL and rename it to match the DLL name.  Example: cryptbase.dll
14. Right click on the Process Explorer tool and run as administrator
15. Go to View Select Columns: Turn on Integrity Level
16. Sort by process name by clicking on that heading
17. close any cmd.exe processes you already have open
18. Right click on the installer and run as administrator, if any cmd.exe show up running as high integrity at all then you reproduced a DLL injection.

If you find a dll that starts a bunch of cmd.exe processes as mentioned in #18 above:
1. Open firefoxinstaller.exe in depends.exe (Dependency Walker tool)
2. In the module list view, find the dll in the list box (not the tree).  See if it has an icon next to the DLL of an hourglass. 
If it does that means it is a delayed loaded DLL and can easily be fixed.


I'd suggest testing with Win8x86 first and if you did the steps correctly you should find cryptbase.dll as a Dll injection.

Comment 113

[Brian R. Bondy [:bbondy]]

It would probably be good to do the steps for the stub as well if you have time on each OS.

Comment 114

[Frank Wein [:mcsmurf]]

You can save a bit of time when using the WinObj tool from http://technet.microsoft.com/en-us/sysinternals/bb896657.aspx in Step 10 instead. It displays all known Windows system DLLs (Windows loads statically linked DLLs into the KnownDLLs list, too, so that's the transitive closure of the registry list). You need to use the dll list from the KnownDlls32 section, at least when testing the normal 32-bit version of Firefox.

Comment 115

[u279076]

Thanks for the help so far Brian and Frank. I'd like to seek approval for me to use Softvision (our Romanian QA partner) to load-balance this testing. There's a lot to do here and if the end-result is time sensitive we need to try to parallelize the testing. My initial estimate is that I'd get this done in a couple of weeks on my own.

Comment 116

[Robert Strong (they/them - no direct email)]

Attachment: include cryptbase.dll - m-c dll compiled with VC6

Creating a dll for esr next

Comment 117

[Robert Strong (they/them - no direct email)]

Comment on attachment 679381 [details]
include cryptbase.dll - m-c dll compiled with VC6

Forgot to make manifest changes

Comment 118

[Robert Strong (they/them - no direct email)]

Attachment: include cryptbase.dll - m-c dll compiled with VC6

Comment 119

[Robert Strong (they/them - no direct email)]

Attachment: include cryptbase.dll - esr10 dll compiled with VC6

Comment 120

[Brian R. Bondy [:bbondy]]

Comment on attachment 679208 [details] [diff] [review]
Patch v1 - Load cryptbase.dll from system32 explicitly at startup of 7zipstub

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Since we introduced 7zip stub installers
User impact if declined: Processes that are low integrity could elevate themselves to high integrity if they know the location of a firefox installer and that installer exists inside a low integrity location, such as the downloads folder.
Testing completed (on m-c, etc.): Tested locally
Risk to taking this patch (and alternatives if risky): Very low 
String or UUID changes made by this patch: None

Comment 121

[Frank Wein [:mcsmurf]]

(In reply to Brian R. Bondy [:bbondy] from comment #112)
> More info on testing:
> I'd only test with the latest Nightly.
> And I'd use these OS:
> - Vista x86
> - Vista x64
> - Windows 7 x86
> - Windows 7 x64
> - Windows 8 x86
> - Windows 8 x64
[...]

To speed the test a bit up ;): I would export the log file in Step 8 with File->Save... as Comma-Separated Value file and save it somewhere. Then open that CSV file in Microsoft Office (use comma as separator) and use the "Remove Duplicates" function in the Data header to get all unique values in the Path column.

Comment 122

[Brian R. Bondy [:bbondy]]

Comment on attachment 679208 [details] [diff] [review]
Patch v1 - Load cryptbase.dll from system32 explicitly at startup of 7zipstub

Cancelling until QA verifies all platforms.
Based on some preliminary work, Anthony already found one more instance.

Comment 123

[u279076]

I just started this testing and will try to bang it out a bit each day as I find free time.

I've only tested the win32 normal installer on Windows 8 64-bit so far and found SHCore.dll to be spawning cmd.exe high integrity processes with the POC dll.

Comment 124

[Brian R. Bondy [:bbondy]]

Confirmed SHCore.dll on win8x64. This isn't a delay loaded DLL so the previous fixes won't work. I think the only way to fix it is to turn it into a delay loaded DLL and then apply the same fix.

Comment 125

[Lukas Blakk [:lsblakk] use ?needinfo]

Given the need for additional testing here and the timeframe proposed in comment 115, I'm wonfixing this for 17. We're one a few days away from our final beta and it's looking like we won't have a low-risk fix to land here.  Please put in a nomination if that changes or I'm not reading this right.

Comment 126

[Brian R. Bondy [:bbondy]]

I think that's the correct call for FX17.

Comment 127

[Brian R. Bondy [:bbondy]]

Hrm given Comment 124 I tried to delay load shcore.dll from VC6 on win2k and from vc2010 on win7 but I get this error.
LINK : warning LNK4199: /DELAYLOAD:shcore.dll ignored; no imports found from shcore.dll

I think if I built with win8 it would work though.  But then it wouldn't be VC6.

Comment 128

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - shcore.dll

Should I be providing patches and landing as QA finds new DLLs here? I think that would be best but let me know if I should just wait since you are the one that has to build the VC6 SFX files.

SHCore.dll wasn't showing as a delay loaded DLL in the module list view, but it did show up as a delay loaded library in the tree view. And from testing, it is indeed a delay loaded module.

Comment 129

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #127)
> Hrm given Comment 124 I tried to delay load shcore.dll from VC6 on win2k and
> from vc2010 on win7 but I get this error.
> LINK : warning LNK4199: /DELAYLOAD:shcore.dll ignored; no imports found from
> shcore.dll
> 
> I think if I built with win8 it would work though.  But then it wouldn't be
> VC6.
And then comment #64 comes back into play... right?

Comment 130

[Brian R. Bondy [:bbondy]]

Ya unfortunately if we commit as we go we need to keep doing new builds of ESR10 and non ESR10 SFXs.

Comment 131

[Robert Strong (they/them - no direct email)]

I was under the impression that compiling with VC6 wouldn't delay load shcore.dll due to comment #127. Is that impression incorrect?

Comment 132

[Brian R. Bondy [:bbondy]]

The option is ignored yes, but it's already delay loaded though. I just thought it wasn't at first.  Usually the delay loaded icon shows up on both the module list on the bottom of depends.exe and the top tree. This time it only showed up on the top tree.  It is delay loaded though.

Comment 133

[u279076]

CCing Kamil Jozwiak who is vouched by Brian Bondy to assist QA in testing this issue.

Kamil, please use the DLL attached to this bug for testing, "trojan DLL loads cmd.exe". I'll send you further instructions via email.

Thanks!

Comment 134

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/129bbff8b246
http://hg.mozilla.org/mozilla-central/rev/034b5a3db339

Comment 135

[Brian R. Bondy [:bbondy]]

Attachment: cryptbase SFX binary

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Since we've introduced 7zip wrapped installers
User impact if declined: escalation of privileges attack, a medium integrity process can become a high integrity process. (normal process can gain admin rights).
Testing completed (on m-c, etc.): I've tested locally and this just landed on m-c.
Risk to taking this patch (and alternatives if risky): Very low 
String or UUID changes made by this patch: none

Comment 136

[Brian R. Bondy [:bbondy]]

Attachment: cryptbase.dll sfx binary for esr10

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: escalation of privs. low access rights process can elevate themselves to a high access rights process.
Fix Landed on Version: m-c
Risk to taking this patch (and alternatives if risky): Very low
String or UUID changes made by this patch: none

Similar to the other patch I just requested except this allows win2k

Comment 137

[Brian R. Bondy [:bbondy]]

akeybl is asking that this ESR10 bug be nominated/landed before Wednesday 11/14.  I don't think we'll have this fully tested by then, but we'll try to land whatever is identified by that date.  That may or may not be everything depending on if QA finds more after that date.  The bug will be resolved/fixed once QA finishes the tests and the patches land.

Anthony is currently actively coordinating testing to get this done as soon as possible.

Comment 138

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 679935 [details] [diff] [review]
cryptbase SFX binary

We already wontfixed for 17 (sorry for any confusion caused by the esr10 call) so only approving for beta. We can take this on the final esr10 that ships with FF18

Comment 139

[Brian R. Bondy [:bbondy]]

>  We can take this on the final esr10 that ships with FF18

Not clear to me, so is that an a+ for the esr10 patch? It's a different patch than the aurora patch by the way.

Comment 140

[:Gavin Sharp [email: [e-mail]]]

(In reply to Curtis Koenig [:curtisk] from comment #93)
> reopening based on 809373 for re0investigation

Too late now, but a note for the future: reopening a FIXED bug is bad form, particularly for one that's had patches landed on branches. Tracking multiple landings in a single bug results in very confusing bug history, and introduces plenty of potential for confusion. In these cases you should always track the newly discovered problem in a new bug (even if the original landed fix was entirely insufficient).

Comment 141

[u279076]

Some results from testing last week. The following DLLs were found to have launched cmd.exe processes and are not listed as Known DLLs using the WinObj tool.

Win32 Normal Installer:
 * SHCore.dll: Windows 8 64-bit
 * uxtheme.dll: Windows Vista 32-bit

Win32 Stub Installer
 * cabinet.dll: Windows 7 64-bit, Windows 7 32-bit, Windows Vista 32-bit
 * credssp.dll: Windows Vista 32-bit
 * cryptbase.dll: Windows 7 32-bit
 * cryptnet.dll: Windows 7 32-bit, Windows 7 64-bit
 * cryptsp.dll: Windows 7 32-bit, Windows 7 64-bit, Windows Vista 32-bit
 * devrtl.dll: Windows 7 32-bit, Windows 7 64-bit
 * dnsapi.dll: Windows 7 32-bit, Windows 7 64-bit, Windows Vista 32-bit
 * dwmapi.dll: Windows 7 64-bit
 * gpapi.dll: Windows 7 32-bit, Windows 7 64-bit, Windows Vista 32-bit
 * IPHLPAPI.dll: Windows 7 32-bit, Windows 7 64-bit, Windows Vista 32-bit
 * linkinfo.dll: Windows Vista 32-bit
 * ncrypt.dll: Windows 7 32-bit, Windows 7 64-bit, Windows Vista 32-bit
 * netapi32.dll: Windows Vista 32-bit
 * ntmarta.dll: Windows 7 64-bit, Windows Vista 32-bit
 * ntshrui.dll: Windows Vista 32-bit
 * profapi.dll: Windows 7 32-bit, Windows 7 64-bit
 * propsys.dll: Windows Vista 32-bit
 * rasadhlp.dll: Windows 7 32-bit, Windows 7 64-bit, Windows Vista 32-bit
 * rasapi32.dll: Windows 7 64-bit, Windows Vista 32-bit
 * riched20.dll: Windows 7 32-bit, Windows 7 64-bit, Windows Vista 32-bit
 * RpcRtRemote.dll: Windows 7 64-bit, Windows Vista 32-bit
 * rtutils.dll: Windows 7 32-bit, Windows 7 64-bit
 * secur32.dll: Windows 7 32-bit, Windows 7 64-bit, Windows Vista 32-bit
 * SensApi.dll: Windows 7 32-bit, Windows 7 64-bit, Windows Vista 32-bit
 * shfolder.dll: Windows 7 64-bit, Windows Vista 32-bit
 * SLC.dll: Windows Vista 32-bit
 * userenv.dll: Windows 7 32-bit, Windows 7 64-bit
 * uxtheme.dll: Windows Vista 32-bit

Keep in mind that we are only a third of the way through testing. Though I suspect we've caught the lion's share of DLLs already (at least one would hope).

Full results are being added here as we test:
https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking

Comment 142

[Robert Strong (they/them - no direct email)]

Brian, what do you think about copying the file into a temp directory to deal with this? Might have to perform a disk space check first though.

Comment 143

[Brian R. Bondy [:bbondy]]

Possibly for the stub installer, I'm going to break that out into a different bug since it will likely be a different fix and should be tracked differently.

Comment 144

[Robert Strong (they/them - no direct email)]

Why not for the 7-zip self-extractor? Seems like it would be fairly simple to get around most of this that way.

Comment 145

[Brian R. Bondy [:bbondy]]

So my only concern with copying it to a temp directory is that a low integrity process could use ReadDirectoryChangesW or similar and wait for new files by whatever name we copy as is added.  Then it could copy DLLs before the process is actually executed but after the copy happens.  This process could then get execute itself as a high integrity process through the DLL.

It's unlikely that someone will do that but by using a similar fix to what we already did we have a fool proof method. 

We may want to do that in addition to the above fix though in case new DLL dependencies get added in over time. 

If we copy to a directory that only high integrity processes have access to that would probably work.  I think we can do that in a spin off of this bug though.  We also may have to move the UAC elevate call up into the 7zip stub.

Comment 146

[Robert Strong (they/them - no direct email)]

The 7-zip self-extractor doesn't run elevated unless it is executed as elevated so how would the new process run elevated? It would be possible through the NSIS exe in the stated scenario when it requests elevation.

My curiosity about possible other methods is mainly due to the large number of dll's and that we will need to check for additional dll's whenever a new Windows is released (possibly SP as well though less likely)... lots of whack-a-mole.

Moving the UAC elevate call to the stub would be a PITA. It would require moving the call to launch the application to the stub as well since we need to be in the user process, the ability to require elevation in the stub for people that are a member of the admin group and the request to elevate for those that are not... among other things.

Comment 147

[Brian R. Bondy [:bbondy]]

> The 7-zip self-extractor doesn't run elevated unless it is executed as 
> elevated so how would the new process run elevated?

This whole bug is about right clicking on the installer and running as admin. The DLLs that I've fixed so far do not reproduce a problem if you simply double click the installer.  So the 7zip self-extractor does run elevated in that case. 

> My curiosity about possible other methods is mainly due to the large number
> of dll's and that we will need to check for additional dll's whenever a 
> new Windows is released (possibly SP as well though less likely)... lots 
> of whack-a-mole.

Ya I think it is a good idea and will lessen the possibility of an attack, but for the reasons mentioned above, I don't think it is 100% foolproof. I do think it should be implemented for both installers but possibly as part of another bug.

I agree moving the UAC call up would be a PITA.  I'm fine with just copying to a temp directory, but it is not 100% foolproof, so in addition we should add the DLLs to the preload list like the previous DLL fixes in this bug. 

So moving forward I think we should:
- Fix SHCore.dll and uxtheme.dll for the main installer in this bug, and any others QA finds for that installer.  This fix will be trivial and we can move it up to other branches safely.
- File a follow up to fix the stub, and in that bug we can also do the copy to temp to avoid future problems with major and minor updates to the OS and to our installer.

The whole thing is a PITA, you can't safely execute a binary as a high integrity process from a low integrity folder if it has DLL dependencies.  Even the normal NSIS installer copies itself to temp and extracts DLLs to temp. So that's already susceptible to the same attack that I mentioned earlier with watching directory changes :(

Comment 148

[Robert Strong (they/them - no direct email)]

OK... let's move forward as is with these fixes and consider the options for the full and the stub installer. Note that we *might* be able to get rid of the 7-Zip self-extractor and just use NSIS but we weren't able to get the same high level of compression out of NSIS as we were when using the 7-Zip self-extractor (we tuned the hell out of it) and it would likely cause headaches with l10n repackaging and partner builds though I can think of a couple of ways to work around them.

Comment 149

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - shcore uxtheme

Comment 150

[Brian R. Bondy [:bbondy]]

We have a problem for the stub installer by the way.  The bad DLLs mentioned above are loaded before .onInit is called.  Maybe a fix is to wrap the stub as a 7zip self extracting archive so we can execute it inside the 7zip Main.  We can discuss it more in that bug when I post it soon.

Comment 151

[Robert Strong (they/them - no direct email)]

Yep. :(

Comment 152

[u279076]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #141)
> Keep in mind that we are only a third of the way through testing. Though I
> suspect we've caught the lion's share of DLLs already (at least one would
> hope).
> 
> Full results are being added here as we test:
> https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking

Managed to power through a large chunk of the testing today (please see intra-wiki for details). The following is all that remains to be tested:

* win32 normal installer on Windows 8 32-bit
* win32 stub installer on Windows 8 32-bit
* win64 stub installer on Windows 8 64-bit

Hopefully we can complete this by EOD-tomorrow.

Comment 153

[u279076]

I tried to compile the results so far into a table which hopefully makes for easier reading than parsing bullet-lists:
https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking#Results_Summary

Comment 154

[u279076]

Testing is now complete. Please see results here:
https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking#Results_Summary

I can transpose to this bug if necessary.

Comment 155

[Brian R. Bondy [:bbondy]]

Robert, the patch above covers all outstanding DLLs and QA confirmed that they did not find any new ones for the main installer.

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #154)
> Testing is now complete. Please see results here:
> https://intranet.mozilla.org/User:[e-mail]/
> DLL_Hijacking#Results_Summary
> 
> I can transpose to this bug if necessary.

I think it would be good to have the results in this bug for when this bug goes public. Ditto for the stub installer bug.  Any format is fine though, whatever method is fastest to get the info here.

Comment 156

[Brian R. Bondy [:bbondy]]

Anthony, Matt, Kamil: Thanks a ton for the work going through these DLLs on each OS. I know it was a time intensive task.

Comment 157

[Robert Strong (they/them - no direct email)]

Comment on attachment 681278 [details] [diff] [review]
Patch v1 - shcore uxtheme

I should be able to upload a new dll tonight

Comment 158

[Robert Strong (they/them - no direct email)]

Attachment: include shcore uxtheme - m-c dll compiled with VC6

Comment 159

[Robert Strong (they/them - no direct email)]

Attachment: include shcore uxtheme cryptbase - esr10 dll compiled with VC6

In case you'd like to skip the version that only has cryptbase for esr10.

Comment 160

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - vc-6 patch file for new SFX containing dwmapi, cryptbase, shcore (new), and uxtheme (new) - For mozilla-central

Comment 161

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #160)
> Created attachment 681814 [details] [diff] [review]
> Patch v1 - vc-6 patch file for cryptbase shcore uxtheme SFX for
> mozilla-central
Is this actually the cryptbase shcore uxtheme SFX for esr or is it the shcore uxtheme SFX for mozilla-central (cryptbase already landed on mozilla-central)?

Comment 162

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - vc-6 patch file for new SFX containing dwmapi, cryptbase (new), shcore (new), and uxtheme (new) - For esr10

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: escalation of privs for installer. Low integrity process can copy a DLL into the downloads folder and wait for a user to execute firefox.
Fix Landed on Version: This is a special version of the SFX that works on win2k.
Risk to taking this patch (and alternatives if risky): Very low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

I didn't land the previous SFX patch since I knew this one was coming.  Could I carry forward the a+ here?

Comment 163

[Brian R. Bondy [:bbondy]]

Comment on attachment 681814 [details] [diff] [review]
Patch v1 - vc-6 patch file for new SFX containing dwmapi, cryptbase, shcore (new), and uxtheme (new) - For mozilla-central

[Approval Request Comment]
User impact if declined: escalation of privs for installer. Low integrity process can copy a DLL into the downloads folder and wait for a user to execute firefox.
Fix Landed on Version: Just adds more DLLs to the previous fix
Risk to taking this patch (and alternatives if risky): Very low
String or UUID changes made by this patch: none

Comment 164

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0c386d6fadec
http://hg.mozilla.org/integration/mozilla-inbound/rev/6ec27ac9edac

Comment 165

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6ec27ac9edac
https://hg.mozilla.org/mozilla-central/rev/0c386d6fadec

(This still has [leave open] in the whiteboard; please close if it in fact should have been this time around)

Comment 166

[Brian R. Bondy [:bbondy]]

Yup I think we're good now, just uplifting left.

Comment 167

[Brian R. Bondy [:bbondy]]

I think we should keep this as "esr10: affected" unless the full fix to all DLLs gets promoted.

Comment 168

[Mark Banner (:standard8)]

As Gavin recommended in comment 140 - I'd really suggest having separate bugs for any more patches. It is already getting confusing as to what has landed where and when, and what is pending, I'm only able to track it as I'm actively watching what is going through, and even then I suspect I'm at risk of not matching the changes properly.

Comment 169

[Brian R. Bondy [:bbondy]]

(In reply to Mark Banner (:standard8) from comment #168)
> As Gavin recommended in comment 140 - I'd really suggest having separate
> bugs for any more patches. It is already getting confusing as to what has
> landed where and when, and what is pending, I'm only able to track it as I'm
> actively watching what is going through, and even then I suspect I'm at risk
> of not matching the changes properly.

There are no more patches being landed in this bug onto mozilla-central.  But for the bugs that are already landed there, and that need to be uplifted to other channels, we should leave the tracking flags as affected and handle those already issued approval requests here.

That's part of the reason I moved out the stub installer DLLs into its own bug.

Comment 170

[Alex Keybl [:akeybl]]

Comment on attachment 681814 [details] [diff] [review]
Patch v1 - vc-6 patch file for new SFX containing dwmapi, cryptbase, shcore (new), and uxtheme (new) - For mozilla-central

[Triage Comment]
Approving for Aurora - please land before Monday morning PT to make it in befor the merge. We'll approve for uplift to the ESR once 10.0.11 ships.

Comment 171

[Brian R. Bondy [:bbondy]]

Pushed the approval SFX to Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/ac7cef05200f

Comment 172

[Mark Banner (:standard8)]

(In reply to Alex Keybl [:akeybl] from comment #170)
> Comment on attachment 681814 [details] [diff] [review]
> Patch v1 - vc-6 patch file for new SFX containing dwmapi, cryptbase, shcore
> (new), and uxtheme (new) - For mozilla-central
> 
> [Triage Comment]
> Approving for Aurora - please land before Monday morning PT to make it in
> befor the merge. We'll approve for uplift to the ESR once 10.0.11 ships.

I believe this patch will also be needed on ESR 17, as 17 has shipped...

Comment 173

[Brian R. Bondy [:bbondy]]

I think so since it's not fixed in v17.

How is the esr17 branch created by the way, is it a fresh clone of the v17 release?  Or is it esr10 with v17's changesets pulled in?

Comment 174

[:Gavin Sharp [email: [e-mail]]]

(In reply to Brian R. Bondy [:bbondy] from comment #173)
> How is the esr17 branch created by the way, is it a fresh clone of the v17
> release?  Or is it esr10 with v17's changesets pulled in?

The former.

Comment 175

[Alex Keybl [:akeybl]]

We also need a fix on mozilla-esr17, as you all note.

Comment 176

[Brian R. Bondy [:bbondy]]

Are we dropping win2k support for esr17? I'll take Comment 175 as an a+ for ESR17 but just need to know which version of the patch to land. The one with or without Win2k support.

Comment 177

[u279076]

(In reply to Brian R. Bondy [:bbondy] from comment #176)
> Are we dropping win2k support for esr17? I'll take Comment 175 as an a+ for
> ESR17 but just need to know which version of the patch to land. The one with
> or without Win2k support.

Yes. 

esr10: Win2K supported, Win8 unsupported
esr17: Win2k unspported, Win8 supported

Comment 178

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/99a7860de6c6
https://hg.mozilla.org/releases/mozilla-esr17/rev/34f7e490988d

Comment 179

[u279076]

Thanks to Kamil, this has now been tested with the normal win32 installer.

Firefox 18:
Firefox 18.0b2 + uxtheme.dll + Vista 32-bit
Firefox 18.0b2 + uxtheme.dll + Vista 64-bit
Firefox 18.0b2 + SHCore.dll + Windows 8 64-bit

Firefox 19:
Firefox 19.0a2 + uxtheme.dll + Vista 32-bit
Firefox 19.0a2 + uxtheme.dll + Vista 64-bit
Firefox 19.0a2 + SHCore.dll + Windows 8 64-bit

Firefox esr10:
Firefox 10.0.11esrpre + uxtheme.dll + Vista 32-bit
Firefox 10.0.11esrpre + uxtheme.dll + Vista 64-bit
Firefox 10.0.11esrpre + SHCore.dll + Windows 8 64-bit

Firefox esr17:
Firefox 17.0.1esrpre + uxtheme.dll + Vista 32-bit
Firefox 17.0.1esrpre + uxtheme.dll + Vista 64-bit
Firefox 17.0.1esrpre + SHCore.dll + Windows 8 64-bit

Comment 180

[u279076]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #179)

Sorry, forgot the most important detail. None of the previously affected DLLs were able to be hijacked using the latest installers for these builds.

Comment 181

[Brian R. Bondy [:bbondy]]

Awesome, thanks for testing guys. Definitely the hardest part of this whole bug.

Comment 182

[Al Billings [:abillings - ex-MoCo]]

We assigned a CVE and wrote an advisory for this for Firefox 17 (mfsa2012-98). Reading this, it looks like this was an error and this never got fixed in Firefox 17? 

It looks like bug 809373 was opened, which is the same technique but using a different dll name. That bug was duped to this and this was re-opened. 

Is that correct?

I have to figure out how to rewrite mfsa2012-98 to reflect this.

Comment 183

[Brian R. Bondy [:bbondy]]

Initially 1 DLL was fixed here, then a new DLL with the same problem was discovered in bug 809373, then that DLL and others were fixed inside this original bug.  The original DLL was discovered by Robert Kugler, and later other DLLs were discovered by Frank Wein, Anthony Hughes, Kamil Jozwiak, myself, and Matt Wobensmith.

Comment 184

[Brian R. Bondy [:bbondy]]

Full detailed results were summarized here by Anthony:
https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking

Comment 185

[u279076]

(In reply to Brian R. Bondy [:bbondy] from comment #184)
> Full detailed results were summarized here by Anthony:
> https://intranet.mozilla.org/User:[e-mail]/DLL_Hijacking

Feel free to email me or ping me on IRC if you have any questions about this document.

Comment 186

[Al Billings [:abillings - ex-MoCo]]

Thanks, Brian.

I'll figure out how to write it up. It would have been better, for me, to use a new bug for this but I know it was probably easier to keep the issue in one place given it is the same overall issue.

Comment 187

[Brian R. Bondy [:bbondy]]

> I'll figure out how to write it up. It would have been better, 
> for me, to use a new bug for this but I know it was probably easier to keep 
> the issue in one place given it is the same overall issue.

Already discussed and I agree with it's better to use a new bug. See Comment 140.

Comment 188

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719