Bug 1229426 (CVE-2017-5458)

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

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Location Bar
P2
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: dveditz, Assigned: Gijs)

Tracking

(Depends on: 1 bug, {sec-low})

unspecified
Firefox 53
sec-low
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox52 wontfix, firefox53 fixed, firefox-esr45 wontfix, firefox-esr52 wontfix)

Details

(Whiteboard: [2016-GBT-Y][adv-main53+])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

58 bytes, text/x-review-board-request
mak
: review+
Details | Review
(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Keywords: sec-want → sec-low
(Reporter)

Comment 1

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

Updated

2 years ago
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.
Whiteboard: [2016-GBT-Y]
(Assignee)

Comment 3

9 months ago
(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.
(Assignee)

Updated

8 months ago
Blocks: 1018154
(Assignee)

Updated

7 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 5

7 months ago
mozreview-review
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 hidden (mozreview-request)

Comment 7

7 months ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 9

7 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/37a877e840fc
avoid dnd of js URIs, r=mak

Comment 10

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37a877e840fc
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

7 months ago
Depends on: 1320502

Updated

6 months ago
Depends on: 1324410

Updated

4 months ago
Depends on: 1345813
Gijs, does this have automated coverage? Should manual QA be looking at it?
Flags: qe-verify?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 12

3 months ago
(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-
status-firefox52: --- → wontfix
status-firefox-esr45: --- → wontfix
status-firefox-esr52: --- → wontfix
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.