1336979 - (CVE-2017-7768) 32 byte arbitrary file reads as SYSTEM with maintenance service

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Seb Patane (he/him)]

Attachment: proof-of-concept

An unprivileged (standard) user can use the maintenance service to read the first 32 bytes of any file as the _SYSTEM user. Which isn't amazing, but still better than nothing :)

For example, running the poc with arbitrary_read.rb "C:\Users\Seb\creds.txt" will return the first 32 bytes of the first line in the shell. The full 32 bytes of multiline files can be seen in the C:\Program Files (x86)\Mozilla Maintenance Service\logs\maintenanceservice.log file.

The root cause is that we can trick the maintenance service into reading an arbitrary file in IsStatusApplying [0] by providing a hardlinked update.status where it expects one from the updater.exe. Even though the updater.exe runs, it seems to do so in such a way that it doesn't remove/update the update.status that we have prepared.

When the maintenance service goes to read the update.status from the updater, instead it reads the hardlinked file as _SYSTEM. It then writes this to the maintenanceservice.log file [1], which is world-readable.

The buffer is only 32 bytes long, so that's all we get out of the file. If all 32 bytes are read, the buf string isn't null terminated so the memory past the end of the buffer is logged as well (until it hits a null byte).

The proof of concept is a little bit rushed sorry - although you can create a hardlinked file as an unprivileged user, I've been having trouble deleting them without elevating. Either elevating and removing the hardlink before rerunning the poc, or using a different directory should fix that. The CreateHardlink binary is a compiled version from https://github.com/google/symboliclink-testing-tools/tree/master/CreateHardlink.

Tested on Windows 8.1 x64, Firefox 51.0.1 as well as mozilla-central.

[0] - https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/toolkit/components/maintenanceservice/workmonitor.cpp#69
[1] - https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/toolkit/components/maintenanceservice/workmonitor.cpp#74

Comment 1

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

Attachment: patch - rstrong will land this with other patches.

Comment 2

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

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

I wouldn't be okay with this if we were losing this logging altogether, but we still get some more limited output from IsStatusApplying's caller StartUpdateProcess, so I think this should be fine.

Comment 3

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

It is also recorded in telemetry.

Comment 4

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

Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
As I see it, it isn't obvious at all

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not at all

Which older supported branches are affected by this flaw?
All

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?
Not yet but trivial to create if this patch doesn't apply.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely

Comment 5

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

Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

As a sec-moderate, this doesn't need sec-approval to land on trunk.

Do you think we want to take this on branches or should it just "ride the trains?"

Comment 6

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

It is a very safe and minor change. Might as well land it on the branches at the same time as the other ones.

Comment 7

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

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbec3b469121e53b1b77a0fbb9f365274f0762d2

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fbec3b469121

Comment 9

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

Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

(In reply to Al Billings [:abillings] from comment #5)
> Comment on attachment 8858139 [details] [diff] [review]
> patch
> 
> As a sec-moderate, this doesn't need sec-approval to land on trunk.
> 
> Do you think we want to take this on branches or should it just "ride the
> trains?"

Not sure if it needs sec-approval to land other than trunk so requesting since I'd like to land this at the same time as a couple of other bugs in early May.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
As I see it, it isn't obvious at all

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not at all

Which older supported branches are affected by this flaw?
All

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?
Not yet but trivial to create if this patch doesn't apply.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely

Comment 10

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

Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

This doesn't really need sec-approval (which only applies to trunk). If we want to land this on branches immediately after it lands on trunk, we should do it early in the cycle (like May 3). Please nominate patches for the affected branches and I or Release Management can approve them.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

There are no further planned releases intended for the ESR45 branch. Please do request Beta & ESR52 approval on this, though :)

Comment 12

[Marcia Knous [:marcia]]

Tracking 54/55 and esr52+.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> There are no further planned releases intended for the ESR45 branch. Please
> do request Beta & ESR52 approval on this, though :)

Hi Robert, you were needinfo'd on this for approval and then just canceled the needinfo.  Was that intentional and you plan to request beta and ESR approval still, or...?  Just not clear who's holding the ball here.

Comment 14

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

I have a list of bugs that I will be requesting approval for beta and esr in the next couple of days.

Comment 15

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

Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
I'd prefer keeping the code consistent between branches and this is a very simple fix.
User impact if declined:
They will still be vulnerable to this local exploit
Fix Landed on Version:
55
Risk to taking this patch (and alternatives if risky): 
This is not risky (see below)
String or UUID changes made by this patch: 
None
See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]:
Original maintenance service implementation
[User impact if declined]:
They will continue to be vulnerable to this local exploit
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]:
I have verified it on Nightly
[Needs manual test from QE? If yes, steps to reproduce]:
No
[List of other uplifts needed for the feature/fix]:
N/A
[Is the change risky?]:
This is not risky
[Why is the change risky/not risky?]:
It removes logging that is not needed
[String changes made/needed]:
None

I have also built beta and esr52 with this patch and all update tests pass.

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

Fix a security issue. Beta 54+. Should be in 54 beta 5.

Comment 17

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

Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

I will land this with other patches

Comment 18

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

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

Comment 19

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

Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

Seems like a low risk sec fix, ESR52.2+

Comment 20

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

Pushed to mozilla-esr52
https://hg.mozilla.org/releases/mozilla-esr52/rev/8bbc7b586d68b601e5b21807db4b8d91ea667ad7

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Robert Strong PTO 5/5 [:rstrong] (use needinfo to contact me) from comment #15)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]:
> I have verified it on Nightly
> [Needs manual test from QE? If yes, steps to reproduce]:
> No

Setting qe-verify- based on Robert's assessment on manual testing needs and the fact that this fix has automated coverage.