Closed Bug 1336979 (CVE-2017-7768) Opened 3 years ago Closed 3 years ago

32 byte arbitrary file reads as SYSTEM with maintenance service

Categories

(Toolkit :: Application Update, defect)

51 Branch
Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: sebbity, Assigned: robert.strong.bugs)

Details

(Keywords: csectype-priv-escalation, sec-moderate, Whiteboard: [adv-main54+][adv-esr52.2+])

Attachments

(2 files)

Attached file 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
Summary: 32 byte arbitrary file reads as _SYSTEM with maintenance service → 32 byte arbitrary file reads as SYSTEM with maintenance service
Flags: sec-bounty?
Assignee: nobody → robert.strong.bugs
Attachment #8858139 - Flags: review?(mhowell)
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.
Attachment #8858139 - Flags: review?(mhowell) → review+
It is also recorded in telemetry.
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
Attachment #8858139 - Flags: sec-approval?
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?"
Attachment #8858139 - Flags: sec-approval?
It is a very safe and minor change. Might as well land it on the branches at the same time as the other ones.
https://hg.mozilla.org/mozilla-central/rev/fbec3b469121
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Attachment #8858139 - Flags: sec-approval?
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.
Attachment #8858139 - Flags: sec-approval? → sec-approval+
There are no further planned releases intended for the ESR45 branch. Please do request Beta & ESR52 approval on this, though :)
Flags: needinfo?(robert.strong.bugs)
Flags: needinfo?(robert.strong.bugs)
Flags: sec-bounty? → sec-bounty+
Tracking 54/55 and esr52+.
(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.
Flags: needinfo?(robert.strong.bugs)
I have a list of bugs that I will be requesting approval for beta and esr in the next couple of days.
Flags: needinfo?(robert.strong.bugs)
Group: toolkit-core-security → core-security-release
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.
Attachment #8858139 - Flags: approval-mozilla-esr52?
Attachment #8858139 - Flags: approval-mozilla-beta?
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.
Attachment #8858139 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

I will land this with other patches
Attachment #8858139 - Attachment description: patch → patch - rstrong will land this with other patches.
Comment on attachment 8858139 [details] [diff] [review]
patch - rstrong will land this with other patches.

Seems like a low risk sec fix, ESR52.2+
Attachment #8858139 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
(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.
Flags: qe-verify-
Whiteboard: [adv-main54+][adv-esr52.2+]
Alias: CVE-2017-7768
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.