1229426 - (CVE-2017-5458) "javascript:" scheme not removed for drag and drop, unlike pasting into the URL bar

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Daniel Veditz [:dveditz]]

As an anti-self-XSS feature (where people are lured into hacking themselves, typically their social media accounts) we remove the javascript: scheme when people paste URLs into the Location bar. We should be doing the same for drag and drop, which is also used for the same kinds of self-xss attacks.

At least we don't execute the javascript: url when dropped so it's not terrible, but it leaves the victim one innocuous-sounding instruction ('hit return') away from the same result.

Comment 1

[Daniel Veditz [:dveditz]]

This can be seen in attachment 8693540 [details] from bug 1228997

Comment 2

[Masatoshi Kimura [:emk]]

I don't think it makes sense to prevent javascript: URLs from dropping to the URL bar. If the attacker can generate a link on the page, he can just instruct victims to click the link.

Comment 3

[:Gijs (he/him)]

(In reply to Masatoshi Kimura [:emk] from comment #2)
> I don't think it makes sense to prevent javascript: URLs from dropping to
> the URL bar. If the attacker can generate a link on the page, he can just
> instruct victims to click the link.

1) not all cases where the attacker can provide text to dnd also allow said attacker to generate "real" links
2) there are subtle behavioural differences between pasting a URL and then hitting enter, versus clicking a link. Those differences can be abused. We should avoid that being (easily) possible.

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1229426 - avoid dnd of js URIs,

Review commit: https://reviewboard.mozilla.org/r/93504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93504/

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8811390 [details]
Bug 1229426 - avoid dnd of js URIs,

https://reviewboard.mozilla.org/r/93504/#review93948

It's unfortunate we allow to drop data: here, otherwise we could just pass disallowInherit to dropLinks and it would automatically cancel the event and throw... It would be much simpler.

::: browser/base/content/urlbarBindings.xml:729
(Diff revision 1)
>              let url = links[0].url;
>              aEvent.preventDefault();
> +            let strippedURL = stripUnsafeProtocolOnPaste(url);
> +            if (strippedURL != url) {
> +              // Unfortunately we're not allowed to set the bits being pasted
> +              // so cancel this event:

This comment is confusing, what are "the bits", and what has pasting to do with a drop? The comment should not assume that one has to check what the other consumers do... Please rewrite it specifically for drop.

The possibilities here are 2:
1. drop the url without javascript. I don't se the point in doing this, the cut url would make no sense.
2. do nothing, that is what this patch is doing.

But honestly we could handle this better from a UI feedback point of view, in onDragOver we could check dropLinks and if the url is a javascript url, we could set aEvent.dataTransfer.dropEffect = "none", so the user gets a no-drop indicator. Not doing this means the user drops the link and nothing happens, that is not great.
The code can be refactored to go through a common method in both dragover and drop.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8811390 [details]
Bug 1229426 - avoid dnd of js URIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93504/diff/1-2/

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8811390 [details]
Bug 1229426 - avoid dnd of js URIs,

https://reviewboard.mozilla.org/r/93504/#review95522

::: browser/base/content/urlbarBindings.xml:716
(Diff revision 2)
>              aEvent.preventDefault();
> -            this.value = url;
> -            SetPageProxyState("invalid");
> -            this.focus();
> +            let url = links[0].url;
> +            let strippedURL = stripUnsafeProtocolOnPaste(url);
> +            if (strippedURL != url) {
> +              aEvent.stopImmediatePropagation();
> +              aEvent.dataTransfer.dropEffect = "none";

I think it should actually be set in all the cases where we return null.. I'd indeed move this to onDragOver as:
if (!this._getDroppableLink(aEvent)) {
  aEvent.dataTransfer.dropEffect = "none";
}
that would make it clearer what we do (just invoking this._getDroppableLink sounds like a no-op without looking into its implementation)

In the drop case you should not need it cause it will take the last value set by dragover.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8811390 [details]
Bug 1229426 - avoid dnd of js URIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93504/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37a877e840fc
avoid dnd of js URIs, r=mak

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37a877e840fc

Comment 11

[Andrei Vaida [:avaida]]

Gijs, does this have automated coverage? Should manual QA be looking at it?

Comment 12

[:Gijs (he/him)]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #11)
> Gijs, does this have automated coverage? Should manual QA be looking at it?

This got automated coverage in bug 1320502, and there were a few regressions reported and fixed already, so I guess it being high profile and having automated coverage mean additional QA is unlikely to uncover new things. Thanks for checking!

Comment 13

[Andrei Vaida [:avaida]]

(In reply to :Gijs from comment #12)
> (In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #11)
> > Gijs, does this have automated coverage? Should manual QA be looking at it?
> 
> This got automated coverage in bug 1320502, and there were a few regressions
> reported and fixed already, so I guess it being high profile and having
> automated coverage mean additional QA is unlikely to uncover new things.
> Thanks for checking!

Thank you for following up, Gijs!