Bug 1177861 (CVE-2015-4505)

Arbitrary file manipulation through updater.exe (Privilege Escalation)

VERIFIED FIXED in Firefox 41

Status

()

defect
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: hofusec, Assigned: rstrong)

Tracking

({csectype-priv-escalation, sec-high})

unspecified
mozilla43
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox40 wontfix, firefox41+ verified, firefox42+ verified, firefox43 verified, firefox-esr31 wontfix, firefox-esr3841+ verified, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.2r unaffected, b2g-master unaffected)

Details

(Whiteboard: [adv-main41+][adv-esr38.3+])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Posted file poc.zip
The updater.exe which is shipped with Firefox 38.0.5 opens the possibility for a local user to manipulate arbitrary files. That can be exploited for example with the maintenance service to run arbitrary executables files with the same privileges as the service.

The vulnerability exists in the process of executing a staged update. During this process the updated files are written to an user controlled working directory. Also temporary update files like *.patch files are written to this directory. As in Bug #925747 these files can be manipulated to change how files are updated, if the attacker sets the working directory path to a writeable location. Manipulations are limited to files in the working directory but with a junction it is possible to break out of the working directory and change arbitrary files. Different to hard links a standard windows user has the rights to create junctions (win7).

The poc needs firefox 38.0.5 (http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/38.0.5/win32/en-US/Firefox%20Setup%2038.0.5.exe) installed to "C:\Program Files (x86)\Mozilla Firefox", the maintenance service and ruby. I have tested the poc with win7 pro. The poc tries to execute a staged update with a junction-prepared working directory and manipulated .manifest and .patch files. For the manipulation the right timing is needed so the poc tries again if it fails. On my computer I need 3-20 tries. After the poc succeeded the content of the helper.exe in the firefox directory is replaced with the content of the standard cmd.exe of windows. And this replaced helper.exe is running in the background with system rights (after an update with the maintenance service the service executes the helper.exe if an updater.ini is present).
I've been trying to get the poc to work with well over 100 attempts without success. What I do see in the maintenance service logs are error 19's which is the error for an incorrectly signed mar file.

Unless I am mistaken, the patch files are written to <install-dir>\updating\ and not a user controlled directory.
Kamil Dan, and Al, could any or all of you please try to reproduce this bug? Thanks!
Flags: needinfo?(kjozwiak)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #1)
> I've been trying to get the poc to work with well over 100 attempts without
> success. What I do see in the maintenance service logs are error 19's which
> is the error for an incorrectly signed mar file.
Note: I had some test code that might have been interfering with this on my system so I am going to try again.
Thanks for the bug report and I was able to reproduce
Flags: needinfo?(kjozwiak)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Assignee: nobody → robert.strong.bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee: robert.strong.bugs → nobody
Status: ASSIGNED → NEW
Posted patch patch in progress (obsolete) — Splinter Review
Hey Brian, could I get your feedback on this approach. I'm also considering adding:
* is the post update binary cert the same as the updater cert check before launching the post update binary for all cases.
* only use the service when the install is under program files. Though not directly applicable to this bug this would limit the cases where the service could possibly be exploited while still being used by the vast majority of users.

I'll also be adding tests where possible (e.g. staging dir invalid location, multiple post update process for service tests, etc.)
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
Attachment #8629705 - Flags: feedback?
Posted patch patch in progress (obsolete) — Splinter Review
Attachment #8629705 - Attachment is obsolete: true
Attachment #8629705 - Flags: feedback?
Attachment #8629765 - Flags: feedback?
CC'd Brian and needinfo'd as per comment # 5
Flags: needinfo?(netzen)
Comment 6 is private: false
Thanks for the n-i. I don't have access to these security bugs anymore without it. I'll get back to you soon Robert.
Note that there are problems with calling LaunchWinPostProcess that I am looking into. I'm mainly looking for feedback on the mitigations.
Comment on attachment 8629765 [details] [diff] [review]
patch in progress

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

> is the post update binary cert the same as the updater cert check before
> launching the post update binary for all cases.

Yes please. IIRC the binary should be locked for writing before the check is done and until after it is executed.  This avoids timing attacks.


This may even a good idea from within browser code before setting the default browser below win8, but that's probably more work.
That extra browser part would probably be in a diff bug if you think it's a good idea.


> only use the service when the install is under program files. Though not directly applicable to this bug this would limit
> the cases where the service could possibly be exploited while still being used by the vast majority of users.

So we have this check already in updater code before it decides to run the service, but I think it's a good idea to do this within the maintenance service code as well.

::: toolkit/mozapps/update/common/registrycertificates.cpp
@@ +58,5 @@
>      } else if (allowFallbackKeySkip) {
>        LOG_WARN(("Fallback key present, skipping VerifyCertificateTrustForFile "
>                  "check and the certificate attribute registry matching "
>                  "check."));
> +      RegCloseKey(baseKey);

I think there's an nsAutoRegKey which would avoid having to do all of these. I don't remember if it can be used within this file or not though.
Attachment #8629765 - Flags: feedback? → feedback+
Flags: needinfo?(netzen)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
(In reply to Brian R. Bondy [:bbondy] from comment #10)
> Comment on attachment 8629765 [details] [diff] [review]
> patch in progress
> 
> Review of attachment 8629765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > is the post update binary cert the same as the updater cert check before
> > launching the post update binary for all cases.
> 
> Yes please. IIRC the binary should be locked for writing before the check is
> done and until after it is executed.  This avoids timing attacks.
Yep

> 
> This may even a good idea from within browser code before setting the
> default browser below win8, but that's probably more work.
> That extra browser part would probably be in a diff bug if you think it's a
> good idea.
Will file a bug

> 
> 
> > only use the service when the install is under program files. Though not directly applicable to this bug this would limit
> > the cases where the service could possibly be exploited while still being used by the vast majority of users.
> 
> So we have this check already in updater code before it decides to run the
> service, but I think it's a good idea to do this within the maintenance
> service code as well.
I don't believe we do and I just checked without finding any references where it does. The only thing I can think of like this is that we used to have a check for progfiles before requesting elevation that I added around 2.0.0.2.

> ::: toolkit/mozapps/update/common/registrycertificates.cpp
> @@ +58,5 @@
> >      } else if (allowFallbackKeySkip) {
> >        LOG_WARN(("Fallback key present, skipping VerifyCertificateTrustForFile "
> >                  "check and the certificate attribute registry matching "
> >                  "check."));
> > +      RegCloseKey(baseKey);
> 
> I think there's an nsAutoRegKey which would avoid having to do all of these.
> I don't remember if it can be used within this file or not though.
It can't after the move.
Posted patch patch rev1 (obsolete) — Splinter Review
There are several complications with the additional items I'd like to do so I'm going to go with this and file followup bugs for the additional items.
Attachment #8629765 - Attachment is obsolete: true
Attachment #8641532 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8641532 [details] [diff] [review]
patch rev1

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

::: toolkit/mozapps/update/updater/updater.cpp
@@ +1934,5 @@
> +  free(cmdline);
> +  if (ok) {
> +    if (!async) {
> +      // If the process takes over 30 seconds continue
> +      WaitForSingleObject(pi.hProcess, 30000);

The change to 30 seconds (from infinite previously) doesn't seem related to this bug. Should we split this out to make it easier to handle any possible fallout from it?
Attachment #8641532 - Flags: review?(spohl.mozilla.bugs) → review+
(In reply to Stephen A Pohl [:spohl] from comment #17)
> Comment on attachment 8641532 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 8641532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/updater/updater.cpp
> @@ +1934,5 @@
> > +  free(cmdline);
> > +  if (ok) {
> > +    if (!async) {
> > +      // If the process takes over 30 seconds continue
> > +      WaitForSingleObject(pi.hProcess, 30000);
> 
> The change to 30 seconds (from infinite previously) doesn't seem related to
> this bug. Should we split this out to make it easier to handle any possible
> fallout from it?
I'm ok with that
[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not too 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

Which older supported branches are affected by this flaw? All with the maintenance service.

If not all supported branches, which bug introduced the flaw? N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No but should be easy to create.

How likely is this patch to cause regressions; how much testing does it need? I've tested this on oak over several days. I'd like it on m-c a couple of days before uplifting.
Attachment #8641532 - Attachment is obsolete: true
Attachment #8641872 - Flags: sec-approval?
Attachment #8641872 - Flags: review+
Al, is this being held back intentionally?
Flags: needinfo?(abillings)
I didn't approve it over the weekend but since we're shipping in a week, this can't go in until 8/25 unless there is a specific reason to take it sooner.
Flags: needinfo?(abillings)
Whiteboard: [checkin on 8/25]
Attachment #8641872 - Flags: sec-approval? → sec-approval+
I'd like to let it bake on m-c until it can land on other branches. I'll land it on m-c later tonight.
All right.
Whiteboard: [checkin on 8/25]
https://hg.mozilla.org/mozilla-central/rev/66bf3e9e71d4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Robert, given that tomorrow is 8/25 (comment 21), do you want to request patch uplift to Beta41 and Aurora42?
Flags: needinfo?(robert.strong.bugs)
Comment on attachment 8641872 [details] [diff] [review]
patch - updated to comments

Approval Request Comment
[Feature/regressing bug #]: Update Staging
[User impact if declined]: User systems could be locally exploited
[Describe test coverage new/current, TreeHerder]: Current tests verify that the patch doesn't break functionality. 
[Risks and why]: Minimal since the patch has baked without regressions and the current fallbacks should allow users to update if there is a failure.
[String/UUID change made/needed]: None
Flags: needinfo?(robert.strong.bugs)
Attachment #8641872 - Flags: approval-mozilla-esr38?
Attachment #8641872 - Flags: approval-mozilla-beta?
Attachment #8641872 - Flags: approval-mozilla-aurora?
Comment on attachment 8641872 [details] [diff] [review]
patch - updated to comments

Approving for uplift to all nom'd branches. This is a sec-high bug that we ought to fix. Patch has stabilized in Nightly for 2 weeks so should be safe to uplift to Beta41, Aurora42 and ESR38.3.0.
Attachment #8641872 - Flags: approval-mozilla-esr38?
Attachment #8641872 - Flags: approval-mozilla-esr38+
Attachment #8641872 - Flags: approval-mozilla-beta?
Attachment #8641872 - Flags: approval-mozilla-beta+
Attachment #8641872 - Flags: approval-mozilla-aurora?
Attachment #8641872 - Flags: approval-mozilla-aurora+
The patches didn't apply cleanly on beta and esr38 so I'll land this myself later today
Relating to testing, I can reproduce the original issue using fx38.0.5 and the poc from comment #0 but I couldn't reproduce it with an older version of nightly before the fix. I took a look at poc.rb and changed all the appropriate paths to point to Nightly but I still couldn't reproduce. This makes it hard to verify as I never could reproduce on nightly.

Could the problem be the .mar file? I'm assuming that it's specifically targeting fx38.0.5 and won't work on nightly/aurora. I know your plate is pretty full Robert, but could you taking a quick look?
will do after I update the patches
Flags: in-testsuite+ → in-testsuite?
(In reply to Kamil Jozwiak [:kjozwiak] from comment #30)
> Relating to testing, I can reproduce the original issue using fx38.0.5 and
> the poc from comment #0 but I couldn't reproduce it with an older version of
> nightly before the fix. I took a look at poc.rb and changed all the
> appropriate paths to point to Nightly but I still couldn't reproduce. This
> makes it hard to verify as I never could reproduce on nightly.
> 
> Could the problem be the .mar file? I'm assuming that it's specifically
> targeting fx38.0.5 and won't work on nightly/aurora. I know your plate is
> pretty full Robert, but could you taking a quick look?
The esr38 patch took up my evening. I'll try to look again tomorrow.
Kamil, I took a look at poc.rb and found that it is specifically designed to only work with Firefox 38.0.5.
Depends on: 1199118
> Kamil, I took a look at poc.rb and found that it is specifically designed to
> only work with Firefox 38.0.5.

Thanks for checking Robert :) Would it be worth converting poc.rb to work with nighty/other channels? I could take a stab if the change(s) aren't difficult.
I think it would be more trouble than it is worth but if you have the time please go for it.
One thing you can check is whether it is possible on Windows to replace an install with a staged update that is not under the install directory.

For example
"C:\moz\poc\firefoxWork\updater.exe" "C:\moz\poc\updateWork" "C:\Program Files (x86)\Mozilla Firefox" "C:\moz\poc\firefoxWork" -1/replace

The update.log should contain
Stage and Replace requests require that the working directory is a sub-directory of the installation directory! Exiting

The update.status should contain
failed: 72

Updated

4 years ago
Group: core-security → core-security-release
fx43 results:
=============

- build: https://archive.mozilla.org/pub/firefox/nightly/2015-09-08-03-02-03-mozilla-central/
- mar: https://archive.mozilla.org/pub/firefox/nightly/2015-09-11-03-02-04-mozilla-central/firefox-43.0a1.en-US.win32.complete.mar

cmd: "C:\poc\updateFX\updater.exe" "C:\poc\updateFX" "C:\Program Files (x86)\Nightly" "C:\poc\updateFX" -1/replace

* update.log: -> Stage and Replace requests require that the working directory is a sub-directory of the installation directory! Exiting
* update.status: -> failed: 72

fx42 results:
=============

- build: https://archive.mozilla.org/pub/firefox/nightly/2015-09-08-00-40-20-mozilla-aurora/
- mar: https://archive.mozilla.org/pub/firefox/nightly/2015-09-11-00-41-12-mozilla-aurora/firefox-42.0a2.en-US.win32.complete.mar

cmd: "C:\poc\updateFX\updater.exe" "C:\poc\updateFX" "C:\Program Files (x86)\Firefox Developer Edition" "C:\poc\updateFX" -1/replace

* update.log: -> Stage and Replace requests require that the working directory is a sub-directory of the installation directory! Exiting
* update.status: -> failed: 72

fx41 results:
=============

- build: https://archive.mozilla.org/pub/firefox/candidates/41.0b8-candidates/build1/win32/en-US/
- mar: https://archive.mozilla.org/pub/firefox/candidates/41.0b9-candidates/build1/update/win32/en-US/firefox-41.0b9.complete.mar

cmd: "C:\poc\updateFX\updater.exe" "C:\poc\updateFX" "C:\Program Files (x86)\Mozilla Firefox" "C:\poc\updateFX" -1/replace

* update.log: -> Stage and Replace requests require that the working directory is a sub-directory of the installation directory! Exiting
* update.status: -> failed: 72

fxesr38 results:
================

- build: https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-esr38-win32/1441449693/
- mar: https://archive.mozilla.org/pub/firefox/nightly/38.2.1esr-candidates/build2/update/win32/en-US/firefox-38.2.1esr.complete.mar

cmd: "C:\poc\updateFX\updater.exe" "C:\poc\updateFX" "C:\Program Files (x86)\Nightly" "C:\poc\updateFX" -1/replace

* update.log: -> Stage and Replace requests require that the working directory is a sub-directory of the installation directory! Exiting
* update.status: -> failed: 72

OS's Used:
==========

- Win 10 x64 (VM) -> PASSED
- Win 7 x64 (VM) -> PASSED
Whiteboard: [adv-main41+][adv-esr38.3+]
Alias: CVE-2015-4505
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.