1490276 - (CVE-2018-12399) UI spoof when adding protocol handler

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect, P1)

Description

[mathiaswu]

Attachment: protocal_handler.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.81 Safari/537.36
Firefox for Android

Steps to reproduce:

online demo: http://f.3cm.me/r/phandler_spoof.html
attacker could spoof the handler's domain.


Actual results:

see protocal_handler.jpg


Expected results:

function registerProtocolHandler('mailto', 'url', 'title');
It should just show 
adding "title" on f.3cm as an application for mailto links

Comment 1

[mathiaswu]

a similiar vulnerability in google chrome has been fixed as https://bugs.chromium.org/p/chromium/issues/detail?id=347720

Comment 2

[:Gijs (he/him)]

:jkt, can you take a look? Looks like Chrome just ignores the title parameter. I'd suggest we should, too.

Comment 3

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1490276 - Removing title from addProtocolHandler message. r?Gijs

Comment 4

[:Gijs (he/him)]

Comment on attachment 9008056 [details]
Bug 1490276 - Removing title from addProtocolHandler message. r?Gijs

:Gijs (he/him) has approved the revision.

Comment 5

[:Gijs (he/him)]

Comment on attachment 9008056 [details]
Bug 1490276 - Removing title from addProtocolHandler message. r?Gijs

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Pretty easy - we're removing the title from the notification bar string in a sec-sensitive patch. Not hard to put 2 and 2 together...

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

Which older supported branches are affected by this flaw?
All the things.

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

Not easily, because l10n, AIUI. We've limited our exposure in bug 1476035 which is in 62. I don't know how severe we consider this bug and how much we care to backport. Some options:
- We could disable registerProtocolHandler, but that seems pretty extreme.
- We could conceivably create a backportable patch that just passes "" as the title instead of whatever the webpage gives us. From looking at https://transvision.mozfr.org/string/?entity=browser/chrome/browser/feeds/subscribe.properties:addProtocolHandler&repo=gecko_strings , AFAICT every single translation uses "%S (%S)" here so omitting it wouldn't look much more strange than it does in English (though perhaps *slightly* stranger in the languages that start with that, ie "%S (%S) something something something").
- We could conceivably create a backportable patch that forced the title string to be ascii [a-z0-9._-] but that seems pretty rude to non-en-US sites.

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

Comment 6

[:Gijs (he/him)]

Comment on attachment 9008056 [details]
Bug 1490276 - Removing title from addProtocolHandler message. r?Gijs

This got marked sec-low so now we can just land it.

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #5)
> - We could conceivably create a backportable patch that just passes "" as
> the title instead of whatever the webpage gives us. From looking at
> https://transvision.mozfr.org/string/?entity=browser/chrome/browser/feeds/
> subscribe.properties:addProtocolHandler&repo=gecko_strings , AFAICT every
> single translation uses "%S (%S)" here so omitting it wouldn't look much
> more strange than it does in English (though perhaps *slightly* stranger in
> the languages that start with that, ie "%S (%S) something something
> something").

:flod, how would you feel about this?


:jkt, can you push this to inbound? (Also, have you checked this doesn't break any browser tests?)

Comment 8

[Francesco Lodolo [:flod]]

I don't have access to the patch, so I'm having a hard time answering
https://phabricator.services.mozilla.com/D5517

Comment 9

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #8)
> I don't have access to the patch, so I'm having a hard time answering

I added you. bmo/phab should be doing that automatically, not sure why it didn't work here.

> https://phabricator.services.mozilla.com/D5517

I'm not sure what to do with this link, it points to a compare-locales patch?

Comment 10

[Francesco Lodolo [:flod]]

(In reply to :Gijs (he/him) from comment #9)
> (In reply to Francesco Lodolo [:flod] from comment #8)
> > I don't have access to the patch, so I'm having a hard time answering
> 
> I added you. bmo/phab should be doing that automatically, not sure why it
> didn't work here.

Thanks, I've added a few comments on the .properties change.

> > https://phabricator.services.mozilla.com/D5517
> 
> I'm not sure what to do with this link, it points to a compare-locales patch?

Clipboard failure, it was supposed to be a link to this bug's patch.

> We could conceivably create a backportable patch that just passes "" as the title instead of whatever the webpage gives us. 

We have a few exceptions:
kk: «%S» (%S) сілтемелер бағдарламасы ретінде %S үшін қалайсыз ба?
lt: Ar įtraukti „%S“ (%S) kaip programą %S saitams atverti?
ru: Добавить «%S» (%S) в качестве приложения для ссылок %S?
son: %S tonton (%S) sanda porogaram %S dobey se?

The last one is likely going to make little sense. with an empty title, but it's a small language, and we can live with that. Russian would be confusing, and that's a larger language, but still understandable.

I assume we need to wait for security to understand how far we need to uplift? Beta clearly wouldn't be a problem.

> We could conceivably create a backportable patch that forced the title string to be ascii [a-z0-9._-] but that seems pretty rude to non-en-US sites.

How would this mitigate the issue?

Comment 11

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #10)
> > We could conceivably create a backportable patch that forced the title string to be ascii [a-z0-9._-] but that seems pretty rude to non-en-US sites.
> 
> How would this mitigate the issue?

If we limit to the regex involved, the string the example uses ( "www.mozilla.org and mozilla's sponsor") would not pass the check.

Anyway, it might also break legit websites, and would still allow ending up with "Add foo.com (bar.com) as ..." which would still be completely confusing to users.

Comment 12

[Jonathan Kingston [:jkt] he/him]

> :jkt, can you push this to inbound?

I can but aren't we waiting for what rollout plan we are doing here?

> Also, have you checked this doesn't break any browser tests?

I ran all the relevant tests I could find and nothing seems to break.

Given the usage of this feature I still think the blank string backport solution is the best idea if we want to do this.

Comment 13

[:Gijs (he/him)]

(In reply to Jonathan Kingston [:jkt] from comment #12)
> > :jkt, can you push this to inbound?
> 
> I can but aren't we waiting for what rollout plan we are doing here?

It's marked sec-low, so you can land. If you want to have something for beta first and get me and flod to review that so we're ready to uplift that, that also wfm.

Comment 14

[Francesco Lodolo [:flod]]

For Beta it's fine to uplift the patch as is, even if it introduces a new string. 

In case the issue would be with release.

Comment 15

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

Comment 16

[Jonathan Kingston [:jkt] he/him]

I think we should uplift the above commit to Beta at least and decide if we want to do stable too.

Comment 17

[Francesco Lodolo [:flod]]

Comment on attachment 9009551 [details]
Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

Francesco Lodolo [:flod] has approved the revision.

Comment 18

[:Gijs (he/him)]

Comment on attachment 9009551 [details]
Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

:Gijs (he/him) has approved the revision.

Comment 19

[Jonathan Kingston [:jkt] he/him]

checkin-needed for: https://phabricator.services.mozilla.com/D5529

Comment 20

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 9009551 [details]
Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

Approval Request Comment
[Feature/Bug causing the regression]: Since implementation
[User impact if declined]: Users could be spoofed into accepting a protocol handler.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: The uplift patch just removes a variable and changed is to a blank string.
[String changes made/needed]: None

Comment 21

[Pascal Chevrel:pascalc]

Comment on attachment 9009551 [details]
Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

Approved for 63 beta 7, thanks.

Comment 22

[Pascal Chevrel:pascalc]

Comment on attachment 9009551 [details]
Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

Resetting the beta approval flag, the patch hasn't landed to central, I'll reapprove once it has actually landed.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/d275273b389b0919c8425438f8e41db283368d19

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/d275273b389b

Comment 25

[Pascal Chevrel:pascalc]

Comment on attachment 9009551 [details]
Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

Approved for 63 Beta 8, thanks.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-beta/rev/2af61ab83215