Closed Bug 995603 (CVE-2014-1539) Opened 10 years ago Closed 10 years ago

Cursor can be totally invisible using flash object and div

Categories

(Core Graveyard :: Plug-ins, defect)

28 Branch
x86
macOS
defect
Not set
normal

Tracking

(firefox29 wontfix, firefox30 verified, firefox31 verified, firefox32 verified, firefox-esr2430+ verified, b2g-v1.2 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, seamonkey2.26 wontfix)

VERIFIED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified
firefox-esr24 30+ verified
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
seamonkey2.26 --- wontfix

People

(Reporter: jordi.chancel, Assigned: smichaud)

References

()

Details

(Keywords: sec-high, testcase, Whiteboard: [description in comment #17][adv-main30+])

Attachments

(2 files, 4 obsolete files)

Attached file test INVISIBLE MOUSE.zip (obsolete) —
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

firstly i would to say that it's not a flash issue and it's works only for firefox for mac os x (don't works on WINDOWS , don't works on Google Chrome , don't works on Opera, don't works on safari).
when you go on a flash object that defined the cursor like unvisible and you go on a <div> with the cursor , the cursor is totaly unvisible (don't works on WINDOWS , don't works on Google Chrome , don't works on Opera, don't works on safari).
This can lead to clickjacking , cursorjacking , spoofing or others...

steps:
1 : go to the flash object (in center into the flash object) 
2 : wait 2 s 
3 : go with your cursor down (div t2)
4 : Cursor are cursor-jacked 

(view this youtube video for more details => https://www.youtube.com/watch?v=5Lu8pcWvUVY&feature=youtu.be  )


Actual results:

Cursor are totaly unsibile , when you try to install a xpi , the cursor is totally unvisible.


Expected results:

this can be used for clickjacking / cursorjacking  / spoofing or others , 
this issue works only on Firefox for MAC .
Summary: Firefox for MAC OS X - cursor can be totaly unvisible (leading to clickjacking , cursorjacking or others) → Firefox for MAC OS X - cursor can be totaly unvisible using flash object and div (leading to clickjacking , cursorjacking or others)
If you use http: instead of file: then there's an additional dialog to click through:
"Firefox prevented this site from asking you to install..."
I'm trying to understand why the real mouse pointer isn't reset to normal
when it's outside the Flash object, is it the overlapping boxes with high
z-index that prevents that?
with WWebRTC we can remotely activate the microphone and the webcam with this cursorjacking/clickjacking!
The mouse pointer is still visible over the plugin on Linux, on OSX it's hidden.
Adding onmouseout/over callbacks shows that Gecko is aware the mouse has left
the plugin element on both platforms.  I wonder if we could use that to send
an event to the plugin somehow to inform it the mouse pointer is outside?
Component: General → Plug-ins
Keywords: testcase
Summary: Firefox for MAC OS X - cursor can be totaly unvisible using flash object and div (leading to clickjacking , cursorjacking or others) → Firefox for MAC OS X - cursor can be totally invisible using flash object and div (leading to clickjacking , cursorjacking or others)
Playing with this bug's testcase (just moving the mouse around over it) I sometimes see *two* cursors in different positions -- one's the standard OS pointer cursor, and I suspect the other is something created by Flash.

So I wonder if there's confusion about the cursor's coordinates, and if it's not actually invisible but offscreen.

Mats, do you see this?

The STR from comment #0 is very vague and hard to understand.  Jordi, I realize that English isn't your first language.  But I understand some French, so please try re-writing the STR in French.  Make it as detailed and precise as you can.

Once I understand it, I'll re-write your STR in English.
(In reply to Steven Michaud from comment #5)
> Playing with this bug's testcase (just moving the mouse around over it) I
> sometimes see *two* cursors in different positions -- one's the standard OS
> pointer cursor, and I suspect the other is something created by Flash.
> 
> So I wonder if there's confusion about the cursor's coordinates, and if it's
> not actually invisible but offscreen.
> 
> Mats, do you see this?
> 
> The STR from comment #0 is very vague and hard to understand.  Jordi, I
> realize that English isn't your first language.  But I understand some
> French, so please try re-writing the STR in French.  Make it as detailed and
> precise as you can.
> 
> Once I understand it, I'll re-write your STR in English.

quand vous allez sur l'object flash , le curseur devient invisible , et quand vous allez vers le bas ensuite un faux curseur décalé apparait, (le vrai curseur reste tout de même invisible comme a pu le remarquer Mats Palmgren.
ceci peut conduire a des attaque de clickjacking/cursorjacking/spoofing non négligeable!

je ne comprend pas pourquoi chez toi le curseur apparait normalement après que tu l'ais placé sur l'objet flash et déplacé ensuite vers un div..?
Yes, the second cursor is part of the clickjacking trick; it's an abs.pos. image
that's intended to look like the real mouse cursor.  When hiding the real cursor
succeeds, it makes you think that's where the mouse pointer is.  Bug 899992 uses
the same trick but is using cursor:url(transparent-image) to hide the real cursor.
You could use cursor:none too.

I think you can ignore that part, it's just a demonstration that hiding the real
mouse pointer can be used for clickjacking.  The interesting part is that the
plugin fails to reset the cursor to normal when moving the mouse outside it.
Thanks Mats.

I remember seeing (some months ago, testing with my Debug Events Plugin from bug 441880) some cases where we *don't* send a mouse-exit event to the plugin.  If I remember right it had something to do with overlapping objects (as are present in this bug's testcase).  When I get the chance (in the next few days?) I'll test again with it to see if that might be the problem here.
> je ne comprend pas pourquoi chez toi le curseur apparait normalement
> après que tu l'ais placé sur l'objet flash et déplacé ensuite vers
> un div..?

I didn't understand your STR (steps to reproduce), so I didn't use
them.  I just moved (not dragged) the mouse around above the testcase.

I *still* don't really understand them.  Could you please re-write
your steps-to-reproduce, in French and in detail?
I'll take a look. Give me a day or so.
I can see this pretty easily. The cursor simply doesn't reappear after moving first to the SWF (wait 2 seconds) and then to the DIV on the right. At this point, the cursor remains invisible, and indeed one could easily create a fake cursor that could trick the user into clicking something they should not.

I don't know what rating this class of attack gets, but it's clearly a bug.
> The cursor simply doesn't reappear after moving first to the SWF (wait 2 seconds) and then to
> the DIV on the right.

"Move", not "drag"?

Once I know we'll have clear STR, and I'll be able to take a closer look at this bug (replacing the SWF with my Debug Events Plugin).
> I don't know what rating this class of attack gets, but it's clearly a bug.

because if the user don't see where he clicks , this can lead to clickjacking or cursor jacking or other spoofing...

you can view my youtube video in the comment 0!
i go code a testcase that works remotely , in this testcase i will cursorjacked the webrtc for show the webcam maliciously.
(In reply to Steven Michaud from comment #12)
> > The cursor simply doesn't reappear after moving first to the SWF (wait 2 seconds) and then to
> > the DIV on the right.
> 
> "Move", not "drag"?


Move.
(Following up comment #8)

> I remember seeing (some months ago, testing with my Debug Events
> Plugin from bug 441880) some cases where we *don't* send a
> mouse-exit event to the plugin.  If I remember right it had
> something to do with overlapping objects (as are present in this
> bug's testcase).  When I get the chance (in the next few days?) I'll
> test again with it to see if that might be the problem here.

I replaced the Flash plugin with my Debug Events plugin in this bug's
testcase, and found that I'd remembered correctly.

After you move the mouse over either of the two big divs to the top or
left of the plugin and wait 2 seconds, both of them are resized to
partially overlap the plugin.  Once this happens, moving the mouse out
of the plugin to the top or right (where the overlapping occurs)
doesn't cause a NPCocoaEventMouseExited event to be sent to the
plugin.

It looks like the divs are above the plugin in the z-order.  I'll try
to set the plugin above them and see if it makes any difference.
> It looks like the divs are above the plugin in the z-order.  I'll
> try to set the plugin above them and see if it makes any difference.

It does -- in this case the bug doesn't happen.

So the bug here is that plugins don't receive mouse-exit events when
moving the mouse out of the plugin over an object that partially
overlaps it and has a higher z-order.

Even if this bug is Mac-only, I strongly suspect the problem is
somewhere in cross-platform code (not in widget code).  Mats, Matt, do
either of you have any idea what it might be?
Just now I thought of a place in widget code where the problem might be.  I'll look through it later.
I think we can now say that this bug is confirmed.

Even if no actual proof of clickjacking has been presented, and the testcase we have is both overly complex and not airtight, it's pretty clear that *someone* could use the bug I described in comment #17 to perform a cursorjack/clickjack.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [description in comment #17]
I've now discovered that this bug is caused at least partly by a bug in ChildViewMouseTracker::ViewForEvent(), here:
https://hg.mozilla.org/mozilla-central/annotate/c962bde5ac0b/widget/cocoa/nsChildView.mm#l6386

The problem is that when a non-plugin object overlaps a plugin object, -[NSView hitTest:] isn't a good way to find which NSView is under the mouse (or more precisely, which NSView the Gecko object under the mouse belongs to).

I'll be working on this.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Can we try to find a solution where it's not necessary to duplicate Gecko's event targeting logic in widget code? For example, I'd be very happy if ChildViewMouseTracker could just always dispatch the event to the top level ChildView in the NSView hierarchy of the target window. If we do that, will Gecko automatically forward events over plugins to the target plugin?
So, obviously we should fix the event-related bug we're seeing w/r/t mouse events and plugins - for the sake of correctness and also clickjacking.

However - slightly OT but relevant - for the specific problem of cam/mic UI clickjacking, a bigger solution for this might be to somehow always show the cursor when that UI is displayed. That would prevent this entire class of attacks. It's what the Flash Player does when its own cam/mic UI is active. I guess I'll go about filing a different bug for that.
(In reply to comment #21)

> Can we try to find a solution where it's not necessary to duplicate
> Gecko's event targeting logic in widget code? For example, I'd be
> very happy if ChildViewMouseTracker could just always dispatch the
> event to the top level ChildView in the NSView hierarchy of the
> target window. If we do that, will Gecko automatically forward
> events over plugins to the target plugin?

We can't just dispatch the event to the top level ChildView in the
NSView hierarchy.  Yes, Gecko will (probably) automatically forward
events over plugins to their correct targets.  But we need to know in
widget code (in ChildViewMouseTracker::ViewForEvent()) what the target
ChildView will actually be -- otherwise we won't be able to know (in
ChildViewMouseTracker::ReEvaluateMouseEnterState()) when to send a
mouse-entered or mouse-exited event.

We don't need to "duplicate Gecko's event targeting logic".  There's a
NS_QUERY_DOM_WIDGET_HITTEST event, which we're already using in
-[ChildView currentEventShouldFocusPlugin].  For a given nsIWidget, it
queries Gecko to find out whether or not a particular point "hits" the
widget -- which is all we need to know.
Attached patch Fix (obsolete) — Splinter Review
Here's a patch that fixes this bug in my tests.

And here's a tryserver build that I've just started:
https://tbpl.mozilla.org/?tree=Try&rev=fba4854b0737

The results should be available in a few hours (or possibly overnight).  When it is I'll ask others to test it.
(In reply to Steven Michaud from comment #23)
> otherwise we won't be able to know (in
> ChildViewMouseTracker::ReEvaluateMouseEnterState()) when to send a
> mouse-entered or mouse-exited event.

Good point. Though I'd be surprised if Gecko still needed view-level mouse enter/exit events.

> We don't need to "duplicate Gecko's event targeting logic".  There's a
> NS_QUERY_DOM_WIDGET_HITTEST event, which we're already using in
> -[ChildView currentEventShouldFocusPlugin].  For a given nsIWidget, it
> queries Gecko to find out whether or not a particular point "hits" the
> widget -- which is all we need to know.

Oh, good.
Comment on attachment 8411323 [details] [diff] [review]
Fix


>+    nsChildView* childWidget = (nsChildView*)[childView widget];
>+    // If childView is being destroyed return nil.
>+    if (!childWidget)
>+      return nil;
>+
>+    if (![childView isPluginView])
>+      break;
>+
>+    // If childView is a plugin view, we need to ask Gecko if some other object
>+    // overlaps the part of it over which the mouse is located.
>+    nsAutoRetainCocoaObject kungFuDeathGrip(childView);
>+
>+    NSPoint eventLoc = nsCocoaUtils::ScreenLocationForEvent(aEvent);
>+    eventLoc.y = nsCocoaUtils::FlippedScreenY(eventLoc.y);
>+    LayoutDeviceIntPoint widgetLoc = LayoutDeviceIntPoint::FromUntyped(
>+      childWidget->CocoaPointsToDevPixels(eventLoc) -
>+      childWidget->WidgetToScreenOffset());
>+
>+    WidgetQueryContentEvent hitTest(true, NS_QUERY_DOM_WIDGET_HITTEST,
>+                                    childWidget);
>+    hitTest.InitForQueryDOMWidgetHittest(widgetLoc);
>+    // This might destroy our widget.
>+    childWidget->DispatchWindowEvent(hitTest);
>+    if (![childView widget])
>+      return nil;
>+    if (hitTest.mSucceeded && hitTest.mReply.mWidgetIsHit)

Can you put this part into an override of the NSView "hitTest:" method instead, and keep the existing code in ChildViewMouseTracker as-is? I'm envisioning something like

- (NSView*)hitTest:(NSPoint*)aPoint
{
  NSView* target = [super hitTest:aPoint];
  if (target == self && [self isPluginView]) {
    ...
    if (gecko says this widget is not hit) {
      return nil;
    }
  }
  return target;
}
That would also make sure that for example mouseDown events target the right widget.
(In reply to comment #26)

I'm not sure whether what you specifically suggest would work:  -[ChildView hitTest:] needs to return an accurate result, not just nil if the particular plugin isn't "hit".

But it *would* be "neater" to put this logic into -[ChildView hitTest:], if that isn't too much trouble.

I'll see what I can come up with.
Actually the solution is obvious, now that I think about it:

Have -[ChildView hitTest] call [superview hitTest:] if the plugin it corresponds to isn't "hit".

Now let's see if what's obvious is also correct :-)
Actually Markus was right (in comment #26).  But there's some weirdness I need to figure out before I can post another patch -- the processing of the NS_QUERY_DOM_WIDGET_HITTEST event sometimes fails mysteriously.

I'll continue working on this next week.
Attached patch Fix rev1Splinter Review
Thanks for your suggestion, Markus.  It allowed me to combine my fix for this bug with my old fix for bug 627649.  It also uncovered failures in the handler for NS_QUERY_DOM_WIDGET_HITTEST events that weren't apparent (for some reason) either in my earlier patch for this bug or my patch for bug 627649.

It turns out the handler was doing far more (in its Init() and InitCommon() methods) that was necessary to handle NS_QUERY_DOM_WIDGET_HITTEST events, and was failing when some of these extra calls failed.

Markus, I'd like you to review the widget code changes.  Smaug, I'm asking you to review the DOM event handler changes.

I've started a tryserver build:
https://tbpl.mozilla.org/?tree=Try&rev=7fbeadb11eb2
Attachment #8411323 - Attachment is obsolete: true
Attachment #8414800 - Flags: review?(mstange)
Attachment #8414800 - Flags: review?(bugs)
Attachment #8414800 - Flags: review?(masayuki)
Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

Nice! I particularly like the code removal.
I suppose this will also fix bug 896550?
Attachment #8414800 - Flags: review?(mstange) → review+
Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

r=masayuki for ContentEventHandler.

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>--- a/widget/cocoa/nsChildView.mm
>+++ b/widget/cocoa/nsChildView.mm

>+- (NSView *)hitTest:(NSPoint)aPoint
>+{
>+  NSView* target = [super hitTest:aPoint];
>+  if ((target == self) && [self isPluginView] && mGeckoChild) {
>+    nsAutoRetainCocoaObject kungFuDeathGrip(self);

nit: If you put this kungFuDeathGrip only for dispatching the event, it must not be necessary because the event doesn't cause DOM event. However, the cost must be enough cheap but it guarantees the safe. Therefore, staying this line is OK.

>+
>+    NSPoint cocoaLoc = [[self superview] convertPoint:aPoint toView:self];
>+    LayoutDeviceIntPoint widgetLoc = LayoutDeviceIntPoint::FromUntyped(
>+      mGeckoChild->CocoaPointsToDevPixels(cocoaLoc));
>+
>+    WidgetQueryContentEvent hitTest(true, NS_QUERY_DOM_WIDGET_HITTEST,
>+                                    mGeckoChild);
>+    hitTest.InitForQueryDOMWidgetHittest(widgetLoc);
>+    // This might destroy our widget.
>+    mGeckoChild->DispatchWindowEvent(hitTest);
>+    if (!mGeckoChild) {
>+      return nil;
>+    }

nit: By same reason, this can be gone or replacing with NS_ENSURE_TRUE.

>+    if (hitTest.mSucceeded && !hitTest.mReply.mWidgetIsHit) {
>+      return nil;
>+    }
>+  }
>+  return target;
>+}
Attachment #8414800 - Flags: review?(masayuki) → review+
(In reply to comment #32)

> I suppose this will also fix bug 896550?

Yes, it does :-)

(In reply to comment #33)

> nit: If you put this kungFuDeathGrip only for dispatching the event,
> it must not be necessary because the event doesn't cause DOM event.

I don't understand.

ContentEventHandler::InitCommon() prior to my patch
(ContentEventHandler::InitBasic() with my patch) contains the
following:

  // If text frame which has overflowing selection underline is dirty,
  // we need to flush the pending reflow here.
  mPresShell->FlushPendingNotifications(Flush_Layout);

  // Flushing notifications can cause mPresShell to be destroyed (bug 577963).
  NS_ENSURE_TRUE(!mPresShell->IsDestroying(), NS_ERROR_FAILURE);

So pretty clearly processing a NS_QUERY_DOM_WIDGET_HITTEST event *can*
cause the widget it's dispatched on to be destroyed.  That's why I put
in the kungFuDeathGrip, and also why I null-checked mGeckoChild after
dispatching the event.

> nit: By same reason, this can be gone or replacing with NS_ENSURE_TRUE.

I'll use NS_ENSURE_TRUE.
Attachment #8414800 - Flags: review?(bugs)
(Following up comment #34)

> I'll use NS_ENSURE_TRUE.

Markus pointed out to me that the NS_ENSURE_* macros have been deprecated and replaced.  See bug 672843.

But what I already have fits the new style just fine, so I'll keep it and not use *any* macro.
(In reply to Steven Michaud from comment #34)
> (In reply to comment #33)
> 
> > nit: If you put this kungFuDeathGrip only for dispatching the event,
> > it must not be necessary because the event doesn't cause DOM event.
> 
> I don't understand.
> 
> ContentEventHandler::InitCommon() prior to my patch
> (ContentEventHandler::InitBasic() with my patch) contains the
> following:
> 
>   // If text frame which has overflowing selection underline is dirty,
>   // we need to flush the pending reflow here.
>   mPresShell->FlushPendingNotifications(Flush_Layout);
> 
>   // Flushing notifications can cause mPresShell to be destroyed (bug
> 577963).
>   NS_ENSURE_TRUE(!mPresShell->IsDestroying(), NS_ERROR_FAILURE);
> 
> So pretty clearly processing a NS_QUERY_DOM_WIDGET_HITTEST event *can*
> cause the widget it's dispatched on to be destroyed.  That's why I put
> in the kungFuDeathGrip, and also why I null-checked mGeckoChild after
> dispatching the event.

Ah, I completely forgot the bug...
https://hg.mozilla.org/mozilla-central/rev/b850412044d7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Blocks: 896550
Attachment #8405757 - Attachment mime type: application/zip → application/java-archive
Flags: sec-bounty? → sec-bounty+
thank you very much for the award!!!
I'm very happy !
where can i download the firefox fixed?
Confirmed bug in Fx29.
Verified fixed in Fx32, 2014-05-12.
Status: RESOLVED → VERIFIED
If i have found a regression (same vulnerability) and if i report this , can i win a new award?
If i have found a regression (same vulnerability) on the fixed version and if i report this , can i win a new award?
We judge each bug on its own merit. Feel free to file new bugs, but don't assume they will be rated the same as this one unless we feel it's appropriate. Thank you.
Is there a reason that this sec-high went in without sec-approval? being set?

We should probably take this in Aurora and Beta if this is a sec-high.

Does this affect ESR24?
> Does this affect ESR24?

Yes.  I checked both FF24.0esr and FF24.5.0esr.
We should get an ESR24 patch then.
Attached patch Fix rev1 for esr24 (obsolete) — Splinter Review
Carrying forward r+.
Attachment #8423933 - Flags: review+
Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Probably present since earliest days of Cocoa widgets
User impact if declined: Users will be exposed to a sec-high bug
Testing completed (on m-c, etc.): Substantial testing by myself, two weeks on m-c
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

This patch is quite large (and removes a lot of code).  But the reasoning behind it is well understood, and no regressions have surfaced after two weeks on m-c.
Attachment #8414800 - Flags: approval-mozilla-beta?
Attachment #8414800 - Flags: approval-mozilla-aurora?
Comment on attachment 8414800 [details] [diff] [review]
Fix rev1

Oops, wrong patch.
Attachment #8414800 - Flags: approval-mozilla-esr24?
Comment on attachment 8423933 [details] [diff] [review]
Fix rev1 for esr24

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Users will remain exposed to a sec-high bug
Fix Landed on Version: Present on trunk (32 branch) for past two weeks
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8423933 - Flags: approval-mozilla-esr24?
Attachment #8423933 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Attachment #8414800 - Flags: approval-mozilla-beta?
Attachment #8414800 - Flags: approval-mozilla-beta+
Attachment #8414800 - Flags: approval-mozilla-aurora?
Attachment #8414800 - Flags: approval-mozilla-aurora+
Fixed build breakage.  Carrying forward r+ and a+.
Attachment #8423933 - Attachment is obsolete: true
Attachment #8425078 - Flags: review+
Comment on attachment 8425078 [details] [diff] [review]
Fix rev1 for esr24 with extra esr24-specific changes

Relanded on mozilla-esr:
https://hg.mozilla.org/releases/mozilla-esr24/rev/6402fb9a21cb
Whiteboard: [description in comment #17] → [description in comment #17][adv-main30+][adv-esr24.6+]
Alias: CVE-2014-1539
Verified fixed also on latest Fx30 and Fx31, and Fx24.6.0esr.

Unfortunately, it appears that we may have broken Flash mouse cursor hiding in general on Fx24.6.0esr, Mac only. I've filed a separate bug for that - bug 1021815.
Summary: Firefox for MAC OS X - cursor can be totally invisible using flash object and div (leading to clickjacking , cursorjacking or others) → Cursor can be totally invisible using flash object and div
> Unfortunately, it appears that we may have broken Flash mouse cursor
> hiding in general on Fx24.6.0esr, Mac only. I've filed a separate
> bug for that - bug 1021815.

I've confirmed bug 102185 by testing with today's esr24 nightly.  And
I've confirmed that my patch for this bug is the trigger by backing it
out, then seeing that the bug disappears.

Off the top of my head, I have *no* idea how my patch could have had
this effect.  Note that bug 1021815 doesn't happen on any of the other
branches where my patch for this bug landed.

I will dig around a bit, trying to find an esr24-specific fix.  But
I'm not going to make any heroic efforts.  So we need to consider 1)
backing out my patch for this bug on esr24, and leaving it unfixed
there; 2) leaving bug 1021815 unfixed.
Thanks Steven, for the investigation. I'd choose option #1 personally, as I don't think the security bug is so bad as to warrant breaking legit Flash content. My concern is that ESR builds might be using enterprise apps that are built in Flash/Flex, and mouse event issues might cause them to break or be noticeably impaired. 

I'm inclined to say that this does not impede the release of Fx24.6.0esr, but others can weigh in on that.
I'm going to be spending at least another day on bug 1021815, next week.  We may get lucky -- it may turn out to be something stupid (and easily fixable).

But if we *aren't* lucky, I agree with your assessment.  Especially if you add in 1020763, it looks my patch has completely broken the passing of events to NPAPI plugins on the esr24 branch :-(  Allowing that to happen is much worse than leaving this bug unfixed in esr24.
I agree with the back out. I don't think it's suitable to ship ESR with the reported breakage. We need a back out today following which I'll kick off a new build. Matt has agreed to spoke test once the build is complete.

Al - Can you please confirm that the sec team is OK shipping this bug in ESR 24.6.0?

Ryan - Can you please assist with the back out on ESR24?
Flags: needinfo?(ryanvm)
I can help with the backout once Al signs off. Please ping me on IRC then.
Flags: needinfo?(ryanvm)
We should back this out on ESR.
> it may turn out to be something stupid (and easily fixable)

It was :-(  I should have been using screen coordinates in the esr24 version of -[ChildView hitTest:].

Here's a patch that fixes bug 1021815 in my tests.  It's more difficult to tell about bug 1020763, which doesn't have good STR.  But (testing with my Debug Events Plugin from bug 441880) I did notice some weirdness clicking on a plugin that this patch clears up.

Matt, please test this patch.  If you don't see any problems I'll ask someone for a review -- probably Markus Stange.
Attachment #8425078 - Attachment is obsolete: true
Whiteboard: [description in comment #17][adv-main30+][adv-esr24.6+] → [description in comment #17][adv-main30+]
Gecko 30 patch didn't apply, esr24 patch had to be backed out, wontfix for SeaMonkey 2.26.1 (Based on gecko 29)
ni? Matt RE: comment 71
Flags: needinfo?(mwobensmith)
I built Fx24esr with your patch. Everything seems OK. The hidden mouse cases that broke before appear to be fixed. I also did some testing with a second display and YouTube video controls, as well as some scenarios with tabbing around inside SWFs. 

I say we're fine and we can check again once the patch lands. Thanks Steven!
Flags: needinfo?(mwobensmith)
Attachment #8437334 - Flags: review?(mstange)
Comment on attachment 8437334 [details] [diff] [review]
Fix rev1 for esr24, fixed again

>+- (NSView *)hitTest:(NSPoint)aPoint
>+{
>+  NSView* target = [super hitTest:aPoint];
>+  NSWindow *window = [self window];
>+  if (window && (target == self) && [self isPluginView] && mGeckoChild) {
>+    nsAutoRetainCocoaObject kungFuDeathGrip(self);
>+
>+    NSPoint windowLoc = [[self superview] convertPoint:aPoint toView:nil];
>+    NSPoint screenLoc = [window convertBaseToScreen:windowLoc];
>+    screenLoc.y = nsCocoaUtils::FlippedScreenY(screenLoc.y);
>+    nsIntPoint widgetLoc = mGeckoChild->CocoaPointsToDevPixels(screenLoc) -
>+      mGeckoChild->WidgetToScreenOffset();
>+

Why not


+    NSPoint viewLoc = [[self superview] convertPoint:aPoint toView:self];
+    nsIntPoint widgetLoc = mGeckoChild->CocoaPointsToDevPixels(viewLoc);

?

Your code should work, too, but it seems unnecessarily complicated. But since it has already been tested and we're not going to be bothered by it on mozilla-central, it's probably better to keep what you have.
Attachment #8437334 - Flags: review?(mstange) → review+
Actually that might also have worked.

But though the code is more complex, it's simpler (and safer) to put everything in screen coordinates and then subtract the widget offset.  In any case, as you've said, we don't need to make this patch as neat as possible.
Comment on attachment 8437334 [details] [diff] [review]
Fix rev1 for esr24, fixed again

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: A sec-high bug will continue to effect esr24 users
Fix Landed on Version: 30
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8437334 - Flags: approval-mozilla-esr24?
Attachment #8437334 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Comment on attachment 8437334 [details] [diff] [review]
Fix rev1 for esr24, fixed again

Landed on mozilla-esr24:
https://hg.mozilla.org/releases/mozilla-esr24/rev/9400d92aa867
Verified fixed on Fx24.6.0esrpre, 2014-06-19.
Attachment #8405757 - Attachment description: test UNVISIBLE MOUSE.zip → test INVISIBLE MOUSE.zip
Attachment #8405757 - Attachment filename: test UNVISIBLE MOUSE.zip → test INVISIBLE MOUSE.zip
Depends on: CVE-2015-0810
Depends on: 1133299
Depends on: 1121833
Group: core-security
Attachment #8405757 - Attachment is obsolete: true
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.