1319157 - (CVE-2018-5169) Home button drop handler should urlSecurityCheck all the links in a pipe (|) separated set of URLs dropped on it

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

... or just not allow dropping such a set of URLs.

Comment 1

[:Gijs (he/him)]

I think this was already fixed, in bug 1379842. :arai, can you confirm?

Comment 2

[Tooru Fujisawa [:arai]]

can you describe the attack scenario?
so that I can test.

Comment 3

[:Gijs (he/him)]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> can you describe the attack scenario?
> so that I can test.

If you selected text like this from a webpage:

http://www.example.com/|chrome://browser/content/

and drag it to the home button, it'd set your homepage to 2 tabs, example.com and chrome://browser/content/ .

This shouldn't be allowed, because the web page can't open (link to) chrome://browser/content/ , so neither should it be able to trick you into opening it.

When I looked at dom/base/contentAreaDropListener.js, it looked like it security-checks each URI so will remove ones that you can't link to based on the triggering principal.

Having just actually tested it, it looks like it's still broken, even though I thought this was, at the latest, fixed by bug 1424107.

I expect the dropped link handler doesn't split on '|' but the home button does. So either the dropped link handler needs to be taught about this, or we stop making this type of multiple-links stuff work in the home button, or we add securitychecking in the home button code itself, or something.

Comment 4

[Tooru Fujisawa [:arai]]

Thanks.
currently the security check is applied to each *line*, not URL.
I'll fix this.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Check each URIs for home button.

To perform extra check on the dropped items, I added nsIDroppedLinkHandler.validateURIsForDrop
that receives the drop event and URIs,
so that we can reuse the code that validates the URIs, in contentAreaDropListener.js.

now homeButtonObserver.onDrop extracts URIs from the dropped text, by splitting them by "|", and check them by nsIDroppedLinkHandler.validateURIsForDrop.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8953057 [details] [diff] [review]
Check each URIs for home button.

Review of attachment 8953057 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me. I don't know that we need to actually show a message to the user. If a user clicks a link to a chrome: URI that we block, we show no user-visible message at all. We just warn in the browser console (SecurityError: so-and-so may not link to so-and-so). I think we could do the same here, but it's up to you.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +9,5 @@
>  droponhomemsg=Do you want this document to be your new home page?
>  droponhomemsgMultiple=Do you want these documents to be your new home pages?
>  
> +droponhome.securityerror.title=This website cannot set the URL as homepage
> +droponhome.securityerror.message=This website does not have enough privilege to set the given URLs as homepage.

Maybe "This website is not allowed to set these URLs as your homepage."

Comment 7

[Tooru Fujisawa [:arai]]

Thanks, I'll remove the alert and land this patch at the same time as bug 1435910 and bug 1435908.

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/083f95ede8ff604ae269c44755112df03b6cfe58
Bug 1319157 - Check each URIs for home button. r=Gijs

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/083f95ede8ff

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Letting this ride the trains since it's a sec-moderate and we're about to build the Fx59 RC build.