750850 - (CVE-2012-1943) Updater.exe loads wsock32.dll from application directory

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[James Forshaw]

Attachment: poc.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

The updater.exe from FF12 loads the wsock32.dll from the application directory during initialization, this is an issue when combined with the maintenance service as it would allow an attacker to privilege escalate to local system without any prompts. As it is the real updater.exe doing the loading it circumvents all the protections in place to ensure it is signed etc.

I have attached a poc which I have tested on win7 64bit with FF12. You need to extract the zip, run the FFServer.exe file with the path to your installation of FF12 on the command line. It should then start the maintenance service (which should be enabled) and it will run updater.exe. This then loads the fake wsock32.dll from the current directory and a localsystem copy of calc should appear in the process list. Wsock32.dll is just a simple dll which runs calc in DllMain. All it needed was to export oridinal 14 to ensure the loader loaded it.


Actual results:

A fake wsock32.dll is loaded from the updater directory which loads into the local system privileged version of updater.exe. At this point it is possible to do anything you like with elevated permissions.


Expected results:

It shouldn't be loading from the current directory (not that it has much choice).

Comment 1

[Brian R. Bondy [:bbondy]]

If it is updater.exe that loads the dll isn't this just as exploitable with updater.exe without the service? The user will get a UAC prompt that the signed Mozilla updater would like to run, they click yes to elevate because they trust mozilla, then it loads the dll.

Comment 2

[Brian R. Bondy [:bbondy]]

Looks like the exports we're using it for is just for the ntohl calls in 
toolkit\mozapps\update\updater\bspatch.cpp

I guess the same thing could be done for SHLWAPI.dll, CRYPT32.dll or shell32.dll?
How about ADVAPI32.dll, Kernel32.dll, user32.dll, gdi32.dll, comctl32.dll?

Comment 3

[Brian R. Bondy [:bbondy]]

If that's true then I guess it would follow that basically any exe that requests elevated access via UAC, or is run elevated, from something other than NTFS with proper ACL's set is insecure.

Comment 4

[James Forshaw]

Some DLLs are in the knowndlls list in the registry, I think only crypt32.dll (along with wsock32.dll) is not in the known list and would in theory be just as vulnerable. (see HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session Manager\KnownDLLs). 

And it would be an issue with UAC prompt, but I didn't necessarily consider that scenario to be as serious as silent privilege escalation.

I assume this is something which could not be exploited remotely. I haven't looked at how the files are downloaded, but I assume they are SSL protected or similar?

And yes to be fair it would apply to an awful lot of other applications, especially if not in a secure location. But with the maintenance service you are making a trust decision to run an application from an untrusted location, even if you trust the application itself.

Comment 5

[(no longer active)]

Usually with this kind of stuff the risk is real if Firefox is installed in a network location, which is possible in some corporate/school environments.

I think this bug is real.  For any DLLs that are not in the known DLLs list and we load in updater, we want to stop linking statically, and dynamically load them after calling SetDllDirectory("").

Comment 6

[(no longer active)]

Also, the impact of this is "remote" code execution (that is, running code put on the network share) with system privileges, so I'm inclined to call this sg:crit.

Comment 7

[Brian R. Bondy [:bbondy]]

Thanks for explaining about the knownDLLs list in the registry.  I guess if they are in this list then they go direct to the Windows directory?  

So I think the best fix here is just to use our own implementation of ntohl correct?  It's strange to me that this dll is not in the known dlls list.

An alternate fix would be for us to add this dll to that list since I think it should already be there, but I think the first fix is better since I don't know what side effects doing this would have.

Comment 8

[Brian R. Bondy [:bbondy]]

I agree by the way that the service case is worse because it can be done automated without user intervention.  But the updater.exe only way is pretty bad as well since our only defense would be to say to the user that it's your fault for trusting us.

Comment 9

[James Forshaw]

Yes the knowndlls indicate that those DLLs should always be loaded from the windows system directory. I think the original rationale was not security as such, more compatibility as they are critical DLLs to the running of the system and loading multiple different versions could be problematic (Dlls are scoped on path so you could end up with say two kernel32.dlls otherwise). I would probably say it isn't a good idea to start playing with the KnownDlls list though otherwise you risk compat problems for other applications. 
 
One reason wsock32.dll is not in the list is it is a compat wrapper for ws2_32.dll (which is in the list). If you look at the DLL itself it is actually just a forwarder for the real DLL. As I mentioned though fixing ntohl won't necessarily help, for a start I believe crypt32.dll would also be vulnerable as it isn't in the list. Also I believe that any runtime load would also run the risk of causing issues, and those might be initiated either explicitly by your code (don't think there are any though atm) or implcititly by performing some other action, e.g. using shell APIs which load COM. Even standard techniques to prevent binary planting won't necessarily be effective, such as calling SetDllDirectory() because all that does is neuter the PATH environment search not loading from the application directory.

I will freely admit I am not sure how to fix this in a simple way which covers all bases and won't run the risk of accidentally becoming unstuck due to either later changes in the updater code or installed software. I believe the official recommendation from MS is basically if you run an exe at elevated privileges it should only be from somewhere which is trusted, but lets face it I am sure most installers in existence break that very principle.

Comment 10

[Brian R. Bondy [:bbondy]]

I would say we should just copy it to the maintenance service directory as you mentioned in the last bug, but this doesn't cover the case of Comment 8.

Comment 11

[James Forshaw]

Perhaps the only way might be to only run the updater already installed in the FF directory (referenced probably from a secure location in the registry). I assume that in the service and UAC case this would prevent this issue (as you wouldn't be running the service or UAC if you were able to write to the installed directory). It would also avoid the race condition issues etc. and might actually simplify the service code as you wouldn't need to worry about it actually being signed in the first place, at least not the updater, you would still need to check update file itself. 

In the service case I could imagine this wouldn't make a difference as you are already ensuring that the new updater you downloaded matches the one you have installed into program files. But for UAC case, it might block your upgrade path going forward. And I guess this is all things you guys have discussed previously.

Comment 12

[(no longer active)]

Hmm, we're technically not supposed to modify the list of known DLLs, and doing that requires privileges in the first place anyways.

I think the correct way to fix this is what I said in the last paragraph of comment 5.  No need to use our own implementation of ntohl or anything.

Comment 13

[Ian Melven :imelven]

(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> Hmm, we're technically not supposed to modify the list of known DLLs, and
> doing that requires privileges in the first place anyways.
> 
> I think the correct way to fix this is what I said in the last paragraph of
> comment 5.  No need to use our own implementation of ntohl or anything.

That sounds good to me - I see James' caveat about it not affecting DLL's loaded implicitly via COM etc but it should address loading from the current directory. James also makes another good point that the runtime loading code should be reviewed carefully - Thanks for the reports and the advice/discussion, James !

Comment 14

[(no longer active)]

(In reply to Ian Melven :imelven from comment #13)
> (In reply to Ehsan Akhgari [:ehsan] from comment #12)
> > Hmm, we're technically not supposed to modify the list of known DLLs, and
> > doing that requires privileges in the first place anyways.
> > 
> > I think the correct way to fix this is what I said in the last paragraph of
> > comment 5.  No need to use our own implementation of ntohl or anything.
> 
> That sounds good to me - I see James' caveat about it not affecting DLL's
> loaded implicitly via COM etc but it should address loading from the current
> directory. James also makes another good point that the runtime loading code
> should be reviewed carefully - Thanks for the reports and the
> advice/discussion, James !

AFAIK, SetDllDirectory affects the search path for *all* DLLs.  The only ones which will not be affected by it are the ones loaded before main(), specifically the statically linked DLLs...

Comment 15

[James Forshaw]

As far as I understand it all calling SetDllDirectory with an empty string does is affect the use of the current working directory. This is distinct from the application's directory and need not be the same. As the attacker would control the applications directory it matters little to where the current directory is pointed to.

This is based on http://msdn.microsoft.com/en-us/library/windows/desktop/ms686203(v=vs.85).aspx the remarks indicate it just affects step two, where normally it would search the current directory. Step one is still the application's directory.

Comment 16

[(no longer active)]

Oh, wait.  Are we talking about the attacker controlling the current directory or the application's directory?  SetDllDirectory("") fixes the former.  In the latter case, we've already lost, and there is practically nothing that we can do about it.

Comment 17

[Brian R. Bondy [:bbondy]]

I found a way to fix this, we have to delay load wsock32.dll and other DLLs not in the known DLL list.  Then we have to simply load the library from an absolute file path at startup.

The delay load dll linker option makes it so the dll isn't loaded until the first symbol is used.  So as long as we do the manual load at the start of main we're good.  I confirmed already it fixes the PoC.

Comment 18

[Brian R. Bondy [:bbondy]]

I tried SetDllDirectory earlier today btw ehsan but it didn't fix the problem.  At least for implicitly loaded DLLs it will not fix it.

Comment 19

[Brian R. Bondy [:bbondy]]

Also the attach is for the application directory and not the current directory.

Comment 20

[James Forshaw]

Ah yes, the bug report was in relation specifically to the updater's use in the new maintenance service for silent installation and its potential for a privilege escalation route. In this case the service will run the updater from the attacker's controlled directory leading to execution at SYSTEM from any user account. However I would say that calling SetDllDirectory("") would be a useful thing to add anyway for the general case to any application.

Of course some have argued that if someone is running code on your machine then you have lost anyway, but I don't hold that same view (otherwise why have privilege separation at all) and technically this doesn't strictly require an attacker to run code, only the ability to drop a file somewhere.

The delay loading is one option, my concern would be robustness of the fix as you would have to track which DLLs were in use for every build (I guess). If the compiler changed something at some point to load new DLLs or someone adds extra code and doesn't realise they need to update the list I would be concerned the fix could break.

Comment 21

[Ian Melven :imelven]

The delay loading + explicit loading seems like a good approach - again James' point about this being a bit brittle is well taken. It seems unlikely to me  there's some way to throw an error if a DLL isn't delay loaded at compile/link time but if there was, we should use that (much the same as we run binscope on every Windows build to make sure non-ASLR etc DLL's haven't been introduced)

James, i agree with your view and believe we certainly want to protect against privilege escalation to SYSTEM via this feature

Comment 22

[James Forshaw]

I am sure there is probably something which will check for delay loads of the DLLs, or at least could be easily written. Of course that probably isn't the issue, the problem comes not if isn't delay loaded but it isn't pre-loaded by the code. And of course this still doesn't fix the potential for implicit loads via loading of DLLs which you don't directly link against, which could change depending on what software a user has installed (AV being a special problem with their habit of loading DLLs into all processes).

Comment 23

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

This patch adds delay loading of the DLLs in question and then load those same DLLs from an absolute path so no search path is used. 

Since the DLLs are already loaded we don't delay load them at all at the time the first symbol is used.

Comment 24

[Brian R. Bondy [:bbondy]]

I posted this to investigate Comment 22 by the way: 
Bug 751931 - Investigate implicitly loaded DLLs into elevated processes

Comment 25

[(no longer active)]

Comment on attachment 621081 [details] [diff] [review]
Patch v1.

Review of attachment 621081 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below addressed.

::: toolkit/mozapps/update/updater/loaddlls.cpp
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#pragma once

You don't need this in a cpp file.  :-)

@@ +28,5 @@
> +      // No need to re-NULL terminate
> +    }
> +
> +    // For each known DLL ensure it is loaded fromt the system32 directory
> +    for (size_t i = 0; i < sizeof(delayDLLs) / sizeof(delayDLLs[0]); ++i) {

Nit: use ArrayLength please.

@@ +30,5 @@
> +
> +    // For each known DLL ensure it is loaded fromt the system32 directory
> +    for (size_t i = 0; i < sizeof(delayDLLs) / sizeof(delayDLLs[0]); ++i) {
> +      size_t fileLen = wcslen(delayDLLs[i]);
> +      wcscpy(systemDirectory + systemDirLen, delayDLLs[i]);

Please use wcsncpy.

@@ +33,5 @@
> +      size_t fileLen = wcslen(delayDLLs[i]);
> +      wcscpy(systemDirectory + systemDirLen, delayDLLs[i]);
> +      systemDirectory[systemDirLen + fileLen] = L'\0';
> +      LPCWSTR fullModulePath = systemDirectory; // just for code readability
> +      LoadLibrary(fullModulePath);

LoadLibraryW.

@@ +36,5 @@
> +      LPCWSTR fullModulePath = systemDirectory; // just for code readability
> +      LoadLibrary(fullModulePath);
> +    }
> +  }
> +} _do_it_now_please_and_thank_you;

Please use a better name.  ;-)  Oh, and don't use an underscore prefix.

Comment 26

[Brian R. Bondy [:bbondy]]

> +#pragma once

Oops I was including the cpp code originally then moved it to the makefile :S

Comment 27

[Brian R. Bondy [:bbondy]]

> Nit: use ArrayLength please.

Although I prefer using it, we don't currently include this dependency in updater code and the current way is consistent with the over 70 other uses.... including some of your bgupdate code! :P

Comment 28

[(no longer active)]

(In reply to Brian R. Bondy [:bbondy] from comment #27)
> > Nit: use ArrayLength please.
> 
> Although I prefer using it, we don't currently include this dependency in
> updater code and the current way is consistent with the over 70 other
> uses.... including some of your bgupdate code! :P

Heh, you caught me red-handed there!  OK, please ignore that comment.

Comment 29

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Implemented nits
Carrying forward r+

Comment 30

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2'.

Fixed comment typo

Comment 31

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/06f397d1f997

Comment 32

[Brian R. Bondy [:bbondy]]

Comment on attachment 621097 [details] [diff] [review]
Patch v2'.

[Approval Request Comment]
Regression caused by (bug #): 481815
User impact if declined: unelevated process can run code elevated
Testing completed (on m-c, etc.): I tested updates on elm.
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 621097 [details] [diff] [review]
Patch v2'.

low risk fix, let's get this landed today and it can go into tomorrow's beta3 go-to-build

Comment 34

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/f4aacb7f246f
http://hg.mozilla.org/releases/mozilla-beta/rev/64df0e604ded

Comment 35

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

Verified fixed with PoC on Win 7 x64. I ran with both Firefox 12 (which showed the bug) and the 5/13 nightly, which did not.

Comment 36

[u279076]

I'm having issues getting the PoC to work in Firefox 12, can someone help me?

Command from cmd.exe:
c:\Users\mozilla\Documents\poc>FFService.exe "c:\Program Files (x86)\Mozilla Fir
efox\firefox.exe" 

Does not seem to do anything...

Comment 37

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

If you watch the task manager on Windows, you should see calc.exe start after running it, as I recall.

Comment 38

[Brian R. Bondy [:bbondy]]

Make sure show processes from all users is clicked.

Comment 39

[u279076]

Yeah, no calc.exe is appearing in Task Manager, even showing processes from all users. Again, tested with Firefox 12 on Win7 64-bit running the command from cmd.exe:

FFService.exe "c:\Program Files (x86)\Mozilla Firefox\firefox.exe"

Also tried the following with the same result:
FFService.exe "c:\Program Files (x86)\Mozilla Firefox\"

Comment 40

[u279076]

Oh wait, I see now. Running the following worked:
FFService.exe "c:\Program Files (x86)\Mozilla Firefox"

I'll use this to test fixed builds now.

Comment 41

[u279076]

Verified fixed in Firefox 15.0a1 2012-06-01, Firefox 14.0a2 2012-06-01, and Firefox 13.0b7.