Last Comment Bug 329385 - Attacker can force mouse drag
: Attacker can force mouse drag
Status: VERIFIED FIXED
[sg:low?]
: verified1.8.1.17
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: 1.8 Branch
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug] (reviewing overload)
:
:
Mentors:
: 251226 376629 (view as bug list)
Depends on: 186708 412862 427143
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-04 18:33 PST by Paul Nickerson
Modified: 2008-10-22 22:42 PDT (History)
18 users (show)
jst: blocking1.9+
dveditz: blocking1.8.1.17+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (343 bytes, text/html)
2006-03-04 18:36 PST, Paul Nickerson
no flags Details
Flip the pref and remove the UI. (4.56 KB, patch)
2007-12-28 16:58 PST, Johnny Stenback (:jst, jst@mozilla.com)
jonas: review+
dveditz: superreview+
Details | Diff | Splinter Review
testcase (slightly modified) (415 bytes, text/html)
2008-03-18 14:17 PDT, Olli Pettay [:smaug] (reviewing overload)
no flags Details
drag suppressor (14.93 KB, patch)
2008-03-18 14:28 PDT, Olli Pettay [:smaug] (reviewing overload)
jst: review+
Details | Diff | Splinter Review
v2 (11.13 KB, patch)
2008-03-18 14:55 PDT, Olli Pettay [:smaug] (reviewing overload)
jst: review+
jst: superreview+
Details | Diff | Splinter Review
additional fix (10.21 KB, patch)
2008-03-19 08:11 PDT, Olli Pettay [:smaug] (reviewing overload)
jst: review+
jst: superreview+
Details | Diff | Splinter Review
for 1.8 (37.28 KB, patch)
2008-07-21 04:10 PDT, Olli Pettay [:smaug] (reviewing overload)
jst: review+
jst: superreview+
dveditz: approval1.8.1.17+
Details | Diff | Splinter Review
for 1.8.0 (just context changes in backport) (35.31 KB, patch)
2008-08-31 05:51 PDT, Alexander Sack
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Paul Nickerson 2006-03-04 18:33:53 PST
(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?
Comment 1 Paul Nickerson 2006-03-04 18:36:54 PST
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.
Comment 2 Daniel Veditz [:dveditz] 2006-03-04 21:53:53 PST
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.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2006-03-05 10:35:38 PST
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.
Comment 4 Paul Nickerson 2006-03-05 19:56:33 PST
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 chris hofmann 2006-04-28 14:39:14 PDT
should we try to do something by 1.9b1?
Comment 6 Jesse Ruderman 2006-04-28 16:32:17 PDT
Dup of bug 251226?
Comment 7 Paul Nickerson 2007-04-05 11:54:13 PDT
*** Bug 376629 has been marked as a duplicate of this bug. ***
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-10-31 17:08:31 PDT
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.
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-12-12 22:16:48 PST
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.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2007-12-28 16:55:58 PST
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. 
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2007-12-28 16:58:21 PST
Created attachment 294795 [details] [diff] [review]
Flip the pref and remove the UI.
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-12-31 17:11:42 PST
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
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2008-01-16 16:40:03 PST
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.
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2008-01-17 13:40:06 PST
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 Jesse Ruderman 2008-01-17 13:58:55 PST
Please work on the patch in a public bug report (such as bug 186708) instead of this hidden bug report.
Comment 16 Ben Turner (not reading bugmail, use the needinfo flag!) 2008-01-17 15:23:18 PST
(In reply to comment #15)

Done, bug 412862.

Comment 17 Mike Beltzner [:beltzner, not reading bugmail] 2008-01-25 18:01:01 PST
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.
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-01-26 05:16:55 PST
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 Jesse Ruderman 2008-01-26 16:20:16 PST
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.
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-01-26 19:46:30 PST
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.
Comment 21 Ben Turner (not reading bugmail, use the needinfo flag!) 2008-01-28 12:38:22 PST
Ok, if we're not going to change the approach then this is just waiting on UI-review (beltzner) in bug 412862.
Comment 22 Ben Turner (not reading bugmail, use the needinfo flag!) 2008-01-29 18:29:17 PST
Now fixed as bug 412862 has landed.
Comment 23 Jesse Ruderman 2008-01-30 18:06:44 PST
*** Bug 251226 has been marked as a duplicate of this bug. ***
Comment 24 Jesse Ruderman 2008-02-08 01:59:06 PST
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.
Comment 25 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-02-08 02:10:38 PST
Ah, so the worry is users flipping the pref back (if only for certain sizes) and then being vulnerable again?
Comment 26 Ben Turner (not reading bugmail, use the needinfo flag!) 2008-02-08 15:10:22 PST
-> default owner
Comment 27 Daniel Veditz [:dveditz] 2008-02-13 18:30:17 PST
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.
Comment 28 Daniel Veditz [:dveditz] 2008-02-13 18:34:43 PST
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.
Comment 29 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-02-20 10:36:00 PST
Jst said you wanted this one
Comment 30 Peter Van der Beken [:peterv] 2008-02-24 02:58:21 PST
(In reply to comment #24)
> Reopening.  This isn't as severe now

Would it be ok to lower the priority then?
Comment 31 Johnny Stenback (:jst, jst@mozilla.com) 2008-02-25 09:06:42 PST
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.
Comment 32 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-02-26 00:15:07 PST
We would also need to prevent the move/resize from initiating a drag.
Comment 33 Olli Pettay [:smaug] (reviewing overload) 2008-02-26 03:40:31 PST
I can take this, as a P2.
Comment 34 Jesse Ruderman 2008-03-16 15:36:21 PDT
Renominating for reasons explained in bug 412862 comment 43.
Comment 35 Olli Pettay [:smaug] (reviewing overload) 2008-03-18 14:17:55 PDT
Created attachment 310338 [details]
testcase (slightly modified)
Comment 36 Olli Pettay [:smaug] (reviewing overload) 2008-03-18 14:28:17 PDT
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.
Comment 37 Olli Pettay [:smaug] (reviewing overload) 2008-03-18 14:32:42 PDT
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 38 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-18 14:36:24 PDT
Comment on attachment 310342 [details] [diff] [review]
drag suppressor

Yeah, that looks good to me. r=jst
Comment 39 Olli Pettay [:smaug] (reviewing overload) 2008-03-18 14:55:35 PDT
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.
Comment 40 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-18 16:01:34 PDT
Comment on attachment 310350 [details] [diff] [review]
v2

Yeah, let's give this a go.
Comment 41 Olli Pettay [:smaug] (reviewing overload) 2008-03-18 17:49:03 PDT
Marking this fixed. Will test this tomorrow on windows too (once I get a nightly build).
Comment 42 Olli Pettay [:smaug] (reviewing overload) 2008-03-19 08:11:39 PDT
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.
Comment 43 Olli Pettay [:smaug] (reviewing overload) 2008-03-20 05:29:08 PDT
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.
Comment 44 Daniel Veditz [:dveditz] 2008-07-18 11:32:14 PDT
How easily would this fix port to the 1.8 branch?
Comment 45 Olli Pettay [:smaug] (reviewing overload) 2008-07-18 12:44:56 PDT
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.
Comment 46 Olli Pettay [:smaug] (reviewing overload) 2008-07-21 04:10:21 PDT
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.
Comment 47 Olli Pettay [:smaug] (reviewing overload) 2008-07-23 02:48:51 PDT
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.
Comment 48 Daniel Veditz [:dveditz] 2008-08-04 11:15:54 PDT
Comment on attachment 330552 [details] [diff] [review]
for 1.8

Approved for 1.8.1.17, a=dveditz for release-drivers
Comment 49 Olli Pettay [:smaug] (reviewing overload) 2008-08-11 11:35:20 PDT
Um, I should find someone to test the branch patch on OSX and Windows.
Comment 50 Olli Pettay [:smaug] (reviewing overload) 2008-08-13 05:44:03 PDT
Checked in. Now waiting to see if the patch compiles ok on Windows and Mac and
then I'll test Windows build.
Comment 51 Olli Pettay [:smaug] (reviewing overload) 2008-08-13 06:42:07 PDT
At least compiled ok on 1.8 Windows/Mac/Linux
Comment 52 Olli Pettay [:smaug] (reviewing overload) 2008-08-14 05:34:59 PDT
Tested latest Windows 1.8-nightly; this bug is fixed - using the testcase 2
the dragging is stopped immediately when window moves.
Comment 53 Alexander Sack 2008-08-31 05:51:10 PDT
Created attachment 336239 [details] [diff] [review]
for 1.8.0 (just context changes in backport)
Comment 54 Alexander Sack 2008-08-31 05:52:20 PDT
a=asac for 1.8.0.15
Comment 55 Al Billings [:abillings] 2008-09-02 16:50:10 PDT
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.
Comment 56 Olli Pettay [:smaug] (reviewing overload) 2008-09-02 17:02:40 PDT
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).
Comment 57 Al Billings [:abillings] 2008-09-02 17:06:11 PDT
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.
Comment 58 Olli Pettay [:smaug] (reviewing overload) 2008-09-02 17:51:20 PDT
Well, hmm, perhaps it depends on your settings a bit, but if you manually continue dragging and then drop, does .16 drop something?
Comment 59 Al Billings [:abillings] 2008-09-02 17:57:48 PDT
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.
Comment 60 Olli Pettay [:smaug] (reviewing overload) 2008-09-02 18:04:31 PDT
After the window has moved, continue dragging...
Comment 61 Al Billings [:abillings] 2008-09-02 18:11:20 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.