Closed Bug 792106 (CVE-2012-4206) Opened 12 years ago Closed 12 years ago

DLL Hijacking - Firefox installer

Categories

(Firefox :: Installer, defect)

Product:
Firefox
Component:
Installer
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla19
Tracking Flags:
Tracking Status
firefox16 - wontfix
firefox17 + wontfix
firefox18 + verified
firefox19 --- verified
firefox-esr10 18+ verified
firefox-esr17 18+ verified

People

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

References

Details

(Keywords: csectype-priv-escalation, reporter-external, sec-high, Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+])

Attachments

(6 files, 23 obsolete files)

trojan DLL loads cmd.exe
481 bytes, application/octet-stream
Details
Patch v5 - 7zip changes for safe DLL loading
5.02 KB, patch
bbondy
: review+
lsblakk
: approval-mozilla-aurora+
lsblakk
: approval-mozilla-beta+
lsblakk
: approval-mozilla-esr10+
Details | Diff | Splinter Review
Patch v1 - Load cryptbase.dll from system32 explicitly at startup of 7zipstub
1.22 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
Patch v1 - shcore uxtheme
1.27 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
Patch v1 - vc-6 patch file for new SFX containing dwmapi, cryptbase, shcore (new), and uxtheme (new) - For mozilla-central
80.29 KB, patch
bbondy
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Patch v1 - vc-6 patch file for new SFX containing dwmapi, cryptbase (new), shcore (new), and uxtheme (new) - For esr10
80.20 KB, patch
akeybl
: approval-mozilla-esr10+
Details | Diff | Splinter Review
Attached file 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.
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
rstrong and I were just discussing the possibility of such an issue a couple days 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.
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.
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?
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.
And I see this clarification was given in Comment 3 :D Sorry for the confusion. I missed it.
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.
@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.
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.
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
Keywords: csec-priv-escalation
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
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.
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.
status-firefox-esr10: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox-esr10: --- → +
tracking-firefox16: --- → ?
tracking-firefox17: --- → +
tracking-firefox18: --- → +
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.
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-critical → sec-high
Attached patch Patch v1 - Remove old 7zip reference only src (obsolete) — — Splinter Review
Attachment #671170 - Flags: review?(robert.bugzilla)
Attached patch Patch v1 - New self extracting file (obsolete) — — Splinter Review
Attachment #671171 - Flags: review?(robert.bugzilla)
Attachment #671172 - Flags: review?(robert.bugzilla)
Attachment #671173 - Flags: review?(robert.bugzilla)
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 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)
Attached patch Patch v1 - New self extracting file (SFX) (obsolete) — — Splinter Review
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)
Also had to add the manifest thing to get it to work with XP+ Common Controls themes
Attachment #671177 - Flags: review?(robert.bugzilla)
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.
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.
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?
(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.
Attachment #671173 - Flags: review-
Attachment #671173 - Attachment is obsolete: true
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.
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?
I have it and will compile it.
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.
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.
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?
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.
Attached patch Patc v2 - New self extracting file (SFX) (obsolete) — — Splinter Review
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)
Attachment #671743 - Flags: review?(robert.bugzilla) → review+
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 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+
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.
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.
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)
Attached patch Patch v3 - New self extracting file (SFX) (obsolete) — — Splinter Review
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)
Attachment #671931 - Flags: review?(robert.bugzilla)
Attached patch Patch v3 - 7zip changes for safe DLL loading (obsolete) — — Splinter Review
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)
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)
Attached patch Patch v4 - 7zip changes for save DLL loading (obsolete) — — Splinter Review
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)
Attachment #672790 - Flags: review?(robert.bugzilla) → review+
Brian, could you verify the resources look correct?
Attachment #671931 - Attachment is obsolete: true
Attachment #673075 - Flags: review?(netzen)
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+
Updated resource title string to match sfx. Carrying forward r+.
Attachment #672790 - Attachment is obsolete: true
Attachment #673121 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/987d2f22f83f https://hg.mozilla.org/mozilla-central/rev/2f83907a7087
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox19: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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?
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.
Should also let SeaMonkey know.
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
Blocks: 805032
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.
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.
status-firefox-esr10: wontfix → affected
tracking-firefox-esr10: + → 17+
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.
Attached patch Patch v1 - ESR10 fix (obsolete) — — Splinter Review
Attachment #675385 - Flags: review?(robert.bugzilla)
Attachment #675385 - Flags: review?(robert.bugzilla) → review+
Attached 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.
Attachment #675387 - Attachment is patch: true
Attachment #675387 - Attachment mime type: application/octet-stream → text/plain
Attachment #675387 - Flags: review+
Thanks I downloaded the file and verified it, I'll package it in a patch and re-attach.
Attachment #675387 - Attachment is patch: false
Attached patch (ESR10 SFX) compiled with VC 6 (obsolete) — — Splinter Review
Attachment #675393 - Flags: review+
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)
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?
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.
I'm using win7 x64 with a 32bit Mozilla build. Well all installers are themselves 32bit so that shouldn't even matter.
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.
Are you maybe using the stub installer?
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.
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).
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.
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.
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)
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.
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.
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.
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.
Attachment #679208 - Flags: review?(robert.bugzilla) → review+
Creating a dll for esr next
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
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.
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.
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.
status-firefox17: affected → wontfix
I think that's the correct call for FX17.
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.
Attached 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?
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?
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!
Attached 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?
Attached patch cryptbase.dll sfx binary for esr10 (obsolete) — — Splinter Review
[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?
Whiteboard: [adv-track-main17+][adv-track-esr17+] → [adv-track-main17+][adv-track-esr17+][leave open]
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+
> 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.
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.
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.
> 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.
Attached patch Patch v1 - shcore uxtheme — — Splinter Review
Attachment #679652 - Attachment is obsolete: true
Attachment #679652 - Flags: review?(robert.bugzilla)
Attachment #681278 - Flags: review?(robert.bugzilla)
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.
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
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.
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)?
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
[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?
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?
Attachment #673075 - Attachment is obsolete: true
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)
status-firefox19: affected → fixed
Yup I think we're good now, just uplifting left.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [adv-track-main17+][adv-track-esr17+][leave open] → [adv-track-main17+][adv-track-esr17+]
I think we should keep this as "esr10: affected" unless the full fix to all DLLs gets promoted.
status-firefox-esr10: fixed → affected
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.
(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...
tracking-firefox-esr17: --- → ?
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.
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
Status: RESOLVED → VERIFIED
status-firefox-esr10: fixed → verified
status-firefox18: fixed → verified
status-firefox19: fixed → verified
status-firefox-esr17: fixed → verified
(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.
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.
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.
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+]
> 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+
Component: NSIS Installer → Installer
Product: Toolkit → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: