830134 - (CVE-2013-0797) The updater.exe loads the cryptsp.dll from the update directory while perfoming an update.

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Holger Fuhrmannek]

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]

Attachment: proof of concept

Comment 2

[David Chan [:dchan]]

: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]]

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]]

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

[u279076]

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]]

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 (they/them - no direct email)]

This creation of something new was added to cvs on 2005-05-04 13:58

Comment 8

[Brian R. Bondy [:bbondy]]

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]]

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 - ex-MoCo]]

Is this really unconfirmed?

Comment 11

[Matt Wobensmith [:mwobensmith][:matt:]]

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:]]

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]]

Great, thanks for the info and work guys.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: 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]]

SetDllDirectory precaution bug was filed and fixed here: bug 830134.

Comment 16

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a08bc57d366

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a08bc57d366

Comment 19

[Daniel Veditz [:dveditz]]

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 - ex-MoCo]]

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]]

(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]]

(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]]

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]]

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]]

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]

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]]

Is this bug sec-approval+ for landing on the other branches?

Comment 28

[Brian R. Bondy [:bbondy]]

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]]

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]]

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]]

(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]]

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 - ex-MoCo]]

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 - ex-MoCo]]

(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]]

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]]

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]]

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]]

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]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/94b026146a99

Comment 40

[Kamil Jozwiak [:kjozwiak]]

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]]

Changed: status-firefox21 & status-firefox22 to verified

Comment 42

[Lukas Blakk [:lsblakk] use ?needinfo]

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 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0bffdd3704ee
https://hg.mozilla.org/releases/mozilla-esr17/rev/6fbcdda7a0f5

Comment 44

[Kamil Jozwiak [:kjozwiak]]

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

[u279076]

Thanks Kamil, beta builds should now be available.

Comment 46

[Kamil Jozwiak [:kjozwiak]]

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)