161721 - install in onkeypress for space key bypasses warning dialog

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect, P1)

Description

[Jesse Ruderman]

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]

Attachment: testcase

Comment 2

[Mitchell Stoltz (not reading bugmail)]

This is a stop-ship. We should at least fix the second bug mentioned above.

Comment 3

[Jesse Ruderman]

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

the cancel button bug is a duplicate of 149478

Comment 5

[Daniel Veditz [:dveditz]]

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]

Removing adt1.0.1 until we have a patch.  Added adt1 rtm for tracking purposes.

Since jimmy is on sabbatical, assinging qa to [e-mail]

Comment 7

[Alec Flett]

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

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

Comment on attachment 94516 [details] [diff] [review]
Fixes bug and any similar attacks

sr=blake

Comment 10

[Daniel Veditz [:dveditz]]

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]

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

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

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

verbal r=saari. pls check this after getting Drivers' approval.

Comment 15

[Daniel Veditz [:dveditz]]

Patch for changing the cancel button default was added to bug 149478

Comment 16

[Daniel Veditz [:dveditz]]

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]

Is this windows only, I can't reproduce it on linux?

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

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

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

*** Bug 99777 has been marked as a duplicate of this bug. ***

Comment 21

[Daniel Veditz [:dveditz]]

Checked into 1.0 and 1.1 branches. Will check into trunk when tree opens

Comment 22

[Daniel Veditz [:dveditz]]

checked into trunk

Comment 23

[Jaime Rodriguez, Jr.]

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]

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.