1177861 - (CVE-2015-4505) Arbitrary file manipulation through updater.exe (Privilege Escalation)

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Holger Fuhrmannek]

Attachment: 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).

Comment 1

[Robert Strong (they/them - no direct email)]

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.

Comment 2

[Robert Strong (they/them - no direct email)]

Kamil Dan, and Al, could any or all of you please try to reproduce this bug? Thanks!

Comment 3

[Robert Strong (they/them - no direct email)]

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

Comment 4

[Robert Strong (they/them - no direct email)]

Thanks for the bug report and I was able to reproduce

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

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.)

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Comment 7

[Kamil Jozwiak [:kjozwiak]]

CC'd Brian and needinfo'd as per comment # 5

Comment 8

[Brian R. Bondy [:bbondy]]

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.

Comment 9

[Robert Strong (they/them - no direct email)]

Note that there are problems with calling LaunchWinPostProcess that I am looking into. I'm mainly looking for feedback on the mitigations.

Comment 10

[Brian R. Bondy [:bbondy]]

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.

Comment 11

[Brian R. Bondy [:bbondy]]

Could I have access to toolkit / app update security bugs? I often get pulled in for feedback. Thanks.

Comment 12

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

Dan can give access.

Comment 13

[Daniel Veditz [:dveditz]]

Access granted (but next time mail security@ rather than clutter up bugs)

Comment 14

[Robert Strong (they/them - no direct email)]

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

Comment 15

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

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.

Comment 16

[Robert Strong (they/them - no direct email)]

Try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d587901449fb

Comment 17

[Stephen A Pohl [:spohl] (OOO until 2/27)]

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?

Comment 18

[Robert Strong (they/them - no direct email)]

(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

Comment 19

[Robert Strong (they/them - no direct email)]

Attachment: patch - updated to comments

[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.

Comment 20

[Robert Strong (they/them - no direct email)]

Al, is this being held back intentionally?

Comment 21

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

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.

Comment 22

[Robert Strong (they/them - no direct email)]

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.

Comment 23

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

All right.

Comment 24

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/66bf3e9e71d4

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66bf3e9e71d4

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Robert, given that tomorrow is 8/25 (comment 21), do you want to request patch uplift to Beta41 and Aurora42?

Comment 27

[Robert Strong (they/them - no direct email)]

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

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 29

[Robert Strong (they/them - no direct email)]

The patches didn't apply cleanly on beta and esr38 so I'll land this myself later today

Comment 30

[Kamil Jozwiak [:kjozwiak]]

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?

Comment 31

[Robert Strong (they/them - no direct email)]

will do after I update the patches

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b1cb1d513a85

Comment 33

[Robert Strong (they/them - no direct email)]

Attachment: patch for beta - landed

Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/447736aa246b

Comment 34

[Robert Strong (they/them - no direct email)]

Attachment: patch for esr38 - landed

https://hg.mozilla.org/releases/mozilla-esr38/rev/be6396f0eb7c

Comment 35

[Robert Strong (they/them - no direct email)]

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

Comment 36

[Robert Strong (they/them - no direct email)]

Kamil, I took a look at poc.rb and found that it is specifically designed to only work with Firefox 38.0.5.

Comment 37

[Kamil Jozwiak [:kjozwiak]]

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

Comment 38

[Robert Strong (they/them - no direct email)]

I think it would be more trouble than it is worth but if you have the time please go for it.

Comment 39

[Robert Strong (they/them - no direct email)]

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

Comment 40

[Kamil Jozwiak [:kjozwiak]]

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