1246945 - (CVE-2016-5293) Write to arbitrary file with updater and moz maintenance service using updater.log hardlink

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Holger Fuhrmannek]

Attachment: 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.

Comment 1

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

Rob, can you take this and suggest a security rating?

Comment 2

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

Similar bugs have been rated sec-high. Looking into what can be done to mitigate this.

Comment 3

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

Matt, is this another one of these bugs that you'll be looking at?

Comment 4

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

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.

Comment 5

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

Attachment: 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.

Comment 6

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

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

Comment 7

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

Note: in bug 1168743 I am removing the append option from logging which also fixes this bug.

Comment 8

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

Comment on attachment 8761353 [details] [diff] [review]
Patch

Won't this suffer from the same issue as bug 1171518 when using CopyFile?

Comment 9

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

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

Comment 10

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

I think it would be better to go with bug 1168743

Comment 11

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

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!

Comment 12

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

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.

Comment 13

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

Works for me and thanks!

Comment 14

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

Attachment: 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.

Comment 15

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

Pushed patches in bug 1168743 to mozilla-inbound.

Pushed by [e-mail]:
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

Comment 16

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

After the patches from bug 1168743 have baked on m-c nightly for a while I'll request branch approval.

Comment 17

[Daniel Veditz [:dveditz]]

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.

Comment 18

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

After baking I found a regression (bug 1287176) that needs to be fixed first and I am working on.

Comment 19

[Daniel Veditz [:dveditz]]

Definitely a no-go for Fx48 then; maybe it'll make Firefox 49.

Comment 20

[Gerry Chang [:gchang]]

Hi :rstrong,
Since this patch also affects 49, are you also considering to uplift this patch to 49 beta?

Comment 21

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

I have considered it and don't think it should be uplifted at present due to comment #18.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Bug 1287176 is marked as unaffected for 49. Does that change things? Or do you think it also affects 49?

Comment 23

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

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.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

I'm happier to leave it in 50. Thanks!

Comment 25

[Sylvestre Ledru [:Sylvestre]]

seems that esr will have to leave with this bug, are we ok?

Comment 26

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

How about uplifting this to esr when this merges to beta?

Comment 27

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

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.

Comment 28

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

I have patches that apply to esr45 and the tests pass locally if this is wanted after the merge from aurora to beta.

Comment 29

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

I think we should take this on ESR45.5 (shipping with Firefox 50).

Comment 30

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

Requested esr45 on bug 1212939 to simplify the landing of this patch.

Comment 31

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

Attachment: 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.

Comment 32

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

Attachment: esr45 patch for bug 1299333

[Approval Request Comment]
See previous request

Comment 33

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

Note: I'll land this myself since there are several test patches that go along with it.

Comment 34

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

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?

Comment 35

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

or is this patch ESR only?

Comment 36

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

This patch is esr only

Comment 37

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

Ah, ok. Thank you for making this clear.

Comment 38

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

Liz, esr approval ping

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 40

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

Liz, this also needs the patch in bug 1212939. Can I get approval for that as well?

Comment 41

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

Pushed the patches from bug 1168743 to mozilla-esr45
https://hg.mozilla.org/releases/mozilla-esr45/rev/54f7eaaa6de3 test data files
https://hg.mozilla.org/releases/mozilla-esr45/rev/a1e06af61ab3 client code
https://hg.mozilla.org/releases/mozilla-esr45/rev/6ac20c152794 test code

Pushed the patches from bug 1287176 to mozilla-esr45
https://hg.mozilla.org/releases/mozilla-esr45/rev/0569d5dce9db client code
https://hg.mozilla.org/releases/mozilla-esr45/rev/804d7029982c test code

Comment 42

[Mihai Boldan, Desktop QA [:mboldan]]

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.

Comment 43

[Kamil Jozwiak [:kjozwiak]]

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