336162 - nsListControlFrame::FireMenuItemActiveEvent called at unsafe times

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

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]

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

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

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

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

Attachment: testcase1

Comment 7

[Martijn Wargers (dead)]

Attachment: Testcase2, using window.close()

Comment 8

[Martijn Wargers (dead)]

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

Attachment: Testcase3, using display none

Comment 10

[Aaron Leventhal]

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

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]

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

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

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]

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

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

Attachment: Final patch for trunk which makes method protected

Bz, you're so right -- wish I had noticed the obvious myself!

Comment 18

[Aaron Leventhal]

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]

Attachment: Checkin patch for 1.8 branch

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Verified fixed with 2006-05-04 trunk build. None of the testcases crash or hang anymore.

Comment 22

[Daniel Veditz [:dveditz]]

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]

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]

Actually, I don't want to check this in right now. It may have caused bug 339237. Investigating.

Comment 25

[Aaron Leventhal]

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]

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

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

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

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

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]

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

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]

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

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]

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

Attachment: 1.0.x without regressions

Comment 37

[Boris Zbarsky [:bzbarsky]]

That patch looks wrong.  For example, |eventName| is unused.

Comment 38

[Alexander Sack]

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

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.