install in onkeypress for space key bypasses warning dialog

VERIFIED FIXED in mozilla1.0.1

Status

Core Graveyard
Installer: XPInstall Engine
P1
critical
VERIFIED FIXED
15 years ago
a year ago

People

(Reporter: Jesse Ruderman, Assigned: dveditz)

Tracking

Trunk
mozilla1.0.1
x86
Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt1 RTM] [ETA 080/9] security)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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
(Reporter)

Comment 1

15 years ago
Created attachment 94482 [details]
testcase
(Reporter)

Updated

15 years ago
Depends on: 99777, 161713
This is a stop-ship. We should at least fix the second bug mentioned above.
Status: NEW → ASSIGNED
Keywords: adt1.0.1, mozilla1.0.1, nsbeta1
(Reporter)

Comment 3

15 years ago
This bug may also affect the enablePrivilege dialog once bug 133582 (many
dialogs have initial focus on checkbox) is fixed.
(Assignee)

Comment 4

15 years ago
the cancel button bug is a duplicate of 149478
Depends on: 149478
No longer depends on: 161713
(Assignee)

Comment 5

15 years ago
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

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

Since jimmy is on sabbatical, assinging qa to bsharma@netscape.com
QA Contact: jimmylee → bsharma

Comment 7

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

Updated

15 years ago
Keywords: adt1.0.1

Updated

15 years ago
Blocks: 143047
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt1 RTM] [ETA Needed]
(Assignee)

Comment 8

15 years ago
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

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

sr=blake
Attachment #94516 - Flags: superreview+
(Assignee)

Comment 10

15 years ago
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

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

Comment 12

15 years ago
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.
Attachment #94516 - Attachment is obsolete: true

Comment 13

15 years ago
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.
Keywords: adt1.0.1+
Whiteboard: [adt1 RTM] [ETA Needed] → [adt1 RTM] [ETA 8/9]

Comment 14

15 years ago
verbal r=saari. pls check this after getting Drivers' approval.
Keywords: approval
Priority: -- → P1
Whiteboard: [adt1 RTM] [ETA 8/9] → [adt1 RTM] [ETA 080/9]
Target Milestone: --- → mozilla1.0.1
(Assignee)

Comment 15

15 years ago
Patch for changing the cancel button default was added to bug 149478
Blocks: 99777
No longer depends on: 99777
(Assignee)

Comment 16

15 years ago
Comment on attachment 94535 [details] [diff] [review]
better patch

carrying over blake's sr= and marking saari's r=
Attachment #94535 - Flags: superreview+
Attachment #94535 - Flags: review+
Is this windows only, I can't reproduce it on linux?
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.
Attachment #94535 - Flags: approval+

Updated

15 years ago
Keywords: mozilla1.0.1 → mozilla1.0.1+

Comment 19

15 years ago
let's get this into the 1.1 branch. Thanks. 
a=asa (on behalf of drivers) for checkin to 1.1
(Assignee)

Comment 20

15 years ago
*** Bug 99777 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 21

15 years ago
Checked into 1.0 and 1.1 branches. Will check into trunk when tree opens
Keywords: mozilla1.0.1+ → fixed1.0.1
(Assignee)

Comment 22

15 years ago
checked into trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 23

15 years ago
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

15 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.1 → verified1.0.1
Group: security?
(Reporter)

Updated

14 years ago
Whiteboard: [adt1 RTM] [ETA 080/9] → [adt1 RTM] [ETA 080/9] security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.