nsListControlFrame::FireMenuItemActiveEvent called at unsafe times

VERIFIED FIXED

Status

()

Core
Disability Access APIs
--
critical
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: bz, Assigned: Aaron Leventhal)

Tracking

(4 keywords)

Trunk
access, fixed1.8.1, qawanted, verified1.8.0.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.0.4 -
blocking1.8.0.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(8 attachments, 2 obsolete attachments)

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.
(Assignee)

Updated

12 years ago
Assignee: aaronleventhal → mats.palmgren
Keywords: access
Er... Is Mats even active currently?  Why did this bug get dumped on him?  Aaron, you have all the blame for the relevant code.
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.4?
(Assignee)

Comment 2

12 years ago
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.
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.  ;)
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?
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.4?
Flags: blocking1.8.0.4-

Updated

12 years ago
Whiteboard: [sg:critical]
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...
Keywords: qawanted
Created attachment 220493 [details]
testcase1
Created attachment 220494 [details]
Testcase2, using window.close()
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.
Created attachment 220496 [details]
Testcase3, using display none
(Assignee)

Comment 10

12 years ago
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.
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.
(Assignee)

Updated

12 years ago
Attachment #220556 - Attachment description: Untested, but compiles → Tested, does not break a11y
(Assignee)

Comment 12

12 years ago
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.
Attachment #220556 - Flags: superreview?
(Assignee)

Updated

12 years ago
Attachment #220556 - Flags: superreview? → superreview?(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!
Attachment #220556 - Flags: superreview?(bzbarsky)
Attachment #220556 - Flags: superreview+
Attachment #220556 - Flags: review+
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
Attachment #220556 - Flags: approval-branch-1.8.1+
(Assignee)

Comment 15

12 years ago
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?
(Assignee)

Updated

12 years ago
Assignee: mats.palmgren → aaronleventhal
> 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.

(Assignee)

Comment 17

12 years ago
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!
(Assignee)

Comment 18

12 years ago
Comment on attachment 220661 [details] [diff] [review]
Final patch for trunk which makes method protected

Bah, forgot to put nsMenuFrame.cpp change in.
Attachment #220661 - Attachment is obsolete: true
(Assignee)

Comment 19

12 years ago
Created attachment 220678 [details] [diff] [review]
Checkin patch for 1.8 branch
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
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.
Keywords: fixed1.8 → fixed1.8.1
(Assignee)

Updated

12 years ago
Attachment #220556 - Flags: approval1.8.0.5?
Verified fixed with 2006-05-04 trunk build. None of the testcases crash or hang anymore.
Status: RESOLVED → VERIFIED

Updated

11 years ago
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.5? → blocking1.8.0.5+
Comment on attachment 220556 [details] [diff] [review]
Tested, does not break a11y

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #220556 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Assignee)

Comment 23

11 years ago
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.
(Assignee)

Comment 24

11 years ago
Actually, I don't want to check this in right now. It may have caused bug 339237. Investigating.
(Reporter)

Updated

11 years ago
Depends on: 339237
(Assignee)

Comment 25

11 years ago
Isn't nsIPresShell::HandleDOMEventWithTarget() just as dangerous? I get my popuphiding event in the same stack it's fired from in layout.
(Assignee)

Comment 26

11 years ago
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 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.
Attachment #220556 - Flags: approval1.8.0.5+ → approval1.8.0.5-
> 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...
(Assignee)

Comment 29

11 years ago
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.
Attachment #226367 - Flags: superreview?(bzbarsky)
Attachment #226367 - Flags: review?(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.
Attachment #226367 - Flags: superreview?(bzbarsky)
Attachment #226367 - Flags: superreview+
Attachment #226367 - Flags: review?(bzbarsky)
Attachment #226367 - Flags: review+
(Assignee)

Comment 31

11 years ago
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.
Attachment #226367 - Flags: approval1.8.0.5?
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
Attachment #226367 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Assignee)

Comment 33

11 years ago
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
Keywords: fixed1.8.0.5

Comment 34

11 years ago
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
Keywords: fixed1.8.0.5 → verified1.8.0.5

Comment 35

11 years ago
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.

Updated

11 years ago
Attachment #232740 - Attachment is obsolete: true

Comment 36

11 years ago
Created attachment 232745 [details] [diff] [review]
1.0.x without regressions
That patch looks wrong.  For example, |eventName| is unused.

Comment 38

11 years ago
(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?
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.
Group: security
Flags: in-testsuite?
Depends on: 400443
You need to log in before you can comment on or make changes to this bug.