Closed Bug 1171518 (CVE-2015-4481) Opened 9 years ago Closed 9 years ago

[Win] Privileged update processes writing to user writable locations can overwrite non-user writable locations using hard links

Categories

(Toolkit :: Application Update, defect)

All
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox38 --- wontfix
firefox39 + wontfix
firefox38.0.5 --- wontfix
firefox40 + fixed
firefox41 + fixed
firefox42 + fixed
firefox-esr31 --- wontfix
firefox-esr38 40+ fixed
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: forshaw, Assigned: robert.strong.bugs)

References

Details

(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [post-critsmash-triage][adv-main40+][adv-esr38.2+] Will be published by Sept 1)

Attachments

(3 files, 3 obsolete files)

Attached file Proof of Concept
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.81 Safari/537.36

Steps to reproduce:

Build ID: 20150525141253
OS: Windows

Summary:
The maintenance service creates a log file in a user writable location. It’s possible to change the log file to a hardlink to another file to cause file corruption or elevation of privilege. 

Description:
When the maintenance service starts it creates a log file under c:\programdata\mozilla\logs. This is done in maintenanceservice.cpp/SvcMain. This directory it creates the file in has fairly permissive permissions which allows a normal user to create new files underneath that directory. It’s possible to race the creation of the log file during the service initialization to drop a hardlink to an existing file on the same drive (which is probably the system drive) which when opened by the maintenance service running as local system will cause the file to be overwritten by the log data.

At the very least this would corrupt the target file, however as the user has some control over bits of the contents, such as the updater path, it’s possible to get some user controlled contents in there. This might be used to elevate privileges by overwriting a script file which has a permissive parser, such as powershell, batch or HTA which subsequently gets executed by a privileged process. 

The only slight difficulty in exploitation is that the user cannot directly delete the log file to replace it with a hardlink. However this isn’t a significant issue as before opening the log file the service backs up the log to a new name leaving the directory entry for “maintenanceservice.log” free. Therefore there’s a race condition between the log file being moved out of the way and the new log file being created. 

So to exploit this you perform the following operations:

1. Start a thread which creates a hard link in the log directory to the file you want to overwrite. Repeat until successful.
2. In another thread start the service passing the arbitrary content you want to insert as the path to the updater file

A similar vulnerability exists in the update.status handling, for example in WriteStatusFailure which will write update.status to any location you specify. You can use a hardlink to force an arbitrary file to be overwritten. In this case this would only cause file corruption as the user has no real control on the contents. 

If I could recommend fixes either make the logs directory writable only by administrators or use CopyFile instead of MoveFile when backing up the previous logs. I would not recommend trying to do anything like inspecting the file for hardlinks or similar. 

Proof of Concept:

I’ve attached a proof of concept, it’s written in C#. You’ll need to compile it with the C# csc compiler. NOTE: you might need to run this on a multi-core machine to stand a chance of winning the race. 

1) Compile the PoC with csc.exe Program.cs resulting in Program.exe
2) Execute the PoC passing the name of a file you want to overwrite on the command line. You only need to be able to read the file. 
3) Program should run and print Done if successful

This bug is subject to a 90 day disclosure deadline. If 90 days elapse
without a broadly available patch, then the bug report will automatically
become visible to the public.


Actual results:

The target file has been overwritten with the contents of the log file


Expected results:

The log file is created as normal
Flags: needinfo?(robert.strong.bugs)
Already looking at it
Flags: needinfo?(robert.strong.bugs)
Assignee: nobody → robert.strong.bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
James, thank you for the clear and concise bug report. It is very much appreciated.
(In reply to James Forshaw from comment #0)
>...
> If I could recommend fixes either make the logs directory writable only by
> administrators or use CopyFile instead of MoveFile when backing up the
> previous logs. I would not recommend trying to do anything like inspecting
> the file for hardlinks or similar. 
Hi James, why would you recommend using CopyFile? From what I have seen it suffers from the same problem.
Flags: needinfo?(forshaw)
I retract my statement about CopyFile, it would be fine if all the backup slots were filled (the rationale being as you never delete the first log the attacker couldn't replace it) but of course you're in a worse position when there are any spare log files as you could get an arbitrary file copied from log-N to log-N+1 where N+1 is a hard link. 

Changing security of the directory is probably the better way of fixing the issue, of course if there are any other users of that directory other than the system service things get more tricky. Perhaps create a sub directory just for the maintenance service in that case.
Flags: needinfo?(forshaw)
From what I have seen, CopyFile just replaces symbolic links but hard links stay in affect.

For the service log files I am planning on using a secure location for the logs. For both the service log files and the update.status files I am plannin on creating a temp file for the log in the log directory, after closing it using DeleteFile to remove the destination if it exist since it removes the file as well as the hard link, and MoveFile to move the temp log file into place.
Well MoveFileEx with MOVEFILE_REPLACE_EXISTING flag should be all you need, you shouldn't need DeleteFile, it will replace the hard link all by itself. That's because the MoveFile API replaces the directory entry, rather than copying the data over the top of the file. CopyFile should actually follow symbolic links, but as you need to be an administrator anyway to create them I don't consider them to be a security risk.

Worth also noting that if there was a way to delete all files in the logs directory you have a more serious problem. Fortunately you'd need another bug to do that (as in an arbitrary delete file at SYSTEM). Currently with the permissions of the logs directory you'd be able to convert it to a directory junction which can be abused in a similar way and creating the temporary file won't necessarily help. But the directory would need to be empty first, which mitigates the issue.
Attached patch patch rev1 (obsolete) — Splinter Review
Brian, do you have the time to provide feedback or review on this patch? BTW: I noticed that PARENT_WAIT periodically timed out when running tests on debug builds and I expect that for at least some users it is too short so I increased it.
Attachment #8616349 - Flags: feedback?
Also, there are some other things I'd like to do to clean this up but I don't want to do them in this patch since it will need to be uplifted.
Attachment #8616349 - Flags: feedback? → feedback?(netzen)
I'll deal with the removal of the old logs dir in a separate patch.
Comment on attachment 8616349 [details] [diff] [review]
patch rev1

This was passing tests locally but Try run had failures... investigating
Attachment #8616349 - Flags: feedback?(netzen)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #11)
> Comment on attachment 8616349 [details] [diff] [review]
> patch rev1
> 
> This was passing tests locally but Try run had failures... investigating
I accidentally pushed an earlier version of the patch to try... new try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ffec25b823d
Comment on attachment 8616349 [details] [diff] [review]
patch rev1

All is looking good with the try run using the correct patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ffec25b823d

Brian, do you have the time to provide feedback or review on this patch? BTW: I noticed that PARENT_WAIT periodically timed out when running tests on debug builds and I expect that for at least some users it is too short so I increased it.

Also, there are some other things I'd like to do to clean this up but I don't want to do them in this patch since it will need to be uplifted.

I'll deal with the removal of the old logs dir in a separate patch.
Attachment #8616349 - Flags: feedback?(netzen)
Comment on attachment 8616349 [details] [diff] [review]
patch rev1

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

Yes I have time, np reviewing.

Doing it in the maint service dir sounds right.  I think it's ok but pls verify if you didn't already that the uninstaller that it still uninstalls without a problem as well.

Patch looks good but it seems like the status file is maybe the more generic case for this right? It would apply to both normal updates and service updates.  Thx for also tackling that. Might be better to make title of this bug more generic.

Would calls like DeleteFile and MoveFile have similar problems if another malicious program is racing to delete our already created file and replacing?
Attachment #8616349 - Flags: feedback?(netzen) → feedback+
Summary: Mozilla Maintenance Service: Log File Overwrite Elevation of Privilege → Privileged update processes writing to user writable locations can overwrite non-user writable locations using hard links
(In reply to Brian R. Bondy [:bbondy] from comment #14)
> Comment on attachment 8616349 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 8616349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes I have time, np reviewing.
> 
> Doing it in the maint service dir sounds right.  I think it's ok but pls
> verify if you didn't already that the uninstaller that it still uninstalls
> without a problem as well.
> 
> Patch looks good but it seems like the status file is maybe the more generic
> case for this right? It would apply to both normal updates and service
> updates.  Thx for also tackling that. Might be better to make title of this
> bug more generic.
> 
> Would calls like DeleteFile and MoveFile have similar problems if another
> malicious program is racing to delete our already created file and replacing?
Thanks! I have been considering ways to make this more bullet proof but wanted to run the approach by you first before spending the time to do so. I'll upload another patch after I've finished it.
Whiteboard: Will be published by Sept 1
[Tracking Requested - why for this release]:
We should be able to have a fix for this for Firefox 40 though if pushed we might be able to get this in for Firefox 39. This should also be uplifted to esr38.
OS: Unspecified → Windows 8.1
Hardware: Unspecified → All
Summary: Privileged update processes writing to user writable locations can overwrite non-user writable locations using hard links → [Win] Privileged update processes writing to user writable locations can overwrite non-user writable locations using hard links
Attached patch updater patch rev1 (obsolete) — Splinter Review
I'm going to go with a separate patch for the installer.

Try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f3b35592bb5
Attachment #8616349 - Attachment is obsolete: true
Attachment #8622030 - Flags: review?(netzen)
Comment on attachment 8622030 [details] [diff] [review]
updater patch rev1

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

::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js
@@ +1285,5 @@
> +
> +  const CSIDL_PROGRAM_FILES = 0x26;
> +  const CSIDL_PROGRAM_FILESX86 = 0x2A;
> +  // This will return an empty string on our Win XP build systems.
> +  let maintSvcDir = getSpecialFolderDir(CSIDL_PROGRAM_FILESX86);

I wonder if that's the case still with our min supported service pack, but it's fine to leave as it was.
Attachment #8622030 - Flags: review?(netzen) → review+
We're going to build for 39 RC tomorrow morning and no one requested uplift, so unless you think this should drive an RC2, I'd like to wontfix this for 39.   Can you request uplift to 40? And if anyone strongly feels it should go into 39 at the last minute, please let me know.
Flags: needinfo?(robert.strong.bugs)
I'd prefer this goes in for 40 per comment #16.
Flags: needinfo?(robert.strong.bugs)
Rob, I see an R+ patch here from three weeks ago. Can we get this in?
Flags: needinfo?(robert.strong.bugs)
I need to finish up an uninstall patch and I need to consider a couple of other things before landing. Definitely in time for 40 though.
Flags: needinfo?(robert.strong.bugs)
Attached patch installer patch (obsolete) — Splinter Review
Attachment #8631900 - Flags: review?(netzen)
Comment on attachment 8631900 [details] [diff] [review]
installer patch

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

::: browser/installer/windows/nsis/maintenanceservice_installer.nsi
@@ +308,5 @@
> +  Push "$APPDATA\Mozilla\logs\maintenanceservice-8.log"
> +  Call un.RenameDelete
> +  Push "$APPDATA\Mozilla\logs\maintenanceservice-9.log"
> +  Call un.RenameDelete
> +  Push "$APPDATA\Mozilla\logs\maintenanceservice-10.log"

This WFM but couldn't it be cleaner with a LogicLib loop in case this changes?
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/maintenanceservice/maintenanceservice.cpp#30

Fine with it as is since you have more important things to do than refactor this.
Attachment #8631900 - Flags: review?(netzen) → review+
[Security approval request comment]
How easily could an exploit be constructed based on the patch? Hard to estimate but I don't think it would be too easily accomplished.

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

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be trivial but I want this to bake on trunk for a few days.

How likely is this patch to cause regressions; how much testing does it need? Not likely. There are also tests that check the log files.
Attachment #8622030 - Attachment is obsolete: true
Attachment #8631900 - Attachment is obsolete: true
Attachment #8632935 - Flags: sec-approval?
Attachment #8632935 - Flags: review+
Comment on attachment 8632935 [details] [diff] [review]
combined patch (nightly, aurora, and beta)

sec-approval+ for trunk. Please make patches for Aurora, Beta, and ESR38 as well and nominate them so we can fix is everywhere.
Attachment #8632935 - Flags: sec-approval? → sec-approval+
Comment on attachment 8632935 [details] [diff] [review]
combined patch (nightly, aurora, and beta)

This patch applies cleanly to aurora and beta. I'll attach a new patch for esr38 and request uplift after the patch has baked a couple of days on m-c.
Attachment #8632935 - Attachment description: combined patch → combined patch (nightly, aurora, and beta)
Comment on attachment 8633642 [details] [diff] [review]
combined patch for esr38

I ran all of the update tests locally for the esr38 patch and they all passed.
https://hg.mozilla.org/mozilla-central/rev/98c0d3e9c6f1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Robert, could you fill the uplift request for aurora, beta and esr38? Thanks
Flags: needinfo?(robert.strong.bugs)
(In reply to Sylvestre Ledru [:sylvestre] PTO => July 10th from comment #32)
> Robert, could you fill the uplift request for aurora, beta and esr38? Thanks
I was planning to after it had baked for a couple of days on nightly per comment #28 and other comments. This way it doesn't accidentally get uplifted before it has baked.
Flags: needinfo?(robert.strong.bugs)
Comment on attachment 8632935 [details] [diff] [review]
combined patch (nightly, aurora, and beta)

Approval Request Comment
[Feature/regressing bug #]: Maintenance service initial implementation
[User impact if declined]: Possible to overwrite files that the user doesn't have write access to
[Describe test coverage new/current, TreeHerder]: The pre-existing tests verify the files that are written are correct and it has baked a few days.
[Risks and why]: Minimal since the tests verify the files contain the expected values.
[String/UUID change made/needed]: None
Attachment #8632935 - Flags: approval-mozilla-beta?
Attachment #8632935 - Flags: approval-mozilla-aurora?
Comment on attachment 8633642 [details] [diff] [review]
combined patch for esr38

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible to overwrite files that the user doesn't have write access to
Fix Landed on Version: Nightly and will be uplifted to aurora and beta
Risk to taking this patch (and alternatives if risky): Minimal since the tests verify the files contain the expected values.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8633642 - Flags: approval-mozilla-esr38?
Comment on attachment 8632935 [details] [diff] [review]
combined patch (nightly, aurora, and beta)

Now that this has baked for a few days and rstrong thinks it's OK to uplift, let's get this on all branches. Beta+ Aurora+ ESR38+
Attachment #8632935 - Flags: approval-mozilla-beta?
Attachment #8632935 - Flags: approval-mozilla-beta+
Attachment #8632935 - Flags: approval-mozilla-aurora?
Attachment #8632935 - Flags: approval-mozilla-aurora+
Attachment #8633642 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
I see some pre-existing tests are mentioned for this fix. Does this need any manual verification? If yes, could you please provide some simple steps to verify this?
Flags: needinfo?(robert.strong.bugs)
I don't see value in manual verification beyond keeping an eye on updates using the service still working which has already been shown to be the case in nightly.
Flags: needinfo?(robert.strong.bugs)
Hi James, have you had a chance to examine this fix? Let us know if it still reproduces.
Flags: needinfo?(forshaw)
Hi, I've not actually tested but looking at the patch it seems to be me that it should mitigate the issue. Moving the log files outside of a user writeable location is ultimately the most important but the changes to update.status also look correct as far as I can tell.
Flags: needinfo?(forshaw)
Whiteboard: Will be published by Sept 1 → [adv-main40+][adv-esr38.2+] Will be published by Sept 1
Whiteboard: [adv-main40+][adv-esr38.2+] Will be published by Sept 1 → [post-critsmash-triage][adv-main40+][adv-esr38.2+] Will be published by Sept 1
Alias: CVE-2015-4481
You need to log in before you can comment on or make changes to this bug.