Closed Bug 1348645 (CVE-2017-7760) Opened 3 years ago Closed 3 years ago

Maintenance Service updater PatchFile file manipulation

Categories

(Toolkit :: Application Update, defect)

defect
Not set

Tracking

()

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

People

(Reporter: hofusec, Assigned: rstrong)

Details

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

Attachments

(7 files, 3 obsolete files)

146.52 KB, application/zip
Details
21.67 KB, patch
mhowell
: review+
Details | Diff | Splinter Review
2.81 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
2.92 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
21.67 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
2.92 KB, text/plain
rstrong
: review+
Details
21.91 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
Attached file 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.
Robert: who works on the maintenance service/updater these days? Please assign as appropriate.
Flags: needinfo?(robert.strong.bugs)
Assignee: nobody → robert.strong.bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(robert.strong.bugs)
Attached patch test patch rev1Splinter Review
Note: these patches rely on other bugs landing first.
Attached patch client patch rev1 (obsolete) — Splinter Review
Attachment #8857818 - Attachment is obsolete: true
Attachment #8857819 - Attachment is patch: true
Attachment #8857849 - Flags: review?(mhowell) → review+
Attachment #8857819 - Flags: review?(mhowell) → review+
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.
Attachment #8857849 - Flags: sec-approval?
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.
Attachment #8857849 - Flags: sec-approval? → sec-approval+
Talked with abillings and I'll land this on June 28 so it can bake a few days before uplifing.
Whiteboard: [checkin on 5/3] → [checkin on 4/28]
Attached patch client patch rev2 (obsolete) — Splinter Review
Forgot to cleanup the Mac command line when elevated.
Attachment #8857849 - Attachment is obsolete: true
Attachment #8860767 - Flags: review?(mhowell)
Attachment #8860767 - Flags: review?(mhowell) → review+
The difference is that I removed one incorrect comment.

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7e91722471a3a114f475a8aecd19a5d608d31c
Attachment #8860767 - Attachment is obsolete: true
Attachment #8863069 - Flags: review+
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.
Whiteboard: [checkin on 4/28]
Merged to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/8f7e91722471
https://hg.mozilla.org/mozilla-central/rev/f0c7ace47ac1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Attachment #8863543 - Flags: review+
Attachment #8863543 - Flags: approval-mozilla-beta?
This also needs the test changes from bug 1354850
Attachment #8863544 - Flags: review+
Attached file 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.
Attachment #8863545 - Flags: review+
Attachment #8863545 - Flags: approval-mozilla-esr52?
This also needs the test changes from bug 1354850
Attachment #8863547 - Flags: review+
Group: toolkit-core-security → core-security-release
Comment on attachment 8863543 [details] [diff] [review]
client patch for beta

Sec-high, Beta54+
Attachment #8863543 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8863545 [details]
client patch for esr52

Sec-high, ESR52.2+
Attachment #8863545 - 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-
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main54+][adv-esr52.2+]
Alias: CVE-2017-7760
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.