mathiaswu Reporter
	Description • 6 years ago

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

	mathiaswu Reporter
	Comment 1 • 6 years ago

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

	:Gijs (he/him)
	Comment 2 • 6 years ago

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

	:Gijs (he/him)
	Comment 4 • 6 years ago

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

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

	:Gijs (he/him)
	Comment 5 • 6 years ago

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.

	:Gijs (he/him)
	Comment 6 • 6 years ago

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.

	:Gijs (he/him)
	Comment 7 • 6 years ago

(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?)

	Francesco Lodolo [:flod]
	Comment 8 • 6 years ago

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

	:Gijs (he/him)
	Comment 9 • 6 years ago

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

	Francesco Lodolo [:flod]
	Comment 10 • 6 years ago

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

	:Gijs (he/him)
	Comment 11 • 6 years ago

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

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 12 • 6 years ago

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

	:Gijs (he/him)
	Comment 13 • 6 years ago

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

	Francesco Lodolo [:flod]
	Comment 14 • 6 years ago

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 16 • 6 years ago

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

	Francesco Lodolo [:flod]
	Comment 17 • 6 years ago

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

Francesco Lodolo [:flod] has approved the revision.

	:Gijs (he/him)
	Comment 18 • 6 years ago

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

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

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 19 • 6 years ago

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

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 20 • 6 years ago

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

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 21 • 6 years ago

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

Approved for 63 beta 7, thanks.

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 22 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 6 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 24 • 6 years ago

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

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 25 • 6 years ago

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

Approved for 63 Beta 8, thanks.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 26 • 6 years ago

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