Last Comment Bug 161721 - install in onkeypress for space key bypasses warning dialog
: install in onkeypress for space key bypasses warning dialog
[adt1 RTM] [ETA 080/9] security
Product: Core Graveyard
Classification: Graveyard
Component: Installer: XPInstall Engine (show other bugs)
: Trunk
: x86 Windows XP
: P1 critical (vote)
: mozilla1.0.1
Assigned To: Daniel Veditz [:dveditz]
: bsharma
: 99777 (view as bug list)
Depends on: 149478
Blocks: 99777 143047
  Show dependency treegraph
Reported: 2002-08-08 10:24 PDT by Jesse Ruderman
Modified: 2015-12-11 07:21 PST (History)
12 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

testcase (355 bytes, text/html)
2002-08-08 10:26 PDT, Jesse Ruderman
no flags Details
Fixes bug and any similar attacks (1.15 KB, patch)
2002-08-08 14:08 PDT, Daniel Veditz [:dveditz]
bugzilla: superreview+
Details | Diff | Splinter Review
better patch (1.46 KB, patch)
2002-08-08 15:58 PDT, Daniel Veditz [:dveditz]
dveditz: review+
dveditz: superreview+
rjesup: approval+
Details | Diff | Splinter Review

Description Jesse Ruderman 2002-08-08 10:24:59 PDT
If I can get you to press the space key while reading a page, I can install an
XPI. This is due to a bug 99777, which makes dialogs respond to the space key
even if they were opened as a result of the space keypress.

This security hole is a combination of two small bugs:

bug  99777 Advance to next unread... dialog not waiting for an answer
bug 161713 default button in install dialog should be Cancel
Comment 1 Jesse Ruderman 2002-08-08 10:26:47 PDT
Created attachment 94482 [details]
Comment 2 Mitchell Stoltz (not reading bugmail) 2002-08-08 11:40:57 PDT
This is a stop-ship. We should at least fix the second bug mentioned above.
Comment 3 Jesse Ruderman 2002-08-08 11:49:27 PDT
This bug may also affect the enablePrivilege dialog once bug 133582 (many
dialogs have initial focus on checkbox) is fixed.
Comment 4 Daniel Veditz [:dveditz] 2002-08-08 13:02:43 PDT
the cancel button bug is a duplicate of 149478
Comment 5 Daniel Veditz [:dveditz] 2002-08-08 13:40:32 PDT
Leaving aside people who hold the spacebar down, how are we able to create a
modal dialog and start listening for keypresses before the keyup event goes
through? What other bad things can be accomplished by switching focus out from
under events? Can we do the same for script-generated events?

In any case we should think about backing out Blake's fix for bug 44676, or
changing it to check that the specific button was in the depressed state from a
keydown event and ignoring the key up if not. That might even address the minor
glitch noted in bug 44676 comment 26.
Comment 6 Paul Wyskoczka 2002-08-08 13:53:35 PDT
Removing adt1.0.1 until we have a patch.  Added adt1 rtm for tracking purposes.

Since jimmy is on sabbatical, assinging qa to
Comment 7 Alec Flett 2002-08-08 13:55:09 PDT
ah! this also affects mail, when you hit space at the end of the last message in
a folder, and it automatically hits "ok" in the "Go to next folder?" message.
Comment 8 Daniel Veditz [:dveditz] 2002-08-08 14:08:27 PDT
Created attachment 94516 [details] [diff] [review]
Fixes bug and any similar attacks

This patch fixes the specific attack and any others that try to take advantage
of a keydown in one window with the keyup in a different dialog (e.g. this
solves bug 99777 too).

Slightly riskier than simply chaging the install dialog default, but safer in
other ways, too. I'm sure the install dialog isn't the only one with a risky
default button.
Comment 9 Blake Ross 2002-08-08 14:23:48 PDT
Comment on attachment 94516 [details] [diff] [review]
Fixes bug and any similar attacks

Comment 10 Daniel Veditz [:dveditz] 2002-08-08 14:38:51 PDT
alecf: care to review? I've got sr= from Blake who wrote this originally.

It's not really an install bug, we should get XUL widget QA looking this over too.
Comment 11 Alec Flett 2002-08-08 14:42:56 PDT
wow, I mean its one of those that seems quite reasonable, but I don't know the
intricacies of the event system. I guess I'd rather deferr to saari, joki, or
danm if possible... 
Comment 12 Daniel Veditz [:dveditz] 2002-08-08 15:58:53 PDT
Created attachment 94535 [details] [diff] [review]
better patch

argh, had the right mask but the line was already so long forgot to test for
equality. Jesse pointed out that with the last patch the button would still be
triggered on keyup if it was either active OR hovered. Probably not
exploitable, but still wrong. Thanks for it.
Comment 13 Paul Wyskoczka 2002-08-08 16:50:57 PDT
saari: Can you review this fix.

Adding adt1.0.1+ on behalf of the adt for checkin to the 1.0 branch, pending
saari's review.  Please get drivers approval before checking in. When you check
this into the branch, please change the mozilla1.0.1+ keyword to fixed1.0.1.
Comment 14 Jaime Rodriguez, Jr. 2002-08-08 17:23:29 PDT
verbal r=saari. pls check this after getting Drivers' approval.
Comment 15 Daniel Veditz [:dveditz] 2002-08-08 19:29:23 PDT
Patch for changing the cancel button default was added to bug 149478
Comment 16 Daniel Veditz [:dveditz] 2002-08-08 19:30:40 PDT
Comment on attachment 94535 [details] [diff] [review]
better patch

carrying over blake's sr= and marking saari's r=
Comment 17 georgi - hopefully not receiving bugspam 2002-08-09 01:55:47 PDT
Is this windows only, I can't reproduce it on linux?
Comment 18 Randell Jesup [:jesup] 2002-08-09 09:40:54 PDT
Comment on attachment 94535 [details] [diff] [review]
better patch

Approved for 1.0.1 branch, please check in ASAP and change mozilla1.0.1+ to
fixed1.0.1.  Also please request 1.1 branch approval, and we should probably
commit to trunk soon too.
Comment 19 Asa Dotzler [:asa] 2002-08-09 10:18:16 PDT
let's get this into the 1.1 branch. Thanks. 
a=asa (on behalf of drivers) for checkin to 1.1
Comment 20 Daniel Veditz [:dveditz] 2002-08-09 16:33:36 PDT
*** Bug 99777 has been marked as a duplicate of this bug. ***
Comment 21 Daniel Veditz [:dveditz] 2002-08-09 17:24:21 PDT
Checked into 1.0 and 1.1 branches. Will check into trunk when tree opens
Comment 22 Daniel Veditz [:dveditz] 2002-08-09 18:34:59 PDT
checked into trunk
Comment 23 Jaime Rodriguez, Jr. 2002-08-12 11:21:23 PDT
bsharma: bindu, can you pls verify this as fixed on the 1.0 branch, then replace
"fixed1.0.1" with "verified1.0.1"? thanks!
Comment 24 bsharma 2002-08-12 12:59:40 PDT
Verified on 2002-08-branch build on Win 2000.

Open the testcase.
Press the space key, a dialog is opened to install the xpi.
Clicked on Install, an error dialog is opened.

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