1348645 - (CVE-2017-7760) Maintenance Service updater PatchFile file manipulation

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Holger Fuhrmannek]

Attachment: poc.up.zip

During a partial update some files are updated by reading the old file as a source file and do some modifications to it. A vulnerability exists because under certain conditions instead of the path of the original file in the firefox installation directory the attacker chosen callback parameter path is used for loading the source file (see PatchFile::Execute in updater.cpp). The conditions are that the callback parameter path must have the length <path of original file>-1 and has to point to a file with the same name as the original file. 

This can be used to manipulate a file in the firefox directory and further to escalate privileges during a partial update with the maintenance service by manipulating the maintenanceservice.exe which will be executed with system rights during an update.

Steps to reproduce (tested with windows 7 64bit):

1.) install firefox 51 from https://ftp.mozilla.org/pub/firefox/releases/51.0.1/win32/en-US/Firefox%20Setup%2051.0.1.exe to C:\Program Files (x86)\Mozilla Firefox
2.) create a C:\updatework directory and download https://ftp.mozilla.org/pub/firefox/releases/52.0/update/win32/en-US/firefox-51.0.1-52.0.partial.mar as update.mar to it
3.) download and extract poc.up.zip
4.) create a C:\a directory and copy 123.dll (payload which executes calc.exe) from the poc directory to it
5.) create a C:\aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa directory and copy maintenanceservice.exe (manipulated to load the 123.dll) from the poc directory to it
6.) execute poc.bat

Some time after executing poc.bat you will see a calc.exe running with system rights in the process list.

Comment 1

[Daniel Veditz [:dveditz]]

Robert: who works on the maintenance service/updater these days? Please assign as appropriate.

Comment 2

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

Attachment: client patch rev1

Comment 3

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

Attachment: test patch rev1

Note: these patches rely on other bugs landing first.

Comment 4

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

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

Comment 5

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

Previous push had errors... new try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1c866057101c1b9e7c0ce62fe526fcf54acbd1d

Comment 6

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

Attachment: client patch rev1

Comment 7

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

Comment on attachment 8857849 [details] [diff] [review]
client patch rev1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not very easily and this exploit needs access to the system.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
In my opinion it doesn't.

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 it won't be hard to create them.

How likely is this patch to cause regressions; how much testing does it need?
Not very likely. I'll land this on oak and manually verify before landing on m-c as well.

Comment 8

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

Sec-approval+ for checkin on May 3, which is two weeks into the next cycle. We're releasing 53 in the next few days.

We'll want branch patches nominated as well so we can ship this on both ESR branches and 54 and 55.

Comment 9

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

Talked with abillings and I'll land this on June 28 so it can bake a few days before uplifing.

Comment 10

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

I meant 4/28

Comment 11

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

Attachment: client patch rev2

Forgot to cleanup the Mac command line when elevated.

Comment 12

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

Attachment: client patch for m-c

The difference is that I removed one incorrect comment.

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

Comment 13

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

Comment on attachment 8857819 [details] [diff] [review]
test patch rev1

Test patch pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c7ace47ac1e129d4fd0cac553696ed50c2f209

I have patches for beta and esr that I will attach soon.

Comment 14

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

Merged to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/8f7e91722471
https://hg.mozilla.org/mozilla-central/rev/f0c7ace47ac1

Comment 15

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

Attachment: client patch for beta

Approval Request Comment
[Feature/Bug causing the regression]:
File in use replacement a very long time ago
[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]:
Bug 1342742
[Is the change risky?]:
No more so than any other similar change to app update.
[Why is the change risky/not risky?]:
It validates the command line arguments used with the updater are what should be sent by Firefox. It is possible though not likely that some system might somehow munge the arguments. 
[String changes made/needed]:
None

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

Comment 16

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

Attachment: test patch for beta

This also needs the test changes from bug 1354850

Comment 17

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

Attachment: client patch for esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
They will continue to be vulnerable to this local exploit
Fix Landed on Version:
55
Risk to taking this patch (and alternatives if risky): 
No more so than any other similar change to app update. It validates the command line arguments used with the updater are what should be sent by Firefox. It is possible though not likely that some system might somehow munge the arguments. 
String or UUID changes made by this patch: 
None

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

Comment 18

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

Attachment: test patch for esr52

This also needs the test changes from bug 1354850

Comment 19

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

Comment on attachment 8863543 [details] [diff] [review]
client patch for beta

Sec-high, Beta54+

Comment 20

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

Comment on attachment 8863545 [details]
client patch for esr52

Sec-high, ESR52.2+

Comment 21

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

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

Comment 22

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

Pushed to mozilla-esr52
https://hg.mozilla.org/releases/mozilla-esr52/rev/e72789cd4486e5d309a127b6790398ca4689f44b
https://hg.mozilla.org/releases/mozilla-esr52/rev/fe41acbfab790675cee9f7305b7ca0db2ca6637b

I believe status-firefox-esr45 should be wontfix. If this is incorrect please let me know.

Comment 23

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