Closed Bug 108104 Opened 19 years ago Closed 18 years ago

Key events from script can execute menu commands

Categories

(Core :: DOM: Events, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: security-bugs, Assigned: joki)

Details

(Whiteboard: AOLTWOK)

Attachments

(5 files, 1 obsolete file)

Sending key events from a script can activate commands in the menus, just as if
the user had chosen a menu item. This allows scripts to add bookmarks without
user confirmation, to name just one example. Events initiated by scripts in
content should not be able to call menu key-shortcuts.

The example below sends Alt-B Alt-B, which corresponds to Add Bookmark. Some
Ctrl-key menu shortcuts work as well.
Target Milestone: --- → mozilla0.9.7
Adding [discoverable] - this is a bug which is easy to figure out.
Whiteboard: [discoverable]
Testcase doesn't work for me.

It might be possible to use this to open/write arbitary files. (That's what I
wanted to try.)
Whiteboard: [discoverable]
My testcase works for me under both Linux and windows.
Mitchell's description of the testcase is slightly incorrect - the script sends
"ALT-B B" 
NOT "ALT-B ---ALT--- B".
Adding PDT for tracking and 6.2 branch review purposes.
Whiteboard: PDT
What are the chances we'd have a patch for this one by this afternoon?
Whiteboard: PDT → [PDT] [ETA ?]
Pretty good, though I need to talk it over with some people to make sure we're
picking up XUL key commands at the best spot.  I'll attach the patch in a mintue.
Status: NEW → ASSIGNED
Whiteboard: [PDT] [ETA ?] → PDT
looks good generally. Is getting the script security service manager an
expensive call for every event dispatch? Should we just cache it once we have it?
who else can we get to review?
Tentative PDT+, pending positive reviews. We'd like to see this one land on the
6.2 branch by 9 am PST (if possible).
Whiteboard: PDT → [PDT+]
r/sr=jst, but hyatt should have a look too and see if there are any other places
where we need to add similar checks.
Saari, I don't think we have to worry too much about performance at the location 
of the security check.  Its only called in the dispatchEvent case which is only 
used for user created events and some of our own internal dispatches, which to 
my knowledge, are not high frequency.  None of the native events hit it.  So due 
to the push to get this in more or less immediately I'm going to take that for 
the r on this and use jst for the sr and get this in tonight.
Attachment #58412 - Flags: superreview+
Attachment #58412 - Flags: review+
Replaces nsEventStateManager.cpp portion of previous patch
Comment on attachment 58514 [details] [diff] [review]
Modified part of patch for nsEventStateManager.cpp

sr=jst
Attachment #58514 - Flags: superreview+
Fix checked into main trunk and 6.2.1 branch.  Still want to talk to hyatt about 
possible optimization point but fix as is works just fine.
Adding Antonio to Cc.  He is helping with Mac testing.

Verified on 2001-11-26-6.2.1 build on WinNT, Linux (7.2), Mac OS X, Mac OS 9.1.

The above test case passes.


Why is this bug still open? Are waiting for checkin approval on the 0.9.4
branch? Or the 6.2 branch?
Verified on 2001-11-29-Trunk build on WinNT.

Above test case passes.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Marking as verified FIXED.
Status: RESOLVED → VERIFIED
edt0.9.4+ per chofmann's e-mail:
"
yes, go ahead.

thanks

chris h."
Keywords: edt0.9.4+
I'm having trouble modifying this patch for the 0.9.4 branch. joki, can you
help? reopening bug.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Here's my best guess at applying this patch to the 0.9.4 branch. When I run the
testcase (the first attachment), it opens the menu but doesn't choose a menu
item. I think something's slightly wrong.
Mitch, I don't have a 0.9.4 branch set up right now.  Can you take a look at 
this and try it out?

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&su
bdir=mozilla/layout/xul/base/src&command=DIFF_FRAMESET&file=nsMenuBarListener.cp
p&rev1=1.55&rev2=1.55.22.1&root=/cvsroot
Ah, that's what I was missing. With that addition, the patch works for 0.9.4.
As it's already been reviewed and approved, I'll check it in ASAP.
Attachment #60758 - Attachment is obsolete: true
Checked in on 0.9.4 branch. Bindu, can you please verify against the above 
testcase (first attachment) on the 0.9.4 embedding branch?
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
Keywords: fixed0.9.4
Doesnt happen on 2002-01-08-22-0.9.4ec windows 98
and 2002-01-04-08-trunk on linux RedHat 6.2

Verifying
Status: RESOLVED → VERIFIED
Arrgh, somehow part of the fix was never checked into the trunk! Reopening.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
The changes for nsMenuBarListener.cpp never got checked in on the trunk -
here's the diff for the current trunk.
Setting milestone to Moz1.0 - needs approval.
Status: REOPENED → ASSIGNED
Whiteboard: [PDT+]
Target Milestone: mozilla0.9.7 → mozilla1.0
Comment on attachment 73593 [details] [diff] [review]
Rest of the fix for the trunk

sr=jst
Attachment #73593 - Flags: superreview+
Comment on attachment 73593 [details] [diff] [review]
Rest of the fix for the trunk

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73593 - Flags: approval+
Verified on 6.2.2 on WinNT.

Added AOLTWOK in the Status Whiteboard.
Whiteboard: AOLTWOK
Adding Michael.
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
verifying on build 2002-04-01-03-trunk win98
Status: RESOLVED → VERIFIED
Group: security?
You need to log in before you can comment on or make changes to this bug.