Last Comment Bug 336162 - nsListControlFrame::FireMenuItemActiveEvent called at unsafe times
: nsListControlFrame::FireMenuItemActiveEvent called at unsafe times
Status: VERIFIED FIXED
[sg:critical]
: access, fixed1.8.1, qawanted, verified1.8.0.5
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Aaron Leventhal
:
Mentors:
Depends on: 339237 400443
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-01 11:44 PDT by Boris Zbarsky [:bz]
Modified: 2007-12-20 13:51 PST (History)
8 users (show)
dveditz: blocking1.8.0.4-
dveditz: blocking1.8.0.5+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase1 (523 bytes, text/html)
2006-05-02 02:12 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Testcase2, using window.close() (837 bytes, text/html)
2006-05-02 02:18 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Testcase3, hang/freeze (603 bytes, text/html)
2006-05-02 03:09 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Testcase3, using display none (517 bytes, text/html)
2006-05-02 03:20 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Tested, does not break a11y (7.38 KB, patch)
2006-05-02 14:20 PDT, Aaron Leventhal
bzbarsky: review+
bzbarsky: superreview+
bzbarsky: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5-
Details | Diff | Splinter Review
Final patch for trunk which makes method protected (7.47 KB, patch)
2006-05-03 10:02 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Checkin patch for 1.8 branch (6.86 KB, patch)
2006-05-03 11:22 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Patch for 1.8.0 branch -- unlike trunk/1.8 patch this only fixes original list bug and does not try to fix menu events, thus avoiding a11y regression (9.93 KB, patch)
2006-06-20 12:10 PDT, Aaron Leventhal
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review
1.0.x backport version of the patch (9.24 KB, patch)
2006-08-08 08:30 PDT, Alexander Sack
no flags Details | Diff | Splinter Review
1.0.x without regressions (8.19 KB, patch)
2006-08-08 08:35 PDT, Alexander Sack
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2006-05-01 11:44:01 PDT
This is similar to bug 231830 except I have no idea where to even start fixing this, short of making FireMenuItemActiveEvent a no-op.  The basic problem is that nsListControlFrame::FireMenuItemActiveEvent fires a DOM event (so allows arbitrary script to execute), at all sorts of random times.  It's trivial to exploit this to cause virtual function calls on deleted frame objects.
Comment 1 Boris Zbarsky [:bz] 2006-05-01 11:50:22 PDT
Er... Is Mats even active currently?  Why did this bug get dumped on him?  Aaron, you have all the blame for the relevant code.
Comment 2 Aaron Leventhal 2006-05-01 11:54:44 PDT
He's starting June 1 to deal with key nav fixes. I'm stepping away from focus and keyboard nav related code as I don't have time for it. I'll deal with this bug if there's no other choice.
Comment 3 Boris Zbarsky [:bz] 2006-05-01 12:04:11 PDT
Ah, cool.

June 1 may be a little late for the security releases.  Then again, if this isn't going to make 1.8.0.4 anyway it may be ok.  Check, please?  Given that this is almost certainly exploitable I'd really prefer we didn't ship releases with it and all.  ;)
Comment 4 Daniel Veditz [:dveditz] 2006-05-01 15:57:35 PDT
We're past the cut-off for 1.8.0.4 bugs. Once we have a safe tested fix you can ask for approval and we could talk about exceptions, but the comments imply that's not likely.

bz: could you attach a little testcase QA can use to verify the eventual fix?
Comment 5 Boris Zbarsky [:bz] 2006-05-01 20:40:04 PDT
I'm not sure I can post a testcase that exercises all aspects of this bug, actually.  And if I post one that exercises only part of it, it's not really useful for verifying the fix, past rubber-stamping that it fixes _something_.

Basically, the problem is that there are at least 2 or 3 different ways we can crash here, probably more, and I'm not sure how to trigger all of them. I guess I can spend a few days figuring that out, if we don't have any testcases by the time we have patches to test...
Comment 6 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-02 02:12:25 PDT
Created attachment 220493 [details]
testcase1
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-02 02:18:01 PDT
Created attachment 220494 [details]
Testcase2, using window.close()
Comment 8 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-02 03:09:06 PDT
Created attachment 220495 [details]
Testcase3, hang/freeze

With this testcase I get a hang/freeze with 100% cpu while following the steps in the testcase.
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-02 03:20:08 PDT
Created attachment 220496 [details]
Testcase3, using display none
Comment 10 Aaron Leventhal 2006-05-02 14:20:59 PDT
Created attachment 220556 [details] [diff] [review]
Tested, does not break a11y

Boris, this would be the most likely approach, no? I'll test with a11y when I get the chance.
Comment 11 Boris Zbarsky [:bz] 2006-05-02 14:33:05 PDT
If that approach works for a11y, great.  It should certainly fix this bug.  But the change to nsBoxFrame most likely breaks menus, for what it's worth.
Comment 12 Aaron Leventhal 2006-05-02 19:02:17 PDT
Comment on attachment 220556 [details] [diff] [review]
Tested, does not break a11y

Bz, what would it break wrt menus? The DOMMenuItemActive event is all that's fired from menus. That's used by a11y, places and toolbar.xml to update a status line (not sure how to get that code to execute).

I assume this could be a bug for remote XUL so we'd need it there.
Comment 13 Boris Zbarsky [:bz] 2006-05-02 21:29:25 PDT
Comment on attachment 220556 [details] [diff] [review]
Tested, does not break a11y

Oh, I see.  I thought you were changing the behavior of events  that triggered menus...

r+sr=bzbarsky if you make this method protected, not public.  Thanks for fixing this!
Comment 14 Boris Zbarsky [:bz] 2006-05-02 21:31:22 PDT
Comment on attachment 220556 [details] [diff] [review]
Tested, does not break a11y

Please land on 1.8 branch too, once it bakes on the trunk for a bit
Comment 15 Aaron Leventhal 2006-05-03 06:01:32 PDT
Boris, I have to leave it public or it won't build, because of this line:
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuFrame.cpp#683

Also, since this is security sensitive do we need to get approval in for 1.8.0.x branch before checking it in everywhere? Otherwise won't we be clueing in potential abusers who can see that there's a bug they don't have access to, but can see what the checkin is?
Comment 16 Boris Zbarsky [:bz] 2006-05-03 08:03:18 PDT
> Boris, I have to leave it public or it won't build, because of this line:

Why not just call 

  FireDOMEvent(NS_LITERAL_STRING("DOMMenuInactive"), menuPopup->GetContent());

there?  It's not like we're in a static method or anything.

> do we need to get approval in for 1.8.0.x branch before checking it in
> everywhere?

You can't get approval until you've checked this in and it's baked.  So no.

> can see that there's a bug they don't have access to, but can see what the
> checkin is?

Yeah, well.  That's life.

Comment 17 Aaron Leventhal 2006-05-03 10:02:14 PDT
Created attachment 220661 [details] [diff] [review]
Final patch for trunk which makes method protected

Bz, you're so right -- wish I had noticed the obvious myself!
Comment 18 Aaron Leventhal 2006-05-03 10:54:55 PDT
Comment on attachment 220661 [details] [diff] [review]
Final patch for trunk which makes method protected

Bah, forgot to put nsMenuFrame.cpp change in.
Comment 19 Aaron Leventhal 2006-05-03 11:22:02 PDT
Created attachment 220678 [details] [diff] [review]
Checkin patch for 1.8 branch
Comment 20 Boris Zbarsky [:bz] 2006-05-03 14:31:43 PDT
1.8.1, not 1.8...  (and what happened to baking on trunk before landing on the branch?)

Also, you probably want to request 1.8.0.x approval on that patch.
Comment 21 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-05 08:40:02 PDT
Verified fixed with 2006-05-04 trunk build. None of the testcases crash or hang anymore.
Comment 22 Daniel Veditz [:dveditz] 2006-06-05 12:12:00 PDT
Comment on attachment 220556 [details] [diff] [review]
Tested, does not break a11y

approved for 1.8.0 branch, a=dveditz for drivers
Comment 23 Aaron Leventhal 2006-06-06 00:29:34 PDT
Does anyone have a MOZILLA_1_8_0 branch they can check this in for me with? I only have Visual Studio 8 here so I can't get a build going.
Comment 24 Aaron Leventhal 2006-06-06 03:51:32 PDT
Actually, I don't want to check this in right now. It may have caused bug 339237. Investigating.
Comment 25 Aaron Leventhal 2006-06-07 00:30:09 PDT
Isn't nsIPresShell::HandleDOMEventWithTarget() just as dangerous? I get my popuphiding event in the same stack it's fired from in layout.
Comment 26 Aaron Leventhal 2006-06-07 04:01:47 PDT
We can't take the menu part of the fix -- it breaks screen reader support for menus. Not sure if the rest is cleanly separable.
Comment 27 Daniel Veditz [:dveditz] 2006-06-07 06:21:42 PDT
Comment on attachment 220556 [details] [diff] [review]
Tested, does not break a11y

removing approval based on regressions and comment 26, will wait for a branch-safe patch.
Comment 28 Boris Zbarsky [:bz] 2006-06-07 07:49:31 PDT
> Isn't nsIPresShell::HandleDOMEventWithTarget() just as dangerous?

If called at the wrong times, yes.

And yes, popups generally violate all sorts of layout invariants; we have bugs on that.  You should see the fireworks in a build that actually asserts said invariants...
Comment 29 Aaron Leventhal 2006-06-20 12:10:20 PDT
Created attachment 226367 [details] [diff] [review]
Patch for 1.8.0 branch -- unlike trunk/1.8 patch this only fixes original list bug and does not try to fix menu events, thus avoiding a11y regression

Note: I am planning to use the nsFrame::FireDOMEvent() method so that the approved checkin for bug 246236 can land without bustage.
Comment 30 Boris Zbarsky [:bz] 2006-06-20 12:14:06 PDT
Comment on attachment 226367 [details] [diff] [review]
Patch for 1.8.0 branch -- unlike trunk/1.8 patch this only fixes original list bug and does not try to fix menu events, thus avoiding a11y regression

OK, I buy this.
Comment 31 Aaron Leventhal 2006-06-20 12:17:34 PDT
Comment on attachment 226367 [details] [diff] [review]
Patch for 1.8.0 branch -- unlike trunk/1.8 patch this only fixes original list bug and does not try to fix menu events, thus avoiding a11y regression

Must re-request approval for latest patch.
Comment 32 Daniel Veditz [:dveditz] 2006-06-20 14:21:18 PDT
Comment on attachment 226367 [details] [diff] [review]
Patch for 1.8.0 branch -- unlike trunk/1.8 patch this only fixes original list bug and does not try to fix menu events, thus avoiding a11y regression

approved for 1.8.0 branch, a=dveditz for drivers
Comment 33 Aaron Leventhal 2006-06-20 17:33:56 PDT
Checked into 1.8.0.5

Checking in layout/forms/nsListControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsListControlFrame.cpp,v  <--  nsListControlFrame.cpp
new revision: 1.372.2.1.4.1; previous revision: 1.372.2.1
done
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.574.2.4.2.4; previous revision: 3.574.2.4.2.3
done
Checking in layout/generic/nsFrame.h;
/cvsroot/mozilla/layout/generic/nsFrame.h,v  <--  nsFrame.h
new revision: 3.228.20.1; previous revision: 3.228
done
Checking in layout/xul/base/src/nsBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.cpp,v  <--  nsBoxFrame.cpp
new revision: 1.273.10.1.4.3; previous revision: 1.273.10.1.4.2
done
Checking in layout/xul/base/src/nsBoxFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.h,v  <--  nsBoxFrame.h
new revision: 1.92.20.1; previous revision: 1.92
done
Checking in layout/xul/base/src/nsImageBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsImageBoxFrame.cpp,v  <--  nsImageBoxFrame.cpp
new revision: 1.93.16.1; previous revision: 1.93
done
Checking in layout/xul/base/src/nsMenuBarFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsMenuBarFrame.cpp,v  <--  nsMenuBarFrame.cpp
new revision: 1.131.14.2.2.1; previous revision: 1.131.14.2
done
Checking in layout/xul/base/src/nsMenuFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsMenuFrame.cpp,v  <--  nsMenuFrame.cpp
new revision: 1.293.6.3.2.2; previous revision: 1.293.6.3.2.1
done
Checking in layout/xul/base/src/nsPopupSetFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp,v  <--  nsPopupSetFrame.cpp
new revision: 1.123.10.2.2.1; previous revision: 1.123.10.2
done
Comment 34 Jay Patel [:jay] 2006-07-07 14:22:07 PDT
v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.5) Gecko/20060706 Firefox/1.5.0.5
Comment 35 Alexander Sack 2006-08-08 08:30:22 PDT
Created attachment 232740 [details] [diff] [review]
1.0.x backport version of the patch

tried my best ... and indeed looks good ... for me. Boris, if you have any wise suggestions don't hestitate to let me know.
Comment 36 Alexander Sack 2006-08-08 08:35:43 PDT
Created attachment 232745 [details] [diff] [review]
1.0.x without regressions
Comment 37 Boris Zbarsky [:bz] 2006-08-08 08:56:40 PDT
That patch looks wrong.  For example, |eventName| is unused.
Comment 38 Alexander Sack 2006-08-08 09:41:49 PDT
(In reply to comment #37)
> That patch looks wrong.  For example, |eventName| is unused.
> 

Ah, ok, I missed that ... "DOMMenuItemActive" is currently hard-coded. Anything else?
Comment 39 Boris Zbarsky [:bz] 2006-08-08 10:02:51 PDT
I'm not sure why you didn't just port nsPLDOMEvent but instead changed it around (e.g. made the type an nsString* instead of nsString).  Makes it hard to tell what else could be up.

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