Bug 792106 (CVE-2012-4206)

DLL Hijacking - Firefox installer

VERIFIED FIXED in Firefox 18

Status

()

defect
VERIFIED FIXED
7 years ago
3 years ago

People

(Reporter: r.ch.kugler, Assigned: bbondy)

Tracking

({csectype-priv-escalation, sec-high})

unspecified
mozilla19
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite -

Firefox Tracking Flags

(firefox16- wontfix, firefox17+ wontfix, firefox18+ verified, firefox19 verified, firefox-esr1018+ verified, firefox-esr1718+ verified)

Details

(Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+])

Attachments

(6 attachments, 23 obsolete attachments)

481 bytes, application/octet-stream
Details
5.02 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
1.22 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
1.27 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
80.29 KB, patch
bbondy
: review+
Details | Diff | Splinter Review
80.20 KB, patch
Details | Diff | Splinter Review
Reporter

Description

7 years ago
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

7 years ago
It's easy to get files into the downloads directory with little interaction (using IE and Firefox); I'm going to mark this critical.
Status: UNCONFIRMED → NEW
Component: Untriaged → Installer
Ever confirmed: true
Keywords: sec-critical
Assignee

Comment 2

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

Comment 3

7 years ago
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
Appears to affect the 7-Zip self-extracting stub and not NSIS itself.
Assignee

Comment 6

7 years ago
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.
I see it under COMCTL32.DLL for the installer though it doesn't load it.
Assignee

Comment 8

7 years ago
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?
Assignee

Comment 9

7 years ago
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.
Assignee

Comment 10

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

Comment 11

7 years ago
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.
Reporter

Comment 12

7 years ago
@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
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.
Assignee

Comment 14

7 years ago
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.
Reporter

Updated

7 years ago
Summary: Firefox installer DLL Hijacking → DLL Hijacking - Firefox installer
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
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.
Assignee: nobody → netzen
Assignee

Comment 17

7 years ago
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.
Assignee

Updated

7 years ago
Component: Installer → NSIS Installer
Product: Firefox → Toolkit
(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! :)
Might be too close to release to get a fix into Fx16, but we should get this in to 17 at least.
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.
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.
Assignee

Comment 22

7 years ago
I can pickup this work for a v17 beta uplift but I don't think I'll have time for v16 beta
Is this a sec-high or sec-critical?
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.
Keywords: sec-criticalsec-high
Assignee

Comment 27

7 years ago
Attachment #671170 - Flags: review?(robert.bugzilla)
Assignee

Comment 28

7 years ago
Attachment #671171 - Flags: review?(robert.bugzilla)
Assignee

Comment 29

7 years ago
Attachment #671172 - Flags: review?(robert.bugzilla)
Assignee

Comment 30

7 years ago
Attachment #671173 - Flags: review?(robert.bugzilla)
Assignee

Comment 31

7 years ago
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.
Assignee

Comment 32

7 years ago
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.
Attachment #671170 - Attachment is obsolete: true
Attachment #671170 - Flags: review?(robert.bugzilla)
Assignee

Comment 33

7 years ago
Attachment #671171 - Attachment is obsolete: true
Attachment #671172 - Attachment is obsolete: true
Attachment #671171 - Flags: review?(robert.bugzilla)
Attachment #671172 - Flags: review?(robert.bugzilla)
Attachment #671176 - Flags: review?(robert.bugzilla)
Assignee

Comment 34

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

Updated

7 years ago
Attachment #671177 - Flags: review?(robert.bugzilla)
Assignee

Comment 35

7 years ago
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.
Assignee

Comment 36

7 years ago
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.
Assignee

Comment 37

7 years ago
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 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?
Attachment #671173 - Flags: review?(robert.bugzilla) → review-
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?
Assignee

Comment 40

7 years ago
(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.
Assignee

Updated

7 years ago
Attachment #671173 - Flags: review-
Assignee

Updated

7 years ago
Attachment #671173 - Attachment is obsolete: true
Assignee

Comment 41

7 years ago
Posted the precaution NSIS SetDllDirectory("") bug here: bug 801853
Comment on attachment 671177 [details] [diff] [review]
Patch v1 - Actual Fix - 7zip changes for DLL preload

Thanks and please try to upstream the fix
Attachment #671177 - Flags: review?(robert.bugzilla) → review+
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>
Attachment #671176 - Flags: review?(robert.bugzilla) → review-
Actually, it was compiled that way since we used it on Win9x as well.
Also, we want to be able to show the your OS is not supported message if at all possible.
Assignee

Comment 46

7 years ago
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?
Assignee

Comment 48

7 years ago
OK thanks, make sure to apply the patch first :D
SetDLLDirectory isn't available until XP SP1 so it won't get to the point of notifying the user.
Assignee

Comment 50

7 years ago
Did you want me to load it dynamically?
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.
Assignee

Comment 52

7 years ago
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.
Brian, we are likely going to go with the ugly error so can you fix up the manifest with reshacker?
Assignee

Comment 54

7 years ago
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?
(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.
Assignee

Comment 56

7 years ago
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>
Attachment #671176 - Attachment is obsolete: true
Attachment #671743 - Flags: review?(robert.bugzilla)
Assignee

Comment 57

7 years ago
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.
Assignee

Comment 58

7 years ago
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.
Attachment #671743 - Flags: review+
Assignee

Comment 59

7 years ago
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.
Assignee

Comment 60

7 years ago
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.
Assignee

Comment 61

7 years ago
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.
Attachment #671177 - Attachment is obsolete: true
Attachment #671930 - Flags: review?(robert.bugzilla)
Assignee

Comment 62

7 years ago
Same as before with same manifest as originally but with an ossupported block
Attachment #671743 - Attachment is obsolete: true
Attachment #671931 - Flags: review?(robert.bugzilla)
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.
Attachment #671930 - Flags: review?(robert.bugzilla)
Assignee

Comment 64

7 years ago
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?
Attachment #671930 - Attachment is obsolete: true
Attachment #672201 - Flags: review?(robert.bugzilla)
Assignee

Comment 65

7 years ago
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.
Attachment #672201 - Flags: review?(robert.bugzilla)
Assignee

Comment 66

7 years ago
Same as last patch but I load the strings from the resource file now.
Attachment #672201 - Attachment is obsolete: true
Attachment #672790 - Flags: review?(robert.bugzilla)
Brian, could you verify the resources look correct?
Attachment #671931 - Attachment is obsolete: true
Attachment #673075 - Flags: review?(netzen)
Assignee

Comment 68

7 years ago
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.
Attachment #673075 - Flags: review?(netzen) → review+
Assignee

Comment 69

7 years ago
Updated resource title string to match sfx.
Carrying forward r+.
Attachment #672790 - Attachment is obsolete: true
Attachment #673121 - Flags: review+
Assignee

Comment 70

7 years ago
https://hg.mozilla.org/mozilla-central/rev/987d2f22f83f
https://hg.mozilla.org/mozilla-central/rev/2f83907a7087
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee

Comment 71

7 years ago
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.
Attachment #673075 - Flags: approval-mozilla-esr10?
Attachment #673075 - Flags: approval-mozilla-beta?
Attachment #673075 - Flags: approval-mozilla-aurora?
Assignee

Comment 72

7 years ago
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.
Attachment #673121 - Attachment description: Patch v5 - 7zip changes for save DLL loading → Patch v5 - 7zip changes for safe DLL loading
Attachment #673121 - Flags: approval-mozilla-esr10?
Attachment #673121 - Flags: approval-mozilla-beta?
Attachment #673121 - Flags: approval-mozilla-aurora?
Blocks: 803515
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 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.
Attachment #673075 - Flags: approval-mozilla-esr10?
Attachment #673075 - Flags: approval-mozilla-esr10+
Attachment #673075 - Flags: approval-mozilla-beta?
Attachment #673075 - Flags: approval-mozilla-beta+
Attachment #673075 - Flags: approval-mozilla-aurora?
Attachment #673075 - Flags: approval-mozilla-aurora+
Attachment #673121 - Flags: approval-mozilla-esr10?
Attachment #673121 - Flags: approval-mozilla-esr10+
Attachment #673121 - Flags: approval-mozilla-beta?
Attachment #673121 - Flags: approval-mozilla-beta+
Attachment #673121 - Flags: approval-mozilla-aurora?
Attachment #673121 - Flags: approval-mozilla-aurora+
(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
Blocks: 804979
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.
Assignee

Comment 79

7 years ago
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.
(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.
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.
Thanks Henrik.
Keywords: verifyme
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.
Assignee

Comment 85

7 years ago
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.
If you can submit a patch within an hour I can compile it within the next hour.
Assignee

Comment 87

7 years ago
Posted patch Patch v1 - ESR10 fix (obsolete) — Splinter Review
Attachment #675385 - Flags: review?(robert.bugzilla)
Posted file (ESR10 SFX) compiled with VC 6 (obsolete) —
This is the binary. Verify that it the resources are correct and r=me on the patch.
Assignee

Updated

7 years ago
Attachment #675387 - Attachment is patch: true
Attachment #675387 - Attachment mime type: application/octet-stream → text/plain
Assignee

Updated

7 years ago
Attachment #675387 - Flags: review+
Assignee

Comment 89

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

Updated

7 years ago
Attachment #675387 - Attachment is patch: false
Assignee

Comment 90

7 years ago
Attachment #675393 - Flags: review+
Assignee

Updated

7 years ago
Attachment #675387 - Attachment is obsolete: true
Whiteboard: [adv-track-main17+][adv-track-esr17+]
reopening based on 809373 for re0investigation
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
Assignee

Comment 95

7 years ago
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.
Shall we remove the fixed value on the tracking flags?
Assignee

Comment 97

7 years ago
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?
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.
Assignee

Comment 99

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

Comment 100

7 years ago
procmon filtering for dlls is a good idea, thanks.
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.
Assignee

Comment 102

7 years ago
Are you maybe using the stub installer?
Assignee

Comment 103

7 years ago
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.
Assignee

Comment 104

7 years ago
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.
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).
Assignee

Comment 106

7 years ago
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.
Assignee

Comment 107

7 years ago
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.
Assignee

Comment 108

7 years ago
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.
Attachment #679208 - Flags: review?(robert.bugzilla)
Assignee

Comment 109

7 years ago
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.
Assignee

Comment 110

7 years ago
Anthony I'm adding qawanted for Comment 109.
Keywords: qawanted
(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.
Assignee

Comment 112

7 years ago
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.
Assignee

Comment 113

7 years ago
It would probably be good to do the steps for the stub as well if you have time on each OS.
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.
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 on attachment 679381 [details]
include cryptbase.dll - m-c dll compiled with VC6

Forgot to make manifest changes
Attachment #679381 - Attachment is obsolete: true
Assignee

Comment 120

7 years ago
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
Attachment #679208 - Flags: approval-mozilla-esr10?
Attachment #679208 - Flags: approval-mozilla-beta?
Attachment #679208 - Flags: approval-mozilla-aurora?
(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.
Assignee

Comment 122

7 years ago
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.
Attachment #679208 - Flags: approval-mozilla-esr10?
Attachment #679208 - Flags: approval-mozilla-beta?
Attachment #679208 - Flags: approval-mozilla-aurora?
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.
Assignee

Comment 124

7 years ago
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.
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.
Assignee

Comment 126

7 years ago
I think that's the correct call for FX17.
Assignee

Comment 127

7 years ago
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.
Assignee

Comment 128

7 years ago
Posted patch Patch v1 - shcore.dll (obsolete) — Splinter Review
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.
Attachment #679652 - Flags: review?(robert.bugzilla)
(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?
Assignee

Comment 130

7 years ago
Ya unfortunately if we commit as we go we need to keep doing new builds of ESR10 and non ESR10 SFXs.
I was under the impression that compiling with VC6 wouldn't delay load shcore.dll due to comment #127. Is that impression incorrect?
Assignee

Comment 132

7 years ago
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.
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!
Assignee

Comment 135

7 years ago
Posted patch cryptbase SFX binary (obsolete) — Splinter Review
[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
Attachment #679383 - Attachment is obsolete: true
Attachment #679935 - Flags: review+
Attachment #679935 - Flags: approval-mozilla-beta?
Attachment #679935 - Flags: approval-mozilla-aurora?
Assignee

Comment 136

7 years ago
[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
Attachment #679388 - Attachment is obsolete: true
Attachment #679936 - Flags: review+
Attachment #679936 - Flags: approval-mozilla-esr10?
Assignee

Updated

7 years ago
Whiteboard: [adv-track-main17+][adv-track-esr17+] → [adv-track-main17+][adv-track-esr17+][leave open]
Assignee

Comment 137

7 years ago
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 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
Attachment #679935 - Flags: approval-mozilla-beta?
Attachment #679935 - Flags: approval-mozilla-beta-
Attachment #679935 - Flags: approval-mozilla-aurora?
Attachment #679935 - Flags: approval-mozilla-aurora+
Assignee

Comment 139

7 years ago
>  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.
(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).
Blocks: 811227
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:Ahughes@mozilla.com/DLL_Hijacking
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.
Assignee

Comment 143

7 years ago
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.
Why not for the 7-zip self-extractor? Seems like it would be fairly simple to get around most of this that way.
Assignee

Comment 145

7 years ago
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.
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.
Assignee

Comment 147

7 years ago
> 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 :(
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.
Assignee

Comment 149

7 years ago
Attachment #679652 - Attachment is obsolete: true
Attachment #679652 - Flags: review?(robert.bugzilla)
Attachment #681278 - Flags: review?(robert.bugzilla)
Assignee

Comment 150

7 years ago
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.
Assignee

Updated

7 years ago
Blocks: 811557
(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:Ahughes@mozilla.com/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.
Alias: CVE-2012-4206
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:Ahughes@mozilla.com/DLL_Hijacking#Results_Summary
Testing is now complete. Please see results here:
https://intranet.mozilla.org/User:Ahughes@mozilla.com/DLL_Hijacking#Results_Summary

I can transpose to this bug if necessary.
Keywords: qawanted, verifyme
Assignee

Comment 155

7 years ago
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:Ahughes@mozilla.com/
> 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.
Assignee

Comment 156

7 years ago
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 on attachment 681278 [details] [diff] [review]
Patch v1 - shcore uxtheme

I should be able to upload a new dll tonight
Attachment #681278 - Flags: review?(robert.bugzilla) → review+
In case you'd like to skip the version that only has cryptbase for esr10.
(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)?
Assignee

Updated

7 years ago
Attachment #681814 - Attachment description: Patch v1 - vc-6 patch file for cryptbase shcore uxtheme SFX for mozilla-central → Patch v1 - vc-6 patch file for new SFX containing dwmapi, cryptbase, shcore (new), and uxtheme (new) - For mozilla-central
Assignee

Comment 162

7 years ago
[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?
Attachment #675385 - Attachment is obsolete: true
Attachment #675393 - Attachment is obsolete: true
Attachment #679936 - Attachment is obsolete: true
Attachment #681802 - Attachment is obsolete: true
Attachment #679936 - Flags: approval-mozilla-esr10?
Attachment #681826 - Flags: approval-mozilla-esr10?
Assignee

Comment 163

7 years ago
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
Attachment #681814 - Flags: approval-mozilla-aurora?
Assignee

Updated

7 years ago
Attachment #673075 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #679935 - Attachment is obsolete: true
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)
Assignee

Comment 166

7 years ago
Yup I think we're good now, just uplifting left.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [adv-track-main17+][adv-track-esr17+][leave open] → [adv-track-main17+][adv-track-esr17+]
Assignee

Comment 167

7 years ago
I think we should keep this as "esr10: affected" unless the full fix to all DLLs gets promoted.
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.
Assignee

Comment 169

7 years ago
(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 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.
Attachment #681814 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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...
Assignee

Comment 173

7 years ago
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?
(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.
Attachment #681826 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
We also need a fix on mozilla-esr17, as you all note.
Assignee

Comment 176

7 years ago
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.
(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
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
(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.
Assignee

Comment 181

7 years ago
Awesome, thanks for testing guys. Definitely the hardest part of this whole bug.
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.
Assignee

Comment 183

7 years ago
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.
Assignee

Comment 184

7 years ago
Full detailed results were summarized here by Anthony:
https://intranet.mozilla.org/User:Ahughes@mozilla.com/DLL_Hijacking
(In reply to Brian R. Bondy [:bbondy] from comment #184)
> Full detailed results were summarized here by Anthony:
> https://intranet.mozilla.org/User:Ahughes@mozilla.com/DLL_Hijacking

Feel free to email me or ping me on IRC if you have any questions about this document.
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.
Whiteboard: [adv-track-main17+][adv-track-esr17+] → [adv-main18+][adv-esr17+][adv-esr10+]
Assignee

Comment 187

7 years ago
> 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.
Group: core-security
Blocks: 883322
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.