Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 748764 - (CVE-2012-1942) Possible Arbitrary Code Execution by Update Service
: Possible Arbitrary Code Execution by Update Service
: sec-critical
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Brian R. Bondy [:bbondy]
: Robert Strong [:rstrong] (use needinfo to contact me)
Depends on:
Blocks: 757711
  Show dependency treegraph
Reported: 2012-04-25 07:19 PDT by Curtis Koenig [:curtisk-use]]
Modified: 2014-06-27 14:50 PDT (History)
14 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1. (3.33 KB, patch)
2012-04-25 09:51 PDT, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
Details | Diff | Splinter Review
Patch v1. Race condition (1.20 KB, patch)
2012-04-25 12:51 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v2. (2.39 KB, patch)
2012-04-27 14:56 PDT, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Curtis Koenig [:curtisk-use]] 2012-04-25 07:19:21 PDT

I notice two security issues in the new Mozilla updater service which came with FF12 on Windows I would like to report.

a) The service path is not quoted which means you can end up with the potential for a planting attack (e.g. if you can create C:\Program.exe it will run it when starting the service).
b) The service itself can be tricked into loading a DLL into the process with SYSTEM privileges and it will run its DLLMain (meaning you can do anything you like). You call LoadLibrary instead of LoadLibraryEx on line 329 of workmonitor.cpp when checking that you have the correct resource string. It should be called to only load it as a resource.

The following C# code will run the service and will load a DLL as long as the files C:\sourcecode\exploitme.dll and C:\sourcecode\updater.exe are the same. The DLL can then in DllMain run something like CreateProcess.

using System;
using System.ServiceProcess;

namespace FFService
    class Program
        static void Main(string[] args)
                ServiceController c = new ServiceController("MozillaMaintenance");

                c.Start(new string[] { "aaa", "software-update", "C:\\sourcecode\\exploitme.dll", "aaaa", "c:\\sourcecode\\", "cccc" });
            catch (Exception e)

I have only tested in XP Pro SP3 but I assume it will work on Win7 and Vista just the same.

If you need more information let me know.

James Forshaw

Principal Security Consultant

Context Information Security

Registered no: 3574635 | Certified to ISO/IEC 27001:2005 (BSI Certificate IS 553326) and ISO 9001:2008 (BSI Certificate FS 581360) | 30 Marsh Wall, London E14 9TP, UK | Mob: +44 (0)7827 248760 | Tel:Â 0207 537 7515 | Fax: 0207 537 1071

The information contained in this email and any attachments may be legally privileged and confidential.  If you are not the intended recipient you are notified that disclosing, copying, distributing or taking any action in reliance on the contents of this information is strictly prohibited.  If you are not the intended recipient please contact us immediately.  Any views or opinions presented in this email are solely those of the author and do not necessarily represent those of the company.
Comment 1 Brian R. Bondy [:bbondy] 2012-04-25 09:51:53 PDT
Created attachment 618330 [details] [diff] [review]
Patch v1.

Thanks for the report.
Comment 2 Brian R. Bondy [:bbondy] 2012-04-25 09:53:22 PDT
(Removed the WinUtils.cpp from my local patch by the way, that was accidentally included)
Comment 3 Robert Strong [:rstrong] (use needinfo to contact me) 2012-04-25 10:58:24 PDT
Comment on attachment 618330 [details] [diff] [review]
Patch v1.

Brian, do the other security measures prevent this from being exploitable?
Comment 4 Brian R. Bondy [:bbondy] 2012-04-25 11:03:38 PDT
For a) no but the user would need write access into C:\ directly.
For b) yes we verify that the updater.exe is the same as the one in the installation directory before loading the library to make sure it is an actual updater.

So both attacks from what I understand require write access to elevated directories.
Comment 5 Curtis Koenig [:curtisk-use]] 2012-04-25 11:25:57 PDT
So anyone running as admin (which is just about anyone on windows) would have rights to these directories I think. On vista and 7 they should get warnings when trying to write to some directories as that may require a UAC decision depending on the settings the user has.
Comment 6 Robert Strong [:rstrong] (use needinfo to contact me) 2012-04-25 11:30:16 PDT
Agreed and I am fairly certain that anyone running as admin that is tricked into setting this up could as easily be tricked into just executing what this would accomplish with the same end result without jumping through as many hoops.
Comment 7 Brian R. Bondy [:bbondy] 2012-04-25 11:32:12 PDT
so I think the main worry is that an admin could execute code as the system account. But any admin can already do this very easily by simply creating a new service that runs as the system account and then starting it.
Comment 8 Curtis Koenig [:curtisk-use]] 2012-04-25 11:40:00 PDT
I thought users were default admin on XP/Vista/7 if there were the first and/or only user of the system? So this is still a large segment of our user population. I do not mean to discount comment 6 but if users are default for admin rights we may be raising the stakes just a bit for attack surface no?
Comment 9 Brian R. Bondy [:bbondy] 2012-04-25 11:42:11 PDT
Just a note that on Windows XP the code in question does not execute.
Also if UAC is off the code does not execute. 
So I believe that writing to those locations would always have a UAC prompt for every case that the code executes.
Comment 10 James Forshaw 2012-04-25 12:02:17 PDT
You do not need to write to c: all paths are attacker supplied and thus controlled I can provider a proper poc tomorrow or compile that c# and change the paths and see where it tries to go using procmon etc
Comment 11 James Forshaw 2012-04-25 12:04:23 PDT
Sorry I only meant b) for a) you would need to write to c: but I have seen machines configured to allow you
Comment 12 Brian R. Bondy [:bbondy] 2012-04-25 12:16:23 PDT
Ya for b) I see now.
The apply to directory is an argument that is supplied.  We do verify that this is a Firefox installation directory but that is in the signature check which happens after this LoadLibrary call.  The user would have to pass a command line argument to the apply to directory that has a copy of the dll they want to load named updater.exe.
Comment 13 James Forshaw 2012-04-25 12:30:21 PDT
That is correct. I also tested Uac admin on win7 with my poc and it works on there to. Of course even if you directly checked for updater.exe in program files you would still have a race condition where the attacker could change the user supplied file before loadlibrary.

I didn't look much further as I had other things but do you ensure that the file you signature check is the same file you exec? By for example copying to a secure location before verification? Or do you run the installed updater in the end? If not there might be a race there as well.
Comment 14 Brian R. Bondy [:bbondy] 2012-04-25 12:51:30 PDT
Created attachment 618401 [details] [diff] [review]
Patch v1. Race condition

I think there is a potential for a race condition there, I added this patch to lock the file for write access before any of the checks happen.
Comment 15 James Forshaw 2012-04-25 13:14:35 PDT
Also worth pointing out that with a), while yes on a vanilla Windows box it should not be possible to write a file to either C:\ or C:\program files, few people have vanilla configurations. I have seen users, admins and OEMs break these security principles before now. You can only really be reasonably confident in the directories you yourself created, not anything else on the file system. 

On a related note, if FF is installed to a non-standard directory does the updater service get installed to a secure directory elsewhere, or does it not get installed at all, or does it get installed along side?
Comment 16 Brian R. Bondy [:bbondy] 2012-04-25 13:17:11 PDT
If installed it will always be installed into program files.   It will only be used if the user does not have write access into the apply to directory.
Comment 17 James Forshaw 2012-04-25 13:44:30 PDT
That fix for the race is not quite correct, for at least two reasons. 

1) The attacker need not provide a path to a file system which honors the write locking. For example if he pointed it to a UNC path then he could change it on the remote server and Windows *could* return the new data at LoadLibrary time.
2) The semantics of LoadLibrary differ from CreateFile when it comes to paths. For example there is no checking done to make the path absolute so if you provide a relative path then LoadLibrary "could" search the path first (an unlikely condition). But probably more importantly, if you supply a filename with no extension then LoadLibrary always appends .DLL to the filename before loading, CreateFile will open the name as is.
Comment 18 Brian R. Bondy [:bbondy] 2012-04-25 14:01:05 PDT
I moved the discussion for Comment 14 - Comment 17 and the second patch to this new bug: Bug 748948
Comment 20 Brian R. Bondy [:bbondy] 2012-04-27 14:56:03 PDT
Created attachment 619188 [details] [diff] [review]
Patch v2.

Last was r+ but changed patch enough to be more simplified.  It does the same thing overall.
Comment 21 Brian R. Bondy [:bbondy] 2012-05-02 12:42:07 PDT
Comment 22 Brian R. Bondy [:bbondy] 2012-05-02 12:44:11 PDT
Comment on attachment 619188 [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, I will wait for 2 days on m-c to ensure updates are working there before pushing to aurora and beta.
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none
Comment 23 Brian R. Bondy [:bbondy] 2012-05-02 12:49:35 PDT
Thank you for the reports and help James
Comment 24 Al Billings [:abillings] 2012-05-03 15:54:04 PDT
Is there a compiled PoC anywhere that can be used for this. How do use the code snippet in comment 0 to see the bug isn't entirely clear to me since I need dlls to use from the C:\sourcecode directory (?), which are copied from somewhere? The updater?
Comment 25 Brian R. Bondy [:bbondy] 2012-05-03 17:13:06 PDT
You can just change the command line parameters to specify whichever directory you want. Comment 0 shows a C# program that you would run to reproduce, you can create a Dll with code that runs in DllMain I believe to reproduce.
Comment 26 James Forshaw 2012-05-04 02:38:33 PDT
Created attachment 620992 [details]
Compiled PoC

Extract the PoC and run it on the command line with the mozilla service installed. updater.exe is just a renamed DLL.
Comment 27 Al Billings [:abillings] 2012-05-04 14:05:40 PDT
Verified fixed in nightly trunk on XP SP3. Running with Firefox 12 and the compiled PoC, calc.exe is run when the PoC is invoked (and running as System). With the nightly, this no longer occurs.
Comment 28 Alex Keybl [:akeybl] 2012-05-06 18:57:41 PDT
Comment on attachment 619188 [details] [diff] [review]
Patch v2.

[Triage Comment]
Looks good on m-c, approving this sg:crit for Aurora 14 and Beta 13.
Comment 30 Daniel Veditz [:dveditz] 2012-05-07 15:45:21 PDT
I'm not entirely convinced this is "critical" for today's Firefox users, because as a practical matter if you've got something running locally that can create c:\Program.exe you can probably trick that user into approving a UAC prompt and then do what you want. But fear of this kind of bug is exactly why we cautiously have chosen not to use the service for users with a limited account. Had this not been reported it would become critical when we enable that in the future; we might as well treat it as such now.
Comment 31 Raymond Forbes[:rforbes] 2013-07-19 18:19:24 PDT

Note You need to log in before you can comment on or make changes to this bug.