307989 - Crash [@ GetWrapperFor] [@ nsIView::GetViewFor] dragging selection within evil changing document

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P1)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050910
Firefox/1.6a1

Steps to reproduce:
1. Open the very evil testcase.
2. Select some text and start dragging it. (You can use the keyboard to select
all before dragging with the mouse.)
3. Wait until the page changes.
4. Drop.

Result: More often than not, the browser crashes [@ nsIView::GetViewFor] [@
GetWrapperFor].  The top of the stack is a random address.  I'm pretty sure this
is exploitable.

TB9234593K, TB9234571Z

Please keep this security-sensitive until a Firefox release with this fix has
gone out and both bug 306663 and bug 306939 have become public.

Comment 1

[Jesse Ruderman]

Attachment: very evil testcase

Comment 2

[Jesse Ruderman]

I can't get it to crash on Windows.  On Windows, the script is paused until I
drop what I'm dragging.

Comment 3

[Asa Dotzler [:asa]]

ROC, can you help us look into these crashes?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can't get it to crash on Linux.

Comment 5

[Mike Schroepfer]

Pulling off 1.5b2 list for now.  Mac-only, requires a specific set of steps to
cause.   Putting on Josh A. radar to see if there is an easy fix

Comment 6

[Jesse Ruderman]

Still crashes using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
rv:1.9a1) Gecko/20051018 Firefox/1.6a1.  TB10831987H

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem here is that the nsMouseEvent cached in the nsSelection object is pointing to a stale widget.  We should probably store some information other than an nsMouseEvent in the *DelayedCaretData functions, i.e., store something like what we do with the result in the one user of GetDelayedCaretData.  (There are multiple implementations, since one forwards to the other.)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So this code is called only when you click in a selection and don't move far enough for the movement to be considered a drag.

What we currently do is use the coordinates of the click start to call GetContentAndOffsetsFromPoint in the frame in which the click ends.  (You can move a few pixels without starting a drag, enough to get to a different line in a textarea.)

We could fix this by doing the GetContentAndOffsetsFromPoint call up front and thus being consistent about using the start coordinates of the click.  But that seems silly, since one usually uses the end coordinates of a click.  The thing is, we normally do do selection for movements that aren't large enough to be considered a drag, so there really isn't a normal behavior for where to place a caret when the start and end of the click are at two different points -- normally you'd end up with a selection.  But I think it seems like it would be fine to just use the end coordinate of the click, which removes the need for storing any of this information at all.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And thanks to Sicking for helping debug this.)

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 206040 [details] [diff] [review]
patch

I wonder if we really need the delayed caret data at all.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk.

Comment 13

[Mike Schroepfer]

Let's bake a couple more days on the trunk and then consider for 1.8.0.1.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, when I tried to merge the nsFrame.cpp changes, I discovered that the code is different on the branch -- I think different enough that the crash bug is not present and this fix won't actually do anything useful.  I want to double-check this a little more carefully, later, though.

Comment 15

[Daniel Veditz [:dveditz]]

I'm not seeing a crash on the 1.8 branch -- can someone else confirm that?  If it's not crashing this doesn't need to block 1.8.0.x

Comment 16

[Daniel Veditz [:dveditz]]

May not happen on 1.8 branch, slip nomination to 1.8.0.2 for reconsideration whether we need this or not.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we should take just the nsSelection.cpp half of the patch on the branches -- doing so would turn any crash that exists (but I don't think it does) from something potentially exploitable into a null dereference.

I don't see a need to attach a new patch for that, but if somebody thinks I need to, let me know.

Comment 18

[Daniel Veditz [:dveditz]]

Can't really a= the wrong patch, but a=dveditz on 1.8.0.2 for the proposed smaller patch

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsSelection.cpp change checked in to MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH

Comment 20

[Tim Riley [:timr]]

Does this still need qawanted?  It looks like comment #15 wanted QA on the 1.8.  But we already have a fix on 1.8.1 branch so it looks like we past that now.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Only half the fix was landed on the branches; that fix would either (a) have no effects or (b) turn a rare potentially exploitable crash into a slightly more common null dereference crash.  I think it's probably safe to assume it's (a) unless somebody reports the crash, although some testing that that's actually the case could be useful.

Comment 22

[Tim Riley [:timr]]

davel or bclary- can you look at dbaron's comment 21 an provide some testing there?

Comment 23

[Dave Liebreich [:davel]]

I've looked at the code, and talked briefly to dbaron, and I'm not sure that there is a good way to do directed testing regarding the patch that landed on the branch.

If we're concerned about this fix, then I one sugestion I have is that we instrument the code (perhaps by setting the widget pointer to a known bad address rather than nsnull), then do a manual mouse-event torture-test with caret-browsing on and off.

Do we have trace or static analysis tools that can figure out the lifecycle (possible or actual) of event objects?  That kind of thing would help direct our testing.

Comment 24

[Dave Liebreich [:davel]]

removing qawanted keyword - we'll verify using the test case and poking around if we have the opportunity

Comment 25

[Marcia Knous [:marcia]]

verified on 1.8.0.2 using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060301 Firefox/1.5.0.1. Unable to crashy despite repeated manipulations of the evil test case. Hopefully it isn't poised to take over the world. Adding relevant keyword.

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 206040 [details] [diff] [review]
patch

The nsSelection.cpp part of this patch was the only part checked in elsewhere, and it still applies cleanly to the older branch.  It would be good to take for the reasons cited in comment 17.

Comment 27

[Tim Riley [:timr]]

Comment on attachment 206040 [details] [diff] [review]
patch

We are already past code freeze and the general testing is done. "-" for 1.0.8.  "?" for 1.0.9.

Comment 28

[Marcia Knous [:marcia]]

verified on 1.8.1 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b1) Gecko/20060810 BonEcho/2.0b1. Unable to crash despite
repeated manipulations of the evil test case. Adding keyword.