1212939 - (CVE-2016-2809) Maintenance Service updater File Deletion Elevation of Privilege

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Holger Fuhrmannek]

Attachment: poc.rb

The updater.exe can be made to delete arbitrary files. It's possible because in the UpdateLog class paths a truncated with snprintf to fit in a buffer of MAXPATH length.
With the ability to delete arbitrary files it´s possible to elevate privilege further.

The poc needs a firefox installed to "C:\Program Files (x86)\Nightly\" and deletes the firefox.exe.

Comment 1

[Daniel Veditz [:dveditz]]

Robert, can you confirm this, please?

Comment 2

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

Attachment: Don't delete a file if its path has been truncated -

Comment 3

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

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

Comment 4

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

Can someone suggest a security rating for this issue? It will need sec-approval in order to land unless it is somehow trunk only.

Comment 5

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

(In reply to Al Billings [:abillings] from comment #4)
> Can someone suggest a security rating for this issue? It will need
> sec-approval in order to land unless it is somehow trunk only.

My guess would be sec-moderate, because this can only be exploited by an attacker who already has some way to execute a specific process with specific arguments, but the effect you get, which is deleting arbitrary files, including beyond the user's permissions, seems too serious for sec-low.

Comment 6

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

The try push in comment 3 initially showed a rather concerning orange for the Win7 debug XPCShell run, but I couldn't reproduce it locally, so I thought I'd try retriggering the job, and sure enough, now it's green. ¯\_(ツ)_/¯

Comment 7

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

Comment on attachment 8682255 [details] [diff] [review]
Don't delete a file if its path has been truncated -

I think it would be safer to just not log if the path was truncated. Make sense?

Comment 8

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

Attachment: Disable update logging if given a log file path longer than MAX_PATH

Comment 9

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

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

Comment 10

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

Comment on attachment 8700210 [details] [diff] [review]
Disable update logging if given a log file path longer than MAX_PATH

This looks fine but this patch is actually a patch on top of the patch that was minused. Could you resubmit without the minused patch? Thanks

Comment 11

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)
> Comment on attachment 8700210 [details] [diff] [review]
> Disable update logging if given a log file path longer than MAX_PATH
> 
> This looks fine but this patch is actually a patch on top of the patch that
> was minused. Could you resubmit without the minused patch? Thanks

Actually it looks like that patch broke non-Windows builds, so I'll have to fix that as well.

Comment 12

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

Attachment: Disable update logging if given a log file path longer than MAX_PATH

Comment 13

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

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

Comment 14

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

Comment on attachment 8700752 [details] [diff] [review]
Disable update logging if given a log file path longer than MAX_PATH

Matt, what do you think of also checking for ".." in the path and not logging when it is present?

Comment 15

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

Comment on attachment 8700752 [details] [diff] [review]
Disable update logging if given a log file path longer than MAX_PATH

Discussed comment #14 with Matt over irc and we are going to go with this as it stands

Comment 16

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

Comment on attachment 8700752 [details] [diff] [review]
Disable update logging if given a log file path longer than MAX_PATH

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The root cause is probably clear from the patch, but how to exploit it would be found a couple of levels removed from there, and would also require already having arbitrary command execution.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I don't believe so.

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?
I don't have backports. The one file the patch touches is not often messed with, so I don't anticipate trouble here.

How likely is this patch to cause regressions; how much testing does it need?
Regressions don't seem likely; there's very little change in the logic, and the patch has been successfully run through the tryserver (comment 13).

Comment 17

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

Comment on attachment 8700752 [details] [diff] [review]
Disable update logging if given a log file path longer than MAX_PATH

Ok. As a sec-moderate, this doesn't need sec-approval+ to land so you can just check it in. The next question is whether we need to back port this to earlier branches or if it should just ride the trains to release.

Comment 18

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

(In reply to Al Billings [:abillings] from comment #17)
> Comment on attachment 8700752 [details] [diff] [review]
> Disable update logging if given a log file path longer than MAX_PATH
> 
> Ok. As a sec-moderate, this doesn't need sec-approval+ to land so you can
> just check it in. The next question is whether we need to back port this to
> earlier branches or if it should just ride the trains to release.

Oh, I knew sec-low didn't need approval, but I didn't know that was the case for moderate too; sorry about the spam, in that case.

My opinion would be to let this ride the trains; doesn't seem like a big enough deal to work on backporting it.

Comment 19

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

Thanks. It will be so then.

Yes, sec-approval is really focused on critical and high rated issues.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81e422d038d3

Comment 21

[Ada [:adalucinet]]

Reproduced with Nightly from 2015-12-27, under Windows 10 64-bit - as soon as I ran the attached script, firefox.exe is deleted from C:\Program Files (x86)\Nightly

With 46 beta 10 [1] and a Nightly build after the fix (from 2016-04-01), under the same OS, I get this prompt when running the poc.rb file - https://i.imgur.com/rDc7XMM.png. Matt, is this the expected outcome? Thanks in advance!

[1] Note that I edited the .rb file to point to Mozilla Firefox instead of Nightly

Comment 22

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

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #21)
> Reproduced with Nightly from 2015-12-27, under Windows 10 64-bit - as soon
> as I ran the attached script, firefox.exe is deleted from C:\Program Files
> (x86)\Nightly
> 
> With 46 beta 10 [1] and a Nightly build after the fix (from 2016-04-01),
> under the same OS, I get this prompt when running the poc.rb file -
> https://i.imgur.com/rDc7XMM.png. Matt, is this the expected outcome? Thanks
> in advance!
> 
> [1] Note that I edited the .rb file to point to Mozilla Firefox instead of
> Nightly

Hmm. No, I wouldn't call that "expected." That's the error you normally get when you try to run a 64-bit binary on a 32-bit OS, or it can happen if you try to run a binary that's corrupt in some way. I can't seem to reproduce it here, I get the expected behavior (meaning nothing really happening except errors in the log files). You might try reinstalling both Firefox and the Maintenance Service, and then try again?

Comment 23

[Ada [:adalucinet]]

(In reply to Matt Howell [:mhowell] from comment #22)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #21)
> > Reproduced with Nightly from 2015-12-27, under Windows 10 64-bit - as soon
> > as I ran the attached script, firefox.exe is deleted from C:\Program Files
> > (x86)\Nightly
> > 
> > With 46 beta 10 [1] and a Nightly build after the fix (from 2016-04-01),
> > under the same OS, I get this prompt when running the poc.rb file -
> > https://i.imgur.com/rDc7XMM.png. Matt, is this the expected outcome? Thanks
> > in advance!
> > 
> > [1] Note that I edited the .rb file to point to Mozilla Firefox instead of
> > Nightly
> 
> Hmm. No, I wouldn't call that "expected." That's the error you normally get
> when you try to run a 64-bit binary on a 32-bit OS, or it can happen if you
> try to run a binary that's corrupt in some way. I can't seem to reproduce it
> here, I get the expected behavior (meaning nothing really happening except
> errors in the log files). You might try reinstalling both Firefox and the
> Maintenance Service, and then try again?

Thanks for your prompt reply!
Apparently it was something wrong with my Ruby installer; after a reinstall, everything worked as expected.
Verified fixed with 46 beta 10 and latest Nightly 48.0a1 - firefox.exe is no longer deleted when running the poc.rb file. Marking here accordingly.

Comment 24

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

Comment on attachment 8700752 [details] [diff] [review]
Disable update logging if given a log file path longer than MAX_PATH

I'd like to take this patch on the next esr 45 to simplify landing the patch for bug 1246945.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: simplify landing of bug 1246945
User impact if declined: additional risk in landing bug 1246945
Fix Landed on Version: Firefox 46
Risk to taking this patch (and alternatives if risky): Extremely minimal.
String or UUID changes made by this patch: None

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

Comment 25

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

Comment on attachment 8700752 [details] [diff] [review]
Disable update logging if given a log file path longer than MAX_PATH

Needed for esr45 for a separate sec-high issue.

Comment 26

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

Pushed to mozilla-esr45
https://hg.mozilla.org/releases/mozilla-esr45/rev/4a0e851f83e4