1247239 - (CVE-2016-5295) moz maintenance service: Ability to read arbitrary files as SYSTEM

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Holger Fuhrmannek]

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.

Comment 1

[Holger Fuhrmannek]

Attachment: read.poc.zip

Comment 2

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

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.

Comment 3

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

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.

Comment 4

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

Attachment: Set tighter permissions on copies of updater.exe created by the maintenance service

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e5a005b1677

Comment 6

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

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 7

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

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.

Comment 8

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

Attachment: Remove copying of updater.exe for service updates

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cefa6a2f5fae

Comment 10

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

Comment on attachment 8737405 [details] [diff] [review]
Remove copying of updater.exe for service updates

Thanks!

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?

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/maintenanceservice/bootstrapinstaller/maintenanceservice_installer.nsi#260

http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/maintenanceservice_installer.nsi#261

http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/maintenanceservice_installer.nsi#321

Comment 11

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

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/0712c02d259e1e0dab776372bc52ee22b0c92091

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0712c02d259e

Comment 14

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

This regressed staging updates. It falls back to applying updates without the service. :(

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out at Robert's request.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b906bf97ccc750b6e9fbf6e8676b303d73be55b1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/b906bf97ccc7

Comment 17

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

Attachment: Patch - Only copy updater.exe from install path, and verify it first

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.

Comment 18

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=398009ebb123

Comment 19

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

Comment on attachment 8769296 [details] [diff] [review]
Patch - Only copy updater.exe from install path, and verify it first

lgtm and thanks!

Comment 20

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

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cca6d362e38

Comment 22

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

How did this land without sec-approval+ on the patch?

Comment 23

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

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.

Comment 24

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

Ah, I missed it was sec-moderate. My bad.

Comment 25

[Kamil Jozwiak [:kjozwiak]]

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

Comment 26

[Kamil Jozwiak [:kjozwiak]]

(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