329385 - Attacker can force mouse drag

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Paul Nickerson]

(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]

Attachment: 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]]

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 [:bzbarsky]]

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]

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]

should we try to do something by 1.9b1?

Comment 6

[Jesse Ruderman]

Dup of bug 251226?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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]

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)]

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)]

Attachment: Flip the pref and remove the UI.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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)]

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!)]

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]

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!)]

(In reply to comment #15)

Done, bug 412862.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

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]

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]

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]

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!)]

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!)]

Now fixed as bug 412862 has landed.

Comment 24

[Jesse Ruderman]

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]

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!)]

-> default owner

Comment 27

[Daniel Veditz [:dveditz]]

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]]

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]

Jst said you wanted this one

Comment 30

[Peter Van der Beken [:peterv]]

(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)]

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]

We would also need to prevent the move/resize from initiating a drag.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

I can take this, as a P2.

Comment 34

[Jesse Ruderman]

Renominating for reasons explained in bug 412862 comment 43.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase (slightly modified)

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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][[e-mail]]]

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)]

Comment on attachment 310342 [details] [diff] [review]
drag suppressor

Yeah, that looks good to me. r=jst

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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)]

Comment on attachment 310350 [details] [diff] [review]
v2

Yeah, let's give this a go.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Marking this fixed. Will test this tomorrow on windows too (once I get a nightly build).

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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][[e-mail]]]

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]]

How easily would this fix port to the 1.8 branch?

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Attachment: 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][[e-mail]]]

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]]

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][[e-mail]]]

Um, I should find someone to test the branch patch on OSX and Windows.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

At least compiled ok on 1.8 Windows/Mac/Linux

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

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]

Attachment: for 1.8.0 (just context changes in backport)

Comment 54

[Alexander Sack]

a=asac for 1.8.0.15

Comment 55

[Al Billings [:abillings - ex-MoCo]]

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][[e-mail]]]

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 - ex-MoCo]]

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][[e-mail]]]

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 - ex-MoCo]]

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][[e-mail]]]

After the window has moved, continue dragging...

Comment 61

[Al Billings [:abillings - ex-MoCo]]

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.