Closed Bug 1247239 (CVE-2016-5295) Opened 5 years ago Closed 4 years ago

moz maintenance service: Ability to read arbitrary files as SYSTEM

Categories

(Toolkit :: Application Update, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- verified

People

(Reporter: hofusec, Assigned: mhowell)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main50+])

Attachments

(2 files, 2 obsolete files)

I noticed the problem is known (Bug 850492) but wasn't fixed because the poc was not working. I have developed a poc which relies on the Volume Snapshot Service of windows to copy locked protected files. I tested the poc with the moz maintenance service at C:\Program Files (x86)\Mozilla Maintenance Service and win7 64 bit. 
If the Volume Snapshot Service is enabled and already a restore point was created (!) the poc is able to copy the C:\Windows\system32\config\SAM file and other protected files.
Attached file read.poc.zip
Flags: needinfo?(robert.strong.bugs)
I'm attempting to fix this by having the service claim an exclusive lock on the copy of updater.exe while performing the copy and the validation steps, so that it's not possible for an outside process to read the file until after we've determined that it's a good updater.exe. Hopefully I'll have a patch ready to post here soon.

Rating this as sec-moderate because it can't be exploited remotely without the use of other vulnerabilities to get command execution with arguments.
Assignee: nobody → mhowell
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(robert.strong.bugs)
Keywords: sec-moderate
Well, that plan did not survive contact with reality. It turns out most of the validation process involves API's that attempt to open their own handles, so holding one with an exclusive lock keeps those from working, and it isn't exactly worth rewriting them all.

There's a second option, which is to still hold that lock while performing the copy, but then apply a tightly limited set of permissions to the copied file and then release the lock. This approach seems to work, in that I can no longer reproduce the bug with that fix in place, and tests are passing locally. So we'll go with that instead, unless a better idea comes along.
Attachment #8726482 - Flags: review?(robert.strong.bugs)
I'm checking out the patch but I have to say that setting restrictive permissions to try to fix these types of issues always scares me since it could possibly lock out part of the process.
Comment on attachment 8726482 [details] [diff] [review]
Set tighter permissions on copies of updater.exe created by the maintenance service

So, copying the updater was added to prevent an exploit with the maintenance service when we were using the updater from the updates/0 directory. We no longer do that per bug 1127481. It should be possible to not copy the file and run the updater from the install directory for the service as well to fix this.
Attachment #8726482 - Flags: review?(robert.strong.bugs) → review-
Attachment #8726482 - Attachment is obsolete: true
Attachment #8737405 - Flags: review?(robert.strong.bugs)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)
> At some point we should also remove the following removals for the /update
> directory as well but I'd like to wait a bit. Could you file a followup for
> this?

Filed bug 1261978.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0712c02d259e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This regressed staging updates. It falls back to applying updates without the service. :(
Backed out at Robert's request.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b906bf97ccc750b6e9fbf6e8676b303d73be55b1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla48 → ---
Okay, here's a different way to deal with this. This patch has the service ignore the updater.exe path parameter and always use the one in the installation directory instead. Also, prior to making that copy, it performs the same verification steps that we run before actually running the updater; otherwise, it would still be possible to use this exploit to read an unrelated file that happens to be named updater.exe.
Attachment #8737405 - Attachment is obsolete: true
Attachment #8769296 - Flags: review?(robert.strong.bugs)
Comment on attachment 8769296 [details] [diff] [review]
Patch - Only copy updater.exe from install path, and verify it first

lgtm and thanks!
Attachment #8769296 - Flags: review?(robert.strong.bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cca6d362e38a4eb1f680c62a17137e04b1169f2
Bug 1247239 - Allow the maintenance service to copy updater.exe only from the installation directory, and verify it first; r=rstrong
https://hg.mozilla.org/mozilla-central/rev/6cca6d362e38
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Group: toolkit-core-security → core-security-release
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Flags: qe-verify-
QA Contact: kjozwiak
Whiteboard: [post-critsmash-triage]
How did this land without sec-approval+ on the patch?
Flags: needinfo?(mhowell)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+]
I asked for approval on a sec-moderate bug once before and was told I didn't need it. The wiki (https://wiki.mozilla.org/Security/Bug_Approval_Process) confirms that.
Flags: needinfo?(mhowell) → needinfo?(abillings)
Ah, I missed it was sec-moderate. My bad.
Flags: needinfo?(abillings)
Reproduced the issue using the poc from comment#0 with the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-10-07-11-15-mozilla-central/

After running the poc from comment#0, the following protected files were copied into the C:\read.poc directory:

* C:\Windows\System32\config\SECURITY
* C:\Windows\System32\config\SECURITY
* C:\Windows\System32\config\software
* C:\Windows\System32\config\system

Went through verification using the following builds:

* Win 7 SP1 x64 - PASSED
** fx52.0a1, buildId: 20161108030212, changeset: f13e90d496cf - PASSED
** fx51.0a2, buildId: 20161108004019, changeset: d9cfe58247e8 - PASSED
** fx50.0b9, buildId: 20161020152750, changeset: 2bb6dc758711 - PASSED
Status: RESOLVED → VERIFIED
(In reply to Kamil Jozwiak [:kjozwiak] from comment #25)
> * C:\Windows\System32\config\SECURITY
> * C:\Windows\System32\config\SECURITY
> * C:\Windows\System32\config\software
> * C:\Windows\System32\config\system

The first entry is supposed to be:

* C:\Windows\System32\config\SAM
Alias: CVE-2016-5295
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.