Closed Bug 1490276 (CVE-2018-12399) Opened 2 years ago Closed 2 years ago

UI spoof when adding protocol handler

Categories

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

62 Branch
defect

Tracking

(firefox-esr60 wontfix, firefox62 wontfix, firefox63 fixed, firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: ma7h1as.l, Assigned: jkt)

References

Details

(Keywords: csectype-spoof, sec-low, Whiteboard: [post-critsmash-triage][adv-main63+])

Attachments

(3 files)

Attached image 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
a similiar vulnerability in google chrome has been fixed as https://bugs.chromium.org/p/chromium/issues/detail?id=347720
:jkt, can you take a look? Looks like Chrome just ignores the title parameter. I'd suggest we should, too.
Flags: needinfo?(jkt)
Assignee: nobody → jkt
Flags: needinfo?(jkt)
Comment on attachment 9008056 [details]
Bug 1490276 - Removing title from addProtocolHandler message. r?Gijs

:Gijs (he/him) has approved the revision.
Attachment #9008056 - Flags: review+
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.
Attachment #9008056 - Flags: sec-approval?
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → RSS Discovery and Preview
Ever confirmed: true
Priority: -- → P1
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.
Attachment #9008056 - Flags: sec-approval?
(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?)
Flags: needinfo?(jkt)
Flags: needinfo?(francesco.lodolo)
I don't have access to the patch, so I'm having a hard time answering
https://phabricator.services.mozilla.com/D5517
(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?
(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?
Flags: needinfo?(francesco.lodolo)
(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.
> :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.
Flags: needinfo?(jkt)
(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.
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.
I think we should uplift the above commit to Beta at least and decide if we want to do stable too.
Comment on attachment 9009551 [details]
Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

Francesco Lodolo [:flod] has approved the revision.
Attachment #9009551 - Flags: review+
Comment on attachment 9009551 [details]
Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

:Gijs (he/him) has approved the revision.
Attachment #9009551 - Flags: review+
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
Attachment #9009551 - Flags: approval-mozilla-beta?
Comment on attachment 9009551 [details]
Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

Approved for 63 beta 7, thanks.
Attachment #9009551 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #9009551 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/d275273b389b
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Comment on attachment 9009551 [details]
Bug 1490276 - Removing title from addProtocolHandler message, uplift. r?Gijs r?flod

Approved for 63 Beta 8, thanks.
Attachment #9009551 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Summary: UI spoof when adding protocal handler → UI spoof when adding protocol handler
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63+]
Alias: CVE-2018-12399
Product: Firefox → Firefox Graveyard
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.