Closed
Bug 1177861
(CVE-2015-4505)
Opened 10 years ago
Closed 10 years ago
Arbitrary file manipulation through updater.exe (Privilege Escalation)
Categories
(Toolkit :: Application Update, defect)
Toolkit
Application Update
Tracking
()
VERIFIED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox40 | --- | wontfix |
firefox41 | + | verified |
firefox42 | + | verified |
firefox43 | --- | verified |
firefox-esr31 | --- | wontfix |
firefox-esr38 | 41+ | 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 |
People
(Reporter: hofusec, Assigned: robert.strong.bugs)
References
Details
(Keywords: csectype-priv-escalation, reporter-external, sec-high, Whiteboard: [adv-main41+][adv-esr38.3+])
Attachments
(4 files, 3 obsolete files)
6.28 MB,
application/zip
|
Details | |
38.21 KB,
patch
|
robert.strong.bugs
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
ritu
:
approval-mozilla-esr38+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
38.23 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
56.61 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
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).
![]() |
Assignee | |
Comment 1•10 years ago
|
||
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.
![]() |
Assignee | |
Comment 2•10 years ago
|
||
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)
![]() |
Assignee | |
Comment 3•10 years ago
|
||
(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.
![]() |
Assignee | |
Comment 4•10 years ago
|
||
Thanks for the bug report and I was able to reproduce
Flags: needinfo?(kjozwiak)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
![]() |
Assignee | |
Updated•10 years ago
|
Assignee: nobody → robert.strong.bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
![]() |
Assignee | |
Updated•10 years ago
|
Assignee: robert.strong.bugs → nobody
Status: ASSIGNED → NEW
Updated•10 years ago
|
Keywords: csectype-priv-escalation,
sec-high
![]() |
Assignee | |
Comment 5•10 years ago
|
||
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 | |
Comment 6•10 years ago
|
||
Attachment #8629705 -
Attachment is obsolete: true
Attachment #8629705 -
Flags: feedback?
Attachment #8629765 -
Flags: feedback?
Comment 8•10 years ago
|
||
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.
![]() |
Assignee | |
Comment 9•10 years ago
|
||
Note that there are problems with calling LaunchWinPostProcess that I am looking into. I'm mainly looking for feedback on the mitigations.
Comment 10•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: needinfo?(netzen)
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Comment hidden (off-topic) |
![]() |
Assignee | |
Comment 14•10 years ago
|
||
(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.
![]() |
Assignee | |
Comment 15•10 years ago
|
||
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)
![]() |
Assignee | |
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
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+
![]() |
Assignee | |
Comment 18•10 years ago
|
||
(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
![]() |
Assignee | |
Comment 19•10 years ago
|
||
[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+
![]() |
Assignee | |
Comment 20•10 years ago
|
||
Al, is this being held back intentionally?
Flags: needinfo?(abillings)
Comment 21•10 years ago
|
||
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.
status-firefox40:
--- → wontfix
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox-esr31:
--- → wontfix
status-firefox-esr38:
--- → affected
tracking-firefox41:
--- → +
tracking-firefox42:
--- → +
tracking-firefox-esr38:
--- → ?
Flags: needinfo?(abillings)
Whiteboard: [checkin on 8/25]
Updated•10 years ago
|
Attachment #8641872 -
Flags: sec-approval? → sec-approval+
![]() |
Assignee | |
Comment 22•10 years ago
|
||
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.
![]() |
Assignee | |
Comment 24•10 years ago
|
||
Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/66bf3e9e71d4
Comment 25•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox43:
--- → fixed
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)
![]() |
Assignee | |
Comment 27•9 years ago
|
||
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+
![]() |
Assignee | |
Comment 29•9 years ago
|
||
The patches didn't apply cleanly on beta and esr38 so I'll land this myself later today
Comment 30•9 years ago
|
||
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?
![]() |
Assignee | |
Comment 31•9 years ago
|
||
will do after I update the patches
Comment 32•9 years ago
|
||
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-v2.2r:
--- → unaffected
status-b2g-master:
--- → unaffected
Flags: in-testsuite?
![]() |
Assignee | |
Comment 33•9 years ago
|
||
Pushed to mozilla-beta
https://hg.mozilla.org/releases/mozilla-beta/rev/447736aa246b
Attachment #8652657 -
Flags: review+
![]() |
Assignee | |
Updated•9 years ago
|
![]() |
Assignee | |
Comment 34•9 years ago
|
||
Attachment #8652698 -
Flags: review+
![]() |
Assignee | |
Updated•9 years ago
|
Flags: in-testsuite? → in-testsuite+
![]() |
Assignee | |
Updated•9 years ago
|
Flags: in-testsuite+ → in-testsuite?
![]() |
Assignee | |
Comment 35•9 years ago
|
||
(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.
Updated•9 years ago
|
![]() |
Assignee | |
Comment 36•9 years ago
|
||
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•9 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 38•9 years ago
|
||
I think it would be more trouble than it is worth but if you have the time please go for it.
![]() |
Assignee | |
Comment 39•9 years ago
|
||
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•9 years ago
|
Group: core-security → core-security-release
Comment 40•9 years ago
|
||
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
Status: RESOLVED → VERIFIED
Updated•9 years ago
|
Whiteboard: [adv-main41+][adv-esr38.3+]
Updated•9 years ago
|
Alias: CVE-2015-4505
Updated•9 years ago
|
Flags: sec-bounty?
Updated•9 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•9 years ago
|
Group: core-security-release
![]() |
Assignee | |
Updated•6 years ago
|
Flags: in-testsuite?
Updated•9 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•