Attacker can force mouse drag

VERIFIED FIXED

Status

()

Core
DOM: Events
--
critical
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: Paul Nickerson, Assigned: smaug)

Tracking

(Depends on: 1 bug, {verified1.8.1.17})

1.8 Branch
x86
Windows XP
verified1.8.1.17
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.17 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low?])

Attachments

(8 attachments)

(Reporter)

Description

12 years ago
(This is the same as Liu Die Yu's hijackclick vulnerability for Internet Explorer)

Instead of the normal process of holding the mouse button down and dragging an item to initiate the dragging event, one can simply move the window when the mouse is down instead. This can create a wide range of potential vulnerabilities, such as forcing the user to download a file.

Note: I'm pretty sure I tried this a while ago in the 1.0 branch, and I don't think it worked. Would someone try this for me to see if I'm wrong?
(Reporter)

Comment 1

12 years ago
Created attachment 214051 [details]
Testcase

The attached testcase adds about:config to the user's quick links bar. Because it uses fixed values for positioning, the test case will not work if multiple tabs are open, since the tab bar will be in the way.
testcase also requires a default value for dom.disable_window_move_resize (false). Advanced users who disable this common annoyance are safe (it's available through the Options dialog so doesn't have to be _too_ advanced).

I don't think you could force a file upload since we don't allow text drops on a file input field. You could force drag a .xpi URL to the location bar, which would work around site whitelisting (but not the confirmation dialog). You could set a new homepage by dragging a link to the home icon, though I guess that has a confirmation dialog too.

I'm sure Jesse's imagination can come up with more powerful attacks that that.

Confirmed that this is a regression from 1.0, which neither goes into a drag state nor performs a click on the user's mouseup after the window move.
Assignee: nobody → jst
Component: General → DOM: Events
Keywords: regression
Product: Firefox → Core
Whiteboard: [sg:low?]
Version: 1.5.0.x Branch → 1.8 Branch
imo we should simply disable window move by default, quite apart from anything else we do with this bug.  Otherwise, it's easy to affect where drops happen in cases when the user _does_ want to drag.
(Reporter)

Comment 4

12 years ago
Other than going against what other browsers do, disabling window moving would not be a drastic loss. Perhaps it could be enabled on a click event, like the window.open function, so that web apps who may use it still can.

Comment 5

12 years ago
should we try to do something by 1.9b1?
Flags: blocking1.9a1?

Comment 6

12 years ago
Dup of bug 251226?
(Reporter)

Updated

11 years ago
Duplicate of this bug: 376629

Updated

10 years ago
Flags: blocking1.9?
Dveditz: could we simply do what comment 3 says? That would be a simple one-line change right? The attacks described here seems like a good enough reason.
Assignee: jst → dveditz
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
The attack described in comment 3 doesn't seem like a regression since it's more or less by design.

I'll take this and write up a patch though.
Assignee: dveditz → jonas
Depends on: 186708
I'm going to go ahead and remove the regression keyword here as whether this worked in Firefox 1.0 or not we didn't break some existing protections to get to where we are, but rather we fixed other bugs along the way that enabled new functionality etc. And that's all assuming that this wasn't a problem is 1.0, and I *really* doubt this wasn't a problem there as well, even if the attack might need to look a bit different to work there.

As for defaulting the pref that controls whether or not a window can be moved from JS or not, if we do that, I think we should take out the UI for the pref too. If we don't, we need at least wording around the UI that warns users about the potential vulnerability they're exposing themselves to by changing the value of the pref

I've got a patch that flips this pref and removes the UI for it if we want to go that route here. 
Keywords: regression
Created attachment 294795 [details] [diff] [review]
Flip the pref and remove the UI.
Attachment #294795 - Flags: superreview?(dveditz)
Attachment #294795 - Flags: review?(jonas)
Comment on attachment 294795 [details] [diff] [review]
Flip the pref and remove the UI.

I think we should also add code to allow you to use a site-specific pref, but lets get this landed first to get critical aspect fixed and get feedback on if too many sites break
Attachment #294795 - Flags: review?(jonas) → review+
QA Contact: general → events
Assignee: jonas → jst
Upping this to P1 (per discussion in yesterdays Gecko/Firefox meeting), and reassigning to Ben Turner who agreed to work on adding support for controlling this with site specific prefs.
Assignee: jst → bent.mozilla
Priority: P3 → P1
Update: beltzner and mconnor both want to use the existing permission manager dialog and backend for this rather than the site-specific content pref service.

Comment 15

10 years ago
Please work on the patch in a public bug report (such as bug 186708) instead of this hidden bug report.
Depends on: 412862
(In reply to comment #15)

Done, bug 412862.

The issue, as I understand it, is that it's possible to move a window 
onMouseDown, which turns an innocent mouseclick into a less innocent 
mousedrag and could be used to copy a js snippet into the location bar and 
run it with privs. Badtimes!

The proposed solution is to remove the pref default that allows scripts to 
move and resize windows, blanket denying the action. This has the advantage 
of also eliminating a lot of web nuisances that are out there.

The solution seems ovely severe to me, though, especially without adding an 
easy way to tell that the action has been blocked and to allow it for the 
site. I have a few counterproposals that I'd like us to consider, in order 
of my preference:

To address the attack:
A1: don't let these methods work onMouseDown, but rather only onMouseUp or 
onClick (which should only fire onMouseUp)
A2: detect when a window move or resize is called on mouseDown and don't 
accept drag events until you are a mouseUp.

To address annoyance:
B1: make pref work like popup blocking, with a global value (default off) 
and a notification bar to tell users and ask if they want to let the window 
be moved.
There are certainly a few useful use cases for the feature, especially for resize. However I think in the vast majority of cases it's just annoying to users. Basically I think turning off this by default is something that most users will appreciate even disregarding the security implications.

Just disabling moves during onmousedown etc still leaves the posibility of firing a timer onmousedown and then doing the move when the timer fires. And you could make a drag/drop in one place be a drag/drop in another by moving the window just before the user is about to do the drop.

It's important to note that this is much less severe than popup blocking. The breakage won't ever be that bad. The user can always move or resize the window themselves. (It is no longer possible for content to open windows that can't be resized)

Having a "notification bar" a'la the popup blocker shouldn't hurt though. Except possibly since the user could be tricked into allowing moving for this site and then launch the attack.

Comment 19

10 years ago
If the window is too small, users can figure that out themselves and resize the window.  Showing a notification bar about the blocked resize() call would just make it less likely for the content to fit and add a step for users.
Yep, that is definitely a risk.

I think it would be fine to just have a site specific pref and let the users that really care use that UI. That way intranet sites that gets "broken" can use the UI too without affecting other users.
Keywords: late-l10n
Ok, if we're not going to change the approach then this is just waiting on UI-review (beltzner) in bug 412862.
Now fixed as bug 412862 has landed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Duplicate of this bug: 251226

Comment 24

10 years ago
Reopening.  This isn't as severe now, but I think it still needs a real fix, such as canceling drag-and-drops in progress when windows resize and not allowing drops (from other applications) soon after window resizes.  Not having a real fix for this might limit how easy we can make it for users to allow scripted resizes.

Notification bar discussion is bug 413792.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ah, so the worry is users flipping the pref back (if only for certain sizes) and then being vulnerable again?
-> default owner
Assignee: bent.mozilla → nobody
Status: REOPENED → NEW

Updated

10 years ago
Assignee: nobody → jst
Comment on attachment 294795 [details] [diff] [review]
Flip the pref and remove the UI.

Clearing request, this patch appears to have been superseded by bug 412862.
Attachment #294795 - Flags: superreview?(dveditz)
Comment on attachment 294795 [details] [diff] [review]
Flip the pref and remove the UI.

Although the bug says "1.8 branch", so sr=dveditz for that branch. I don't think this is how Jesse wants it fixed with his re-open, though it would help.
Attachment #294795 - Flags: superreview+
Flags: blocking1.9a1?
Jst said you wanted this one
Assignee: jst → peterv
Keywords: late-l10n
(In reply to comment #24)
> Reopening.  This isn't as severe now

Would it be ok to lower the priority then?
Not sure what to do with this one. There's been a fair amount of pushback on the UI side of this, so we may need to change our minds on that, don't know yet. Either way, this won't be dealt with for beta4, so setting this as a P2 for now.

It would seem like we should be able to fix this by detecting window moves/resizes and simply disabling any pending drag/drop operations.

Smaug, if you have cycles for this and can help, feel free to take the bug.
Assignee: peterv → jst
Priority: P1 → P2
We would also need to prevent the move/resize from initiating a drag.
(Assignee)

Comment 33

10 years ago
I can take this, as a P2.
Assignee: jst → Olli.Pettay

Updated

10 years ago
Flags: wanted1.9.0.x+
Flags: tracking1.9+
Flags: blocking1.9-

Updated

10 years ago
Priority: P2 → --

Comment 34

10 years ago
Renominating for reasons explained in bug 412862 comment 43.
Flags: blocking1.9- → blocking1.9?

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 35

10 years ago
Created attachment 310338 [details]
testcase (slightly modified)
(Assignee)

Comment 36

10 years ago
Created attachment 310342 [details] [diff] [review]
drag suppressor

Jst, what do you think about this. Suppress dragging when moving/resizing.
The next mouseup enables dragging. This should hopefully work well enough.
Tested on linux.
Attachment #310342 - Flags: review?(jst)
(Assignee)

Comment 37

10 years ago
Hmm, I think I could move enable/disable to nsIDragService. I thought there 
would be problems if chrome or extensions would use the methods but I should
have solution for that now... New patch coming.
Comment on attachment 310342 [details] [diff] [review]
drag suppressor

Yeah, that looks good to me. r=jst
Attachment #310342 - Flags: review?(jst) → review+
(Assignee)

Comment 39

10 years ago
Created attachment 310350 [details] [diff] [review]
v2

This is without the extra interface. suppress/unsuppress should be ok enough.
If some extension wants to disable dragging, calling suppress, but never 
unsuppress should work.
Would be great if someone could test this on mac/windows.
Attachment #310350 - Flags: superreview?(jst)
Attachment #310350 - Flags: review?(jst)
Comment on attachment 310350 [details] [diff] [review]
v2

Yeah, let's give this a go.
Attachment #310350 - Flags: superreview?(jst)
Attachment #310350 - Flags: superreview+
Attachment #310350 - Flags: review?(jst)
Attachment #310350 - Flags: review+
(Assignee)

Comment 41

10 years ago
Marking this fixed. Will test this tomorrow on windows too (once I get a nightly build).
Status: NEW → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 42

10 years ago
Created attachment 310475 [details] [diff] [review]
additional fix

This makes drag suppression work properly also when invokeDragSession is used.
When something on content is dragged, invokeDragSessionWithImage or 
invokeDragSessionWithSelection is used, but page info uses invokeDragSession.
Attachment #310475 - Flags: superreview?(jst)
Attachment #310475 - Flags: review?(jst)
(Assignee)

Updated

10 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

10 years ago
Attachment #310475 - Flags: superreview?(jst)
Attachment #310475 - Flags: superreview+
Attachment #310475 - Flags: review?(jst)
Attachment #310475 - Flags: review+
(Assignee)

Updated

10 years ago
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 43

10 years ago
Tested on Windows too and seems to work fine there.

Safari 3.1 has this same bug. Opera doesn't, at least not with the default settings.

Updated

10 years ago
Depends on: 427143
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.17?
How easily would this fix port to the 1.8 branch?
Flags: blocking1.8.1.17? → blocking1.8.1.17+
(Assignee)

Comment 45

9 years ago
Should be doable. I'll try to make the 1.8 patch within next few days.
I haven't tested the very latest version of Safari, but it may still have this
bug, so not sure if we can mention this in the list of fixed security related bugs or whether we can open this bug. And no idea if IE still has this bug.
(Assignee)

Comment 46

9 years ago
Created attachment 330552 [details] [diff] [review]
for 1.8

Includes both patches and the regression fix.
The code is a bit uglier in 1.8, for example there are several QI implementations
and had to put MaybeSuppressDrag to all places where size/placement of a window
can be changed in globalwindow.
Tested on Linux. Have to either land to branch and test then or
if someone could test the patch on windows and mac.
Attachment #330552 - Flags: superreview?(jst)
Attachment #330552 - Flags: review?(jst)

Updated

9 years ago
Attachment #330552 - Flags: superreview?(jst)
Attachment #330552 - Flags: superreview+
Attachment #330552 - Flags: review?(jst)
Attachment #330552 - Flags: review+
(Assignee)

Comment 47

9 years ago
Comment on attachment 330552 [details] [diff] [review]
for 1.8

Asking approval, but would be great if I find someone to test this on Windows and OSX.
Attachment #330552 - Flags: approval1.8.1.17?
Comment on attachment 330552 [details] [diff] [review]
for 1.8

Approved for 1.8.1.17, a=dveditz for release-drivers
Attachment #330552 - Flags: approval1.8.1.17? → approval1.8.1.17+
Whiteboard: [sg:low?] → [sg:low?] needs 1.8-branch landing
(Assignee)

Comment 49

9 years ago
Um, I should find someone to test the branch patch on OSX and Windows.
(Assignee)

Comment 50

9 years ago
Checked in. Now waiting to see if the patch compiles ok on Windows and Mac and
then I'll test Windows build.
(Assignee)

Comment 51

9 years ago
At least compiled ok on 1.8 Windows/Mac/Linux
Keywords: fixed1.8.1.17
Whiteboard: [sg:low?] needs 1.8-branch landing → [sg:low?]
(Assignee)

Comment 52

9 years ago
Tested latest Windows 1.8-nightly; this bug is fixed - using the testcase 2
the dragging is stopped immediately when window moves.

Comment 53

9 years ago
Created attachment 336239 [details] [diff] [review]
for 1.8.0 (just context changes in backport)
Attachment #336239 - Flags: approval1.8.0.15+

Comment 54

9 years ago
a=asac for 1.8.0.15

Updated

9 years ago
Flags: blocking1.8.0.15+
The test cases still repro the bug in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/2008082909 Firefox/2.0.0.17. I see no difference in behavior at all.
(Assignee)

Comment 56

9 years ago
Really? I have to retest this asap. The difference should be that whether
or not releasing mouse drops something to desktop (if desktop is under the browser).
Running both test cases with Firefox 2.0.0.16 and Firefox 2.0.0.17 build 2 on Windows XP, I see the EXACT same behavior. Neither drops anything to the desktop though.
(Assignee)

Comment 58

9 years ago
Well, hmm, perhaps it depends on your settings a bit, but if you manually continue dragging and then drop, does .16 drop something?
Manually continue what dragging? I'm clicking on the link within the test case. When I click on "Click me", the window moves itself, whether I hold the mouse down or simply single click.
(Assignee)

Comment 60

9 years ago
After the window has moved, continue dragging...
Ah, sorry. That makes sense now. Verified for 1.8.1.17 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/2008082909 Firefox/2.0.0.17.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.17 → verified1.8.1.17
Group: core-security
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.