1171518 - (CVE-2015-4481) [Win] Privileged update processes writing to user writable locations can overwrite non-user writable locations using hard links

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[James Forshaw]

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

Comment 1

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

Already looking at it

Comment 2

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

James, thank you for the clear and concise bug report. It is very much appreciated.

Comment 3

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

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

Comment 4

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

Comment 5

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

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.

Comment 6

[James Forshaw]

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.

Comment 7

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

Attachment: patch rev1

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.

Comment 8

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

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.

Comment 9

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

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

Comment 10

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

I'll deal with the removal of the old logs dir in a separate patch.

Comment 11

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

Comment on attachment 8616349 [details] [diff] [review]
patch rev1

This was passing tests locally but Try run had failures... investigating

Comment 12

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

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

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

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.

Comment 14

[Brian R. Bondy [:bbondy]]

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?

Comment 15

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

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

Comment 16

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

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

Comment 17

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

Attachment: updater patch rev1

I'm going to go with a separate patch for the installer.

Try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f3b35592bb5

Comment 18

[Brian R. Bondy [:bbondy]]

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.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 20

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

I'd prefer this goes in for 40 per comment #16.

Comment 21

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

Rob, I see an R+ patch here from three weeks ago. Can we get this in?

Comment 22

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

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.

Comment 23

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

Attachment: installer patch

Comment 24

[Brian R. Bondy [:bbondy]]

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.

Comment 25

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

Attachment: combined patch (nightly, aurora, and beta)

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

Comment 26

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

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.

Comment 27

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

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/98c0d3e9c6f1

Comment 28

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

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.

Comment 29

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

Attachment: combined patch for esr38

Comment 30

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

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.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98c0d3e9c6f1

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Robert, could you fill the uplift request for aurora, beta and esr38? Thanks

Comment 33

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

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

Comment 34

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

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

Comment 35

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

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.

Comment 36

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63e39f82b316

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/177144e23d02

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/9116d0cf6430

Comment 40

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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?

Comment 41

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

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.

Comment 42

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

Hi James, have you had a chance to examine this fix? Let us know if it still reproduces.

Comment 43

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

Comment 44

[Daniel Veditz [:dveditz]]

The Google bug has been unhidden
  https://code.google.com/p/google-security-research/issues/detail?id=427

And it was added to the offensive-security exploit database on Aug 22
https://github.com/offensive-security/exploit-database/commit/40a9571fd740b7a44d704cd8ba0b10a79b6df578