1527717 - (CVE-2019-9801) Windows programs that aren't "URL Handlers" exposed to web content

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P2)

Description

[Daniel Veditz [:dveditz]]

On Windows Firefox will accept any registered ProgID as an external protocol scheme and offer to launch the local application if given such a URL. Unless a program has specifically advertised itself as a "URL Handler" (and presumably addressed the security implications of remote untrusted content) we should NOT offer to launch them.

For an example see https://crbug.com/889459 where a python.file: URL was used as a key part of a remote execution bug (with user interaction). IE/Edge, meanwhile, safely ignore python.file: (and a zillion others) because they're checking for the presence of the URL Handler key.

We'd want to do a similar check probably in
https://searchfox.org/mozilla-central/source/uriloader/exthandler/win/nsOSHelperAppService.cpp#72

Comment 1

[Daniel Veditz [:dveditz]]

Note: the Chrome RCE POC doesn't work in Firefox because, in part, we store downloads under a temp name until the user agrees to save it. But with a little more social engineering you could probably get around that. The point is that we don't know what dangerous program people will find next, and even if know there will be security bugs in programs that are URL Handlers (Firefox has them!), I'm even more sure there will be problems in programs that aren't expecting to be used that way.

Link to the Chrome bug was found on a Polish web security website so it may prompt researchers to play with variants.

Comment 2

[Daniel Veditz [:dveditz]]

Jim, Rob: who deals with the Windows external app handler these days?

Comment 3

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

Maybe Jim knows but I have no idea.

Comment 4

[Jim Mathies [:jimm]]

Rob's going to take a look, looks like a pretty easy fix.

Comment 5

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

Attachment: patch

So much for this being simple... there are several tests that fail with this change
https://treeherder.mozilla.org/#/jobs?repo=try&revision=55e402aac8ff961d4a640b5b674f448afaa0028a

Comment 6

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

I think this could be due to the return for rv = regKey->Open(... being NS_FAILED when the key doesn't exist. Another try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d40fe66876ba96ecb1faead28a41ebafdd459fd

Comment 7

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

That definitely fixes the tests I ran locally fingers crossed

Comment 8

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

Attachment: Bug 1527717 - Check the registry for 'URL Protocol' when checking if a protocol handler exists. r?jmathies

Comment 9

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

Comment on attachment 9043853 [details]
Bug 1527717 - Check the registry for 'URL Protocol' when checking if a protocol handler exists. r?jmathies

Security Approval Request

How easily could an exploit be constructed based on the patch?

I don't think it could be done easily but it is difficult to know per comment #1

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No

Which older supported branches are affected by this flaw?

60

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

Bug 1357791

Do you have backports for the affected branches?

No

If not, how different, hard to create, and risky will they be?

Extremely simple

How likely is this patch to cause regressions; how much testing does it need?

Not very likely though if clients learned to use this behavior removing it would be a surprise to them.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 9043853 [details]
Bug 1527717 - Check the registry for 'URL Protocol' when checking if a protocol handler exists. r?jmathies

sec-moderate and sec-low bugs don't need explicit approval on mozilla-central, but since I'm here looking sec-approval=dveditz

Is it worth uplifting to beta? sec-moderate and no known attacks say "no", but the recently fixed equivalent Chrome bug (which is somewhat worse combined with other issues) might trigger people poking around.

Comment 11

[Daniel Veditz [:dveditz]]

We'll want this on ESR-60 eventually. Thunderbird in particular might be at risk of malicious mails with links to launch unsafe applications.

Comment 12

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

I verified that the patch applies to both beta and esr60.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/e3a71ebe4064261d62f1e36fc9915471663905c0
https://hg.mozilla.org/mozilla-central/rev/e3a71ebe4064

Comment 14

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

Comment on attachment 9043853 [details]
Bug 1527717 - Check the registry for 'URL Protocol' when checking if a protocol handler exists. r?jmathies

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1357791

User impact if declined

Potential security exploit ( see comment #1 comment #10 and comment #11 ).

Is this code covered by automated tests?

Unknown

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Simple patch that restores functionality that existed prior to Firefox 60.

String changes made/needed

None

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

Potential security exploit ( see comment #1 comment #10 and comment #11 ).

User impact if declined

Potential security exploit ( see comment #1 comment #10 and comment #11 ).

Fix Landed on Version

67

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Simple patch that restores functionality that existed prior to Firefox 60.

String or UUID changes made by this patch

None

Comment 15

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

Comment on attachment 9043853 [details]
Bug 1527717 - Check the registry for 'URL Protocol' when checking if a protocol handler exists. r?jmathies

Blocks a potential vulnerability, OK to uplift for beta 8.

Comment 16

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

Adding checkin-needed for the beta landing

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

There is no need to set checkin-needed for uplifts to beta etc., the bugzilla queries for approval-mozilla-beta+ etc. catch those bugs.

https://hg.mozilla.org/releases/mozilla-beta/rev/bb9e3868a7bf0e1eeca0209d9943732ffb855958

Comment 18

[Julien Cristau [:jcristau]]

Looks like we've got reftest failures on win64 debug on beta after this uplift:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&searchStr=windows%2C10%2Cx64%2Cdebug%2Creftests%2Cwith%2Ce10s%2Ctest-windows10-64%2Fdebug-reftest-e10s-1%2Cr-e10s%28r1%29&fromchange=72498207a0698c24cf368c2ad7b9f97386ef2cc7&selectedJob=228975324

Comment 19

[Julien Cristau [:jcristau]]

Nevermind, these go back further.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9043853 [details]
Bug 1527717 - Check the registry for 'URL Protocol' when checking if a protocol handler exists. r?jmathies

Fixes a publicly-disclosed attack vector. Approved for 60.6esr.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/d9dccac2c4c0