239411 - install delay should stop+reset when Software Installation dialog loses focus

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P4)

Description

[Jesse Ruderman]

Ben's patch in bug 236056 doesn't guard against a variant of the attack I
alluded to in bug 162020 comment 0:
1. Open a new window.
2. In the original window, pop up an XPI dialog.
3. Convince the user to double-click somewhere in the new window.
4. On the first click, close the new window, letting the XPI dialog show through.

If the dialog starts without focus or loses focus, it needs to disable the
button and start counting down when the dialog gains focus.  This will probably
annoy Linux users whose window managers don't focus modal dialogs, but I don't
see an obvious way around that.

Comment 1

[Jesse Ruderman]

Attachment: demo (open evil.html after unzipping)

Comment 2

[Jesse Ruderman]

You have to allow scripts to "Raise or lower windows" for this exploit to work well.

Comment 3

[Doug Turner (:dougt)]

jesse, that is an excellent exploit.

Comment 4

[Jesse Ruderman]

Bug 162020 is blocking1.7+, so this should be too.

Comment 5

[chris hofmann]

any thoughts on how we are going to plug this?

Comment 6

[Doug Turner (:dougt)]

well, if you want something for 1.7, how about a modal dialog?  

Comment 7

[Jesse Ruderman]

The XPI dialog is already modal.  Are you suggesting making it application-modal?

Comment 8

[Doug Turner (:dougt)]

obviously. ;-)

Comment 9

[Dan M]

This exploit is less dangerous since bug 238684 was fixed. The testcase given
doesn't work in recent builds, since XPI installation from a timer is
disallowed. It also doesn't work to use a load listener on the decoy window,
instead of a timer. That's because the XPInstallManager seems to be invoked with
its parent window set to the topmost window, even if invoked from another
window, and the topmost window in this case is the still-loading decoy window.
XPI installation is also disallowed from loading windows.

I'm not saying this is no longer a problem. There may be some way to begin the
installation on the opener window from, say, the decoy window's mousemove
handler in the example. Focus the opener, begin installation, and then return
focus to yourself, or some such thing. I haven't been able to make it work.
There'd be flashing of windows, at best.

In case this exploit can be made to work, I rather like Jesse's suggestion of
stopping the installation countdown while the XPInstall dialog isn't focused.
It's simple, it's elegant, it's sufficient, isn't it?, and it doesn't require
the spanking the widget code would need before it could support
application-modal windows.

Sadly, I can't make that work, either. The XUL change is simple enough but I
can't seem to get blur notifications in the XPInstall dialog unless I first hit
the Cancel button (and drag the mouse off before releasing). See bug 164686.

Comment 10

[Asa Dotzler [:asa]]

p4 priority - not a blocker. if a fully reviewed patch materializes, please
nominate for aviary approval. 

Comment 11

[Asa Dotzler [:asa]]

->bsmedberg

Comment 12

[Benjamin Smedberg]

Attachment: Reset the xpinstall counter onblur, rev. 1

This does make one UI change: it counts down by tenths of a second, instead of
by whole seconds, because it easy to click back to the install confirmation and
see (2) and not realize that it's counting down again.

Comment 13

[Benjamin Smedberg]

If this doesn't cause leaks on trunk it's probably 1.0.x-worthy.

Comment 14

[Benjamin Smedberg]

1.0.5 just about wrapped up.  we might want this if we do a 1.0.6

-chofmann

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 186601 [details] [diff] [review]
Reset the xpinstall counter onblur, rev. 1

r=[e-mail]

Comment 16

[Benjamin Smedberg]

Fixed on trunk for 1.8b4

Comment 17

[Daniel Veditz [:dveditz]]

Won't kill us not to fix this, not blocking (but I'll leave the approval request for now). Fixed in 1.5 which will be released before 1.0.8, mainly interested in severe problems on the old branch.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 186601 [details] [diff] [review]
Reset the xpinstall counter onblur, rev. 1

not needed for old branches