Bug 1246945 (CVE-2016-5293)

Write to arbitrary file with updater and moz maintenance service using updater.log hardlink

VERIFIED FIXED

Status

()

VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: hofusec, Assigned: rstrong)

Tracking

({sec-high})

unspecified
sec-high
Points:
---
Bug Flags:
sec-bounty +
qe-verify +

Firefox Tracking Flags

(firefox47 wontfix, firefox48 wontfix, firefox49+ wontfix, firefox-esr4550+ verified, firefox50+ verified, firefox51 verified, firefox52 verified)

Details

(Whiteboard: [post-critsmash-triage][adv-main50+][adv-esr45.5+] fixed by bug 1168743)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8717458 [details]
write.poc.zip

During an update a updater.log file is created in the update source directory. If possible the new log data are added to an existing log file. If the file is actually a hardlink the new data are added to an arbitrary file. 

The poc use the CreateHardlink tool from https://github.com/google/symboliclink-testing-tools to create the hardlink without the need of write access to the target file. 
I have tested the poc with win7 and ff 47 nightly installed to C:\Program Files (x86)\Nightly\:
To reproduce
- download poc and extract the files
- execute poc.rb
The poc adds amongst other data a user controlled new line to the updater.ini in the firefox directory.

This might be used to elevate privileges by overwriting a script or ini file which has a permissive parser which subsequently gets executed/parsed by a privileged process.
Rob, can you take this and suggest a security rating?
Flags: needinfo?(robert.strong.bugs)
Similar bugs have been rated sec-high. Looking into what can be done to mitigate this.
Flags: needinfo?(robert.strong.bugs)
Keywords: sec-high
Matt, is this another one of these bugs that you'll be looking at?
Flags: needinfo?(mhowell)
This one wasn't on my radar, but I'll go ahead and take it.

rstrong, did you ever have time to come up with any mitigation ideas? This exploit uses a replace request, so it's exploiting the fact that moving the existing log file to the new temporary path before we append to it preserves the hard link from the source path. My first thought is that we should be able to move that file in a way that breaks the link.
Assignee: nobody → mhowell
Flags: needinfo?(mhowell) → needinfo?(robert.strong.bugs)
Created attachment 8761353 [details] [diff] [review]
Patch

This patch copies the existing log file instead of moving it; the original is then deleted by already existing code. This copies the content of the log while breaking the hard link.
Flags: needinfo?(robert.strong.bugs)

Updated

2 years ago
Attachment #8761353 - Flags: review?(robert.strong.bugs)
Note: in bug 1168743 I am removing the append option from logging which also fixes this bug.
Comment on attachment 8761353 [details] [diff] [review]
Patch

Won't this suffer from the same issue as bug 1171518 when using CopyFile?
Attachment #8761353 - Flags: review?(robert.strong.bugs) → review-
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8)
> Won't this suffer from the same issue as bug 1171518 when using CopyFile?

Yeah, looks like it would. I could mitigate that to a degree by including some randomness in the temp file name, instead of a timestamp like GetTempFileName uses. Do you want to bother in light of comment 7 though? Removing the append functionality is definitely the better fix.
Flags: needinfo?(robert.strong.bugs)
I think it would be better to go with bug 1168743
Flags: needinfo?(robert.strong.bugs)
Al, the client patch in bug 1168743 addresses this bug and I'd like to know how you'd like to handle getting that landed. Would you prefer that I put the client patch that only fixes this bug in this bug for the security approvals, etc. for landing on other branches and if so would you like the client patch without the additional cleanup? I don't think the additional cleanup is that risky to land on other branches especially since we are early in this cycle but I'm not against reducing the changes. Also, the test patches in bug 1168743 will need to land on the branches as well. Thanks!
Flags: needinfo?(abillings)
I'd just ask for sec-approval here and branch approvals with the caveat to all involved that we're actually approving (quietly) the patches for bug 1168743, if that works for you. That's assuming that the patches there are review+ in general.
Flags: needinfo?(abillings)
Works for me and thanks!
Assignee: mhowell → robert.strong.bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8761353 - Attachment is obsolete: true
Created attachment 8762239 [details] [diff] [review]
bug 1168743 client patch

Attaching client patch from bug 1168743 so I have something to request sec-approcal on

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily at all since this fix was done as part of bug 1168743.

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? It was introduced in Firefox 42 in bug 1171518 to fix a different security bug. So, all supported branches.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I don't expect any difficulty in creating backports.

How likely is this patch to cause regressions; how much testing does it need? Not very likely but I'd like this to spend a week or two on nightly before uplifting as with all app update changes. With this being the start of the cycle this shouldn't be a problem.
Attachment #8762239 - Flags: sec-approval?
Attachment #8762239 - Flags: review+
Attachment #8762239 - Flags: sec-approval? → sec-approval+
Pushed patches in bug 1168743 to mozilla-inbound.

Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a78bd6c722f8
test data files - Only use env vars in the test updater and general app update cleanup. r=mhowell
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f965be1037f
client changes - Only use env vars in the test updater and general app update cleanup. r=mhowell
https://hg.mozilla.org/integration/mozilla-inbound/rev/c69b17415feb
test changes - Only use env vars in the test updater and general app update cleanup. r=mhowell
After the patches from bug 1168743 have baked on m-c nightly for a while I'll request branch approval.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Group: toolkit-core-security → core-security-release
Has it baked enough? It's too late to get this into Beta but would be nice to get it into Aurora so it has the full Beta period for testing.
status-firefox47: --- → wontfix
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → fixed
status-firefox-esr45: --- → affected
tracking-firefox49: --- → +
tracking-firefox50: --- → +
Flags: needinfo?(robert.strong.bugs)
Whiteboard: fixed by bug 1168743
After baking I found a regression (bug 1287176) that needs to be fixed first and I am working on.
Flags: needinfo?(robert.strong.bugs)
Definitely a no-go for Fx48 then; maybe it'll make Firefox 49.
status-firefox48: affected → wontfix
tracking-firefox-esr45: --- → 49+
Hi :rstrong,
Since this patch also affects 49, are you also considering to uplift this patch to 49 beta?
Flags: needinfo?(robert.strong.bugs)
I have considered it and don't think it should be uplifted at present due to comment #18.
Flags: needinfo?(robert.strong.bugs)
Bug 1287176 is marked as unaffected for 49. Does that change things? Or do you think it also affects 49?
status-firefox49: affected → fix-optional
Flags: needinfo?(robert.strong.bugs)
The patch in bug 1168743 which fixes this bug caused bug 1287176. I'd like the patch for bug 1287176 to bake a little longer but after that I am ok with uplifting bug 1168743 and bug 1287176 if you want them. Please note that like most of the app update bugs like this that local system access is necessary to exploit this and afaik we haven't had reports of this ever happening.
Flags: needinfo?(robert.strong.bugs) → needinfo?(lhenry)
I'm happier to leave it in 50. Thanks!
status-firefox49: fix-optional → wontfix
Flags: needinfo?(lhenry)
seems that esr will have to leave with this bug, are we ok?
Flags: sec-bounty?
How about uplifting this to esr when this merges to beta?
If bug 1168743 is uplifted to esr45 I'd like to take bug 1212939 as well to simplify the changes needed for the esr45 patch. In case we do decide at some point to uplift to esr45 I've created an esr45 patch for bug 1168743 and will soon create one for bug 1287176.
I have patches that apply to esr45 and the tests pass locally if this is wanted after the merge from aurora to beta.
I think we should take this on ESR45.5 (shipping with Firefox 50).
tracking-firefox-esr45: 49+ → 50+
Flags: sec-bounty? → sec-bounty+
Requested esr45 on bug 1212939 to simplify the landing of this patch.
Created attachment 8790403 [details] [diff] [review]
esr45 bug 1168743 client patch

This patch requires the patch from bug 1212939 and bug 1287176 (I'll attach the patch for bug 1299333 in this bug as well).

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible to write to arbitrary file with local computer access using the updater and moz maintenance service and an updater log hardlink
Fix Landed on Version: Firefox 50
Risk to taking this patch (and alternatives if risky): Minimal. The fix and the regression the patch caused has baked on Nightly and Aurora.
String or UUID changes made by this patch:  None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily at all since this fix was done as part of bug 1168743.

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?
It was introduced in Firefox 42 in bug 1171518 to fix a different security bug. So, all supported branches.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes, I do.

How likely is this patch to cause regressions; how much testing does it need?
Not likely since it has baked on Nightly and Aurora for quite some time. Only normal update testing should be needed.
Attachment #8790403 - Flags: sec-approval?
Attachment #8790403 - Flags: review+
Attachment #8790403 - Flags: approval-mozilla-esr45?
Created attachment 8790404 [details] [diff] [review]
esr45 patch for bug 1299333

[Approval Request Comment]
See previous request
Attachment #8790404 - Flags: approval-mozilla-esr45?
Note: I'll land this myself since there are several test patches that go along with it.
This can land on 9/20. sec-approval for that date for trunk. I think we'll want to move this up the branches after things are settled on trunk.

The flags on this bug are confusing since it isn't really "fixed" on 50 is it?
Whiteboard: fixed by bug 1168743 → fixed by bug 1168743 [checkin on 9/20]
Attachment #8790403 - Flags: sec-approval? → sec-approval+
or is this patch ESR only?
This patch is esr only
Ah, ok. Thank you for making this clear.
Liz, esr approval ping
Flags: needinfo?(lhenry)
Comment on attachment 8790403 [details] [diff] [review]
esr45 bug 1168743 client patch

Already landed in 50, sec-high, let's take this for ESR45.5.0.
Flags: needinfo?(lhenry)
Attachment #8790403 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Liz, this also needs the patch in bug 1212939. Can I get approval for that as well?
Flags: needinfo?(lhenry)
Flags: needinfo?(lhenry)
Attachment #8790404 - Flags: approval-mozilla-esr45?
Flags: qe-verify+
QA Contact: kjozwiak
Whiteboard: fixed by bug 1168743 [checkin on 9/20] → fixed by bug 1168743 [checkin on 9/20][post-critsmash-triage]
Whiteboard: fixed by bug 1168743 [checkin on 9/20][post-critsmash-triage] → fixed by bug 1168743 [checkin on 9/20][post-critsmash-triage][adv-main50+][adv-esr45.5+]
I managed to reproduce this issue using Firefox 49.0.2 build 2 and STR from Comment 0.
The issue is no longer reproducible on Firefox 45.5.0ESR, Firefox 50.0, Firefox 51.0a2(2016-11-08), or on Firefox 52.0a1(2016-11-07).

Tests were performed under Windows 10x64.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
status-firefox51: --- → verified
status-firefox52: --- → verified
status-firefox-esr45: fixed → verified
As :mboldan used Win 10 x64 for verification, I double checked using Win 7 as it was the original platform used in comment#0.

Reproduced the issue using the POC from comment#0 with the following build:
* https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-09-03-03-47-mozilla-central/

Platform used:
* Win 7 SP1 x64 - PASSED

Changed the appropriate paths under "write.poc\poc.rb" when using fx51.0a2 and fx50.0b9.

FX builds being used:
* fx52.0a1, buildId: 20161108030212, changeset: f13e90d496cf - PASSED
* fx51.0a2, buildId: 20161108004019, changeset: d9cfe58247e8 - PASSED
* fx50.0b9, buildId: 20161020152750, changeset: 2bb6dc758711 - PASSED
* fx45.5.0, buildId: 20161031153904, changeset: 32ce7be98543 - PASSED
Alias: CVE-2016-5293
Whiteboard: fixed by bug 1168743 [checkin on 9/20][post-critsmash-triage][adv-main50+][adv-esr45.5+] → [post-critsmash-triage][adv-main50+][adv-esr45.5+] fixed by bug 1168743
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.