Last Comment Bug 830134 - (CVE-2013-0797) The updater.exe loads the cryptsp.dll from the update directory while perfoming an update.
(CVE-2013-0797)
: The updater.exe loads the cryptsp.dll from the update directory while perfomi...
Status: VERIFIED FIXED
[adv-main20+][adv-esr1705+]
: csectype-priv-escalation, sec-high
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: 18 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla22
Assigned To: Brian R. Bondy [:bbondy]
: Matt Wobensmith [:mwobensmith][:matt:]
: Robert Strong [:rstrong] (use needinfo to contact me)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-13 11:40 PST by Holger Fuhrmannek
Modified: 2014-07-24 14:38 PDT (History)
11 users (show)
dveditz: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
+
verified
20+
verified
unaffected


Attachments
proof of concept (169.80 KB, application/octet-stream)
2013-01-13 11:42 PST, Holger Fuhrmannek
no flags Details
Patch v1. (1.30 KB, patch)
2013-02-21 12:34 PST, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr17+
abillings: sec‑approval+
Details | Diff | Splinter Review

Description Holger Fuhrmannek 2013-01-13 11:40:52 PST
Besides fixed Bug 750850 the updater.exe in FF 18.0 is vulnerable to a LoadLibrary-based attack. The exe loads the cryptsp.dll from the update directory while perfoming an update.

Reproduce:

I have attached a poc to perfom an update of 18.0 to 18.0 (feature or bug?) which I have tested on win7 pro 32bit and 64bit with Firefox 18.0. 

1.) Download the poc and extract the files.
2.) Download "http://releases.mozilla.org/pub/mozilla.org/firefox/releases/latest/update/win32/en-US/firefox-18.0.complete.mar" and save the file as "update.mar" in the poc directory.
3.) Execute:

	sc start MozillaMaintenance MozillaMaintenance software-update <path_to_poc_dir+\updater.exe> <path_to_poc_dir> <path_to_firefox_dir+\updated> -1

for example:

	sc start MozillaMaintenance MozillaMaintenance software-update "C:\Users\Ash\Desktop\poc\updater.exe" "C:\Users\Ash\Desktop\poc" "C:\Program Files (x86)\Mozilla Firefox\updated" -1

to start the maintainservice. The fake "cryptsp.dll" executes system("calc") in DLLMain so you will see a calc.exe and a cmd.exe running with system rights in the process list. Kill the calc.exe to finish the "update".

The cryptsp.dll from the system directory should be loaded.
Comment 1 Holger Fuhrmannek 2013-01-13 11:42:40 PST
Created attachment 701596 [details]
proof of concept
Comment 2 David Chan [:dchan] 2013-01-14 13:32:46 PST
:bbondy
This seems like a duplicate of bug 811557? I was unsure about the comment regarding NSIS .onInit(). I'm guessing that is when the paths are setup for correct LoadLibrary calls.
Comment 3 Brian R. Bondy [:bbondy] 2013-01-14 14:58:00 PST
We should do a similar analysis as we did for the installers, but for updater.exe and for maintenanceservice.exe across all OS. CC'ing Anthony to see if he can pull in resources to help.
Comment 4 Brian R. Bondy [:bbondy] 2013-01-15 06:24:02 PST
This is not the same as bug 811557 by the way, it probably has to do with a delay loaded DLL which is implicitly loaded from the current dir on some OS.  We'd have to add code to manually load it from the Windows directory.
Comment 5 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-01-16 09:01:30 PST
I'm going to be on vacation for the next week or so (Jan 20-28).

Matt, can you please try to coordinate some testing around this? My suggestion would be to pull in Kamil Jozwiak to assist.
Comment 6 Daniel Veditz [:dveditz] 2013-01-16 10:43:02 PST
Did we not SetDLLDirectory("") in this utility? Sigh, seems like we forget to fix this every time we create something new...
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2013-01-16 10:45:58 PST
This creation of something new was added to cvs on 2005-05-04 13:58
Comment 8 Brian R. Bondy [:bbondy] 2013-01-19 19:42:56 PST
Yup, updater is not a new component, I only suggested we check maintenanceservice at the same time, but the report is not for maintenanceservice.  Also on second thought we don't need to check maintenanceservice because no one can put a dll next to it since it's already in a high integrity location.

SetDLLDirectory wouldn't fix this, we don't call LoadLibrary.  These attacks are for delay loaded dlls which are not in the safe list of dlls on some platforms.  We can do that as a precaution inside a different bug though.

The issue is reproducible even without the service, but it's easiest to reproduce as per the original instructions in comment 0.

CCing Kamil for help.
Ideally we should test this on XPx86, XPx64, Vistax86, Vistax64, Win7x86, Win7x64, and Win8x86, win8x64.

The test is the same as the one we did for installer on each platform. We should use procmon to see which dlls are loaded, and then subtract out the safe dlls from that list.  Then from the remaining dlls, rename the PoC to each DLL and try to reproduce.   

The fixes will be very simple, just modify this line to add the extra DLLs:
http://dxr.mozilla.org/mozilla-central/toolkit/mozapps/update/updater/loaddlls.cpp.html#l15
Comment 9 Brian R. Bondy [:bbondy] 2013-01-19 19:50:58 PST
The reason we should test on each platform by the way is because the safe list and loaded DLLs differ on each platform. The test on each platform turned out to be useful for the similar installer bug.

There could be another similar issue with helper.exe since we run that as an elevated process when setting the default browser. But only when the user installs outside of Program files, so I'm not sure if we care about that case.  We offer the option to install at an alt location though, so we should probably check it too. (In a different bug that has lower priority)
Comment 10 Al Billings [:abillings] 2013-01-25 14:09:05 PST
Is this really unconfirmed?
Comment 11 Matt Wobensmith [:mwobensmith][:matt:] 2013-01-25 14:31:52 PST
As per personal investigation and comment 8, this issue can be reproduced.

I am working on a test matrix and plan.
Comment 12 Matt Wobensmith [:mwobensmith][:matt:] 2013-02-19 10:59:33 PST
Quick summary:
We have reproduced this DLL privilege escalation on the configs below.

Win 7 32-bit 	cryptsp.dll, cryptbase.dll
Win 7 64-bit 	cryptsp.dll
Win 8 32-bit 	cryptsp.dll, cryptbase.dll
Win 8 64-bit 	cryptsp.dll


On the two configs below, no privilege escalations occurred, but some DLLs caused the updater to crash:

Vista 32-bit 	msasn1.dll, userenv.dll, secur32.dll
Vista 64-bit 	msasn1.dll, userenv.dll

I can share a spreadsheet with more details.
Comment 13 Brian R. Bondy [:bbondy] 2013-02-19 11:21:49 PST
Great, thanks for the info and work guys.
Comment 14 Brian R. Bondy [:bbondy] 2013-02-21 12:34:42 PST
Created attachment 716719 [details] [diff] [review]
Patch v1.

As expected SetDllDirectory has no effect. I'll file a separate bug to do that as a precaution though.
Comment 15 Brian R. Bondy [:bbondy] 2013-02-21 12:43:23 PST
SetDllDirectory precaution bug was filed and fixed here: bug 830134.
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-02-28 19:47:49 PST
https://hg.mozilla.org/mozilla-central/rev/9a08bc57d366
Comment 19 Daniel Veditz [:dveditz] 2013-03-07 16:37:38 PST
This bug should have requested sec-approval? before landing. I assume the answers to the questions we were going to ask are that it affects all the older branches we currently support, and we'll need to land this everywhere.
Comment 20 Al Billings [:abillings] 2013-03-07 16:38:32 PST
Brian and Ryan, this needed to get explicit sec-approval+ before being checked in since it affects just about every version of Firefox and is a sec-high. Can you please watch for this when checking in security issues?
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-03-07 16:44:35 PST
(In reply to Al Billings [:abillings] from comment #20)
> Brian and Ryan, this needed to get explicit sec-approval+ before being
> checked in since it affects just about every version of Firefox and is a
> sec-high. Can you please watch for this when checking in security issues?

For my part, this was merged over automatically from inbound during one of the daily merges. There's really nothing I could have done differently unless you expect me to go through every single push to inbound to see if it really should have landed or not before any daily merges to m-c can take place.
Comment 22 Daniel Holbert [:dholbert] 2013-03-07 16:47:51 PST
(I think Al just misinterpreted Comment 17 as RyanVM actively landing this on m-c, instead of the essentially-automated inbound-to-central merge that it actually was.)
Comment 23 Brian R. Bondy [:bbondy] 2013-03-07 16:55:39 PST
Is there a new policy with "sec-approval?" requests? I haven't heard I needed it for sec-high bugs until now and I've done I'm sure over a dozen such bugs in the past. Thanks in advance for the clarification.

Yes it affects all branches.
Comment 24 Brian R. Bondy [:bbondy] 2013-03-07 16:58:33 PST
Is the concern that someone can watch the fix land on m-c and then attack previous versions? Note for updater bugs we also have a concern that bugs should definitely land on m-c first so we don't accidentally ever break anything more than m-c Nighlty updates in a disaster scenario. I can see value in landing close to a migration though.
Comment 25 Brian R. Bondy [:bbondy] 2013-03-07 17:05:50 PST
Found doc here (Oct '12):
https://wiki.mozilla.org/Security/Bug_Approval_Process
You can ignore questions above.
Comment 26 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-08 15:39:13 PST
Please nominate for branch uplift now that its been on central for over a week. Let's get this landed prior to Tuesday Mar 12th go to build on beta 5.
Comment 27 Brian R. Bondy [:bbondy] 2013-03-08 15:50:45 PST
Is this bug sec-approval+ for landing on the other branches?
Comment 28 Brian R. Bondy [:bbondy] 2013-03-08 15:54:21 PST
Comment on attachment 716719 [details] [diff] [review]
Patch v1.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No new comments, but surrounding comments are clear what the purpose of the patch is.

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?
NA

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Low risk, no changes for other branches.

How likely is this patch to cause regressions; how much testing does it need?
Low since it has been on m-c for a week.

Extra note: This landed already on m-c because I didn't know the newer sec policy.
Comment 29 Daniel Holbert [:dholbert] 2013-03-08 15:56:12 PST
sec-approval only gets you approval for landing on trunk, not for branches.

You need to request approval-mozilla-beta & aurora (assuming those are the branches that it needs to be backported to).
Comment 30 Brian R. Bondy [:bbondy] 2013-03-08 15:56:46 PST
Comment on attachment 716719 [details] [diff] [review]
Patch v1.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is a sec-high.
User impact if declined: DLL can be used by a low integrity process to get access to high integrity stuff.
Fix Landed on Version: 22
Risk to taking this patch (and alternatives if risky): Low it has been on m-c for a week. 
String or UUID changes made by this patch: none.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 31 Daniel Holbert [:dholbert] 2013-03-08 16:00:35 PST
(it's a lot of overhead, I know; basically, the flags are:
 * sec-approval is asking: Am I allowed to land this bug on m-c, and effectively make the bug's existence public, given that it's a security bug that affects users on release builds?  This is a decision made by the security group.

 * approval-mozilla-[BRANCH] is asking: Am I allowed to land this bug on [BRANCH]? (i.e. is this critical enough that we need to backport it to the given branch, and tested well enough that we're comfortable backporting it, given where we are in that branch's release cycle)  This is a decision made by the release managers.)
Comment 32 Brian R. Bondy [:bbondy] 2013-03-08 16:04:27 PST
Very familiar with the approval-mozilla-[BRANCH] approvals.  New to sec-approval, but I understand it for the normal case. 

But we're in an abnormal case of already having it on m-c without sec-approval+ so I'd appreciate a sec-approval+ for this abnormal case. For example maybe m-c has too many commits for a lot of people to look at, but maybe attackers watch mozilla-beta more because they know that most of the stuff that lands there is security issues.
Comment 33 Al Billings [:abillings] 2013-03-08 16:06:17 PST
Comment on attachment 716719 [details] [diff] [review]
Patch v1.

We only need to give a patch sec-approval+ once (usually for trunk but whatever the main branch is). Then, after checkin, we nominate it for affected branches if we want to take it there. 

This went in without explicit approval so I understand the confusion but once we checked it in, the cat was out of the bag so no need to renominate at that point. The main thrust of the sec-approval process is to reduce the window of exposure and to make sure we're being very explicit when we take security fixes since they can potentially compromise users (which is why it affects sec-high and sec-critical bugs that are not just on trunk).
Comment 34 Al Billings [:abillings] 2013-03-08 16:09:04 PST
(In reply to Brian R. Bondy [:bbondy] from comment #32)
> Very familiar with the approval-mozilla-[BRANCH] approvals.  New to
> sec-approval, but I understand it for the normal case. 

And to be clear, I'm not trying to give you a hard time, Brian. A bunch of folks missed the notes about this process or just didn't run into before now. So, live and learn. :-)
 
> But we're in an abnormal case of already having it on m-c without
> sec-approval+ so I'd appreciate a sec-approval+ for this abnormal case. For
> example maybe m-c has too many commits for a lot of people to look at, but
> maybe attackers watch mozilla-beta more because they know that most of the
> stuff that lands there is security issues.

And, in fact, we do suspect people watch branch check-ins (probably ESR) for changes referencing non-public bugs to look for security issues. That was part of what caused this process to happen last year.

It's all good at this point.
Comment 35 Brian R. Bondy [:bbondy] 2013-03-08 16:11:06 PST
sure I was just going by this line of the security bug approval case since this is an abnormal case and since some people may only watch less high traffic branches that mostly only get security pushes, we could let a bigger cat out of the bag.

> If developers are unsure about a bug and it has a patch ready, just 
> mark the sec-approval flag to '?' and move on.  
https://wiki.mozilla.org/Security/Bug_Approval_Process

Thanks for the sec-approval+.
Comment 36 Brian R. Bondy [:bbondy] 2013-03-08 16:24:17 PST
By the way could someone also clarify if it's OK to push such patches before sec-approval+ through try tests?  Or better yet update https://wiki.mozilla.org/Security/Bug_Approval_Process with that policy.  I think try gets a small constant factor of more pushes and people could watch that too before a fix is even fully ready. If this is common maybe it would be useful to have a private try repo / tbpl page.
Comment 37 Alex Keybl [:akeybl] 2013-03-08 16:40:19 PST
Comment on attachment 716719 [details] [diff] [review]
Patch v1.

Approving for Aurora initially - all updater changes should really ride up the trains, even if we just stagger the uplifts.
Comment 38 Brian R. Bondy [:bbondy] 2013-03-08 17:29:31 PST
Kamil would you mind verifying this is fixed on a recent mozilla-central build?
https://bugzilla.mozilla.org/show_bug.cgi?id=830134#c12
Just on either of win7 x86 or win8 x86 is enough.
Comment 39 Brian R. Bondy [:bbondy] 2013-03-08 19:37:10 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/94b026146a99
Comment 40 Kamil Jozwiak [:kjozwiak] 2013-03-10 20:15:26 PDT
Went through the following cases to ensure that the issue has been resolved:
- Win 7 32-bit - cryptsp.dll, cryptbase.dll (Used Windows 7 Home Premium SP1 x86)

Used the following build to test in the latest Nightly M-C build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-03-10-03-09-06-mozilla-central/

- Passed without any issues (calc.exe and cmd.exe not launched using High Integrity)

Used the following build to test in the latest Nightly Aurora build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-03-10-04-20-12-mozilla-aurora/

- Passed without any issues (calc.exe and cmd.exe not launched using High Integrity)
Comment 41 Kamil Jozwiak [:kjozwiak] 2013-03-10 20:34:44 PDT
Changed: status-firefox21 & status-firefox22 to verified
Comment 42 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-11 09:58:14 PDT
Comment on attachment 716719 [details] [diff] [review]
Patch v1.

Approving for beta/esr17 to close the loop here, given this is sec-high and has landed without incident on Aurora.
Comment 44 Kamil Jozwiak [:kjozwiak] 2013-03-12 11:35:55 PDT
Verified esr17 using the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-03-12-03-45-02-mozilla-esr17/

- Passed without any issues (calc.exe and cmd.exe not launched using High Integrity)

Waiting for the beta release so I can finish verifying.
Comment 45 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-03-14 13:21:13 PDT
Thanks Kamil, beta builds should now be available.
Comment 46 Kamil Jozwiak [:kjozwiak] 2013-03-14 16:34:52 PDT
Verified latest beta using the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/win32/en-US/

- Passed without any issues (calc.exe and cmd.exe not launched using High Integrity)

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