Closed Bug 1229426 (CVE-2017-5458) Opened 8 years ago Closed 7 years ago

"javascript:" scheme not removed for drag and drop, unlike pasting into the URL bar

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox-esr45 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- fixed

People

(Reporter: dveditz, Assigned: Gijs)

References

Details

(Keywords: sec-low, Whiteboard: [2016-GBT-Y][adv-main53+])

Attachments

(1 file)

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.
Keywords: sec-wantsec-low
This can be seen in attachment 8693540 [details] from bug 1228997
Priority: -- → P2
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.
(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.
Blocks: 1018154
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
Attachment #8811390 - Flags: review?(mak77)
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.
Attachment #8811390 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/37a877e840fc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1320502
Depends on: 1324410
Depends on: 1345813
Gijs, does this have automated coverage? Should manual QA be looking at it?
Flags: qe-verify?
Flags: needinfo?(gijskruitbosch+bugs)
(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!
Flags: needinfo?(gijskruitbosch+bugs) → in-testsuite+
(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!
Flags: qe-verify? → qe-verify-
Whiteboard: [2016-GBT-Y] → [2016-GBT-Y][adv-main53+]
Alias: CVE-2017-5458
You need to log in before you can comment on or make changes to this bug.