Closed Bug 329385 Opened 19 years ago Closed 17 years ago

Attacker can force mouse drag

Categories

(Core :: DOM: Events, defect)

1.8 Branch
x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: pvnick, Assigned: smaug)

References

Details

(Keywords: verified1.8.1.17, Whiteboard: [sg:low?])

Attachments

(8 files)

(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?
Attached file 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.
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.
should we try to do something by 1.9b1?
Flags: blocking1.9a1?
Dup of bug 251226?
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+
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
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
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
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.
Please work on the patch in a public bug report (such as bug 186708) instead of this hidden bug report.
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.
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.
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
Closed: 17 years ago
Resolution: --- → FIXED
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
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
(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.
I can take this, as a P2.
Assignee: jst → Olli.Pettay
Flags: wanted1.9.0.x+
Flags: tracking1.9+
Flags: blocking1.9-
Priority: P2 → --
Renominating for reasons explained in bug 412862 comment 43.
Flags: blocking1.9- → blocking1.9?
Flags: blocking1.9? → blocking1.9+
Attached patch drag suppressorSplinter Review
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)
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+
Attached patch v2Splinter Review
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+
Marking this fixed. Will test this tomorrow on windows too (once I get a nightly build).
Status: NEW → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Attached patch additional fixSplinter Review
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #310475 - Flags: superreview?(jst)
Attachment #310475 - Flags: superreview+
Attachment #310475 - Flags: review?(jst)
Attachment #310475 - Flags: review+
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
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.
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+
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.
Attached patch for 1.8Splinter Review
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)
Attachment #330552 - Flags: superreview?(jst)
Attachment #330552 - Flags: superreview+
Attachment #330552 - Flags: review?(jst)
Attachment #330552 - Flags: review+
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
Um, I should find someone to test the branch patch on OSX and Windows.
Checked in. Now waiting to see if the patch compiles ok on Windows and Mac and then I'll test Windows build.
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?]
Tested latest Windows 1.8-nightly; this bug is fixed - using the testcase 2 the dragging is stopped immediately when window moves.
a=asac for 1.8.0.15
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.
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.
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.
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
Group: core-security
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: