238684 - Onload XPI installs should be blocked by default

Status: RESOLVED FIXED

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

Description

[Roope Lehmuslehto]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040316
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040316

Visit address:
http://cracks.ss.ru/download/tsrh/tsrh-activecaptions_15_exe.zip.html with
Firefox (0.8). It tries automatically to install XPI package called "Content
Access Plugin 1.01".

Note that IE has also signature system that doesn't help much. Stupid users
still click Install button. And onload installs are also annoying. 

Problem is discussed in:
http://forums.mozillazine.org/viewtopic.php?p=448910#448910

Reproducible: Always
Steps to Reproduce:

Comment 1

[Roope Lehmuslehto]

Attachment: Screenshot of spyware installer

Comment 2

[Roope Lehmuslehto]

Attachment: Proposed UI

Comment 3

[Michael Lefevre]

Comment on attachment 144763 [details]
Proposed UI

just to be clear this isn't  a patch, it's a design for the UI

Comment 4

[Tim Powell]

You already get prompted about whether you want to install the XPI. I don't see
any great advantage to adding yet another step. It sounds like this bug is
asking to block the XPI feature, which I think is a bad idea. Just because some
extensions or software installs can be malicious, doesn't mean that you should
get rid of the feature.

Comment 5

[Ben Basson]

The bug is not asking to block XPInstall entirely, simply to stop it being
activated automatically when navigating to a website with the onload command. It
should probably be blocked for stuff like onmouseover and onmouseout too.

Malicious sites know that people hate prompts, and they have two choices:
1) Provide a link which the user must choose to click on, whether disguised or
not, nothing can be done about this.
2) Bombard the user with prompts to install until they finally give in or go away.

This is not desirable, and if it can be supressed, I see no reason not to do so.
It will allow legitimate and requested installs.

Comment 6

[Dan M]

Tim (comment 4): How many people install spyware on their machine by carelessly
smacking the return/OK key when confronted with 20 scrolling pages of "would you
like to install a malicious ActiveX control now" User Agreement dialog? I
wouldn't. You wouldn't. But I'm thinking that's where almost every browser
infection comes from. Now that someone has figured out how to extend the
practice to Mozilla, it's past time we did something about it, to avoid the
ActiveX embarrassment.

Roope suggests we prevent xpi installation from being triggered without an
explicit action from the user. It's a fantastic idea and we're dummies for not
having done it already.

Comment 7

[Bradley Chapman (not reading bugmail, still gone but not forever)]

I agree with you completely, danm - in fact, it almost seems like a compliment
that the spyware developers are beginning to target Mozilla ;-)

Could the unused digital signature functionality present in the current XPI
installer be leveraged as part of this effort as well? Or is that a separate bug?

Comment 8

[Daniel Veditz [:dveditz]]

I'll take this one

Comment 9

[Unknown W. Brackets]

I would like to make a simple suggestion: make this a pref just like everything
else like it.  At least accessable from about:config.

The benefit of this is that advanced users, who know what they are doing, can
turn this off if they even want to.  And the people who just want any dialog
that pops up to *go away asap* will probably never see the pref.

-[Unknown]

Comment 10

[Kevin Brosnan]

This bug will need to wait on the new extension manager bug 170006. No sense in
writing some code then have the extension manager change drastically when bug
170006 is checked in.

Comment 11

[Pascal Chevrel:pascalc]

Do you mean that an extension manager will be provided to Seamonkey too ?
Because bug 170006 is firefox specific while this problem concerns seomonkey too.

Comment 12

[Benjamin Smedberg]

Don't worry about the new-toolkit extension manager... this won't conflict with
that, and we'll use the same logic for extension installs.

Comment 13

[Kevin Brosnan]

Sorry, I thought this was filed as a FireFox bug so my comment 10 is moot.

Comment 14

[Mikko Virkkilä]

Could we handle .xpi files like we handle executables? We would only allow the
user to download it. Then when the user runs 'mozilla spy_ware.xpi' the xpi
would get installed.

If this is found too cumbersome for users, we could always have a whitelist
where trusted sites would be listed or just hardcode mozilla.org as the only
domain from where the user is allowed to directly install xpis.

Comment 15

[Bradley Chapman (not reading bugmail, still gone but not forever)]

No, that would be too restrictive IMHO. I think the best thing to do is exactly
what the bug summary says: prevent the XPI code from being called via an onload
page event. 

Someone on MozillaZine mentioned a number of extension-related checkins into the
trunk; did those checkins include anything related to this bug?

Comment 16

[Michael Lefevre]

No, those checkins relate to the stuff discussed in comments 10 to 13, which
don't have any impact one way or the other on this issue.

Please (not aimed at anyone in particular) keep comments focused on the specific
issue this bug is about - blocking XPI-installs fired by onload (or other events
not deliberately triggered by the user).

Comment 17

[Robert Parenton]

Tweaked summary to cover other events non-intentionally triggered by the user.

Comment 18

[Roope Lehmuslehto]

Another one:
http://forums.mozillazine.org/viewtopic.php?t=66531

Doesn't work anymore though, but looks like that this issue needs to be dealt fast.

Should block 1.7 because "Mozilla 1.7 to Become New Long-Lived Stable Branch".
And how it's dealt in Firefox? Blocking Fx 0.9?

Btw, can Thunderbird install xpi's from mail message? Just a thought...

Comment 19

[MW]

is http://bugzilla.mozilla.org/show_bug.cgi?id=234068 a dupe of this or related

Comment 20

[Robert Parenton]

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

Comment 21

[MW]

sorry to spam.... but can't
http://bugzilla.mozilla.org/show_bug.cgi?id=234068#c2 be a simple implementable
solution 

Comment 22

[Kevin Brosnan]

bug 14338 | Make XPInstall Prefs more granular seems to be related to this bug.
If the solution ends up being the addition of prefs like
accept_XPInstall_on_load [boolean]. 

Comment 23

[jay garcia]

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

Comment 24

[Ben Basson]

Nominating as blocker, since these issues are starting to pop up more
commonly... they need to be stopped asap.

An ActiveX installer issue was featured on BBC Watchdog recently, where the
script hijacked the dialup settings, changing to a premium rate number. While
nothing this serious has been produced for Mozilla yet, we shouldn't wait until
it is before reacting.

I'd go so far as to say that you could implement UI for this as a seperate bug,
although prefs would possibly be a requirement for advanced users. I'd also
recommend a higher severity, because potentially it could become one, very quickly.

Comment 25

[R. A. Cooper]

Below is the thread with my initial post on this matter (for another example):
http://www.spywareinfo.com/forums/index.php?s=f9fd781cf71e08ce693a1c328a5e1fc1&showtopic=40247&st=0&#entry205983

I agree the blocking OnLoad etc events to install XPI files would be the best
solution.  JavaScript blocking already had the ability to block those; why not
the installer as well?

Comment 26

[Dan M]

The practice is spreading. As reported in mozillazine, www.animewallpapers.com
attempts to install a .xpi on load now, too. It was rather insistent with me,
attempting a second time as soon as I hit cancel. Their .xpi consists solely of
an .exe and a small script that executes it. Judging from the URL at which the
.xpi lurks, it installs flingstone adware crap. Bumping severity.

Comment 27

[Brendan Eich [:brendan]]

Blocking 1.7.  ETA on patch?

/be

Comment 28

[Daniel Veditz [:dveditz]]

Attachment: blocks onload installs

dang--forgot to attach diff. No wonder I haven't gotten any reviews yet!

This blocks simple abuse. As danm points out abusers will get creative but we
will have bug 240552 to deal with that (all sites will default to blocked).

Comment 29

[Ben Basson]

Does this patch deal with other events too? I think mouseover and mouseout are
just as dangerous as onload, and they're used to attempt to circumvent popup
blocking systems currently.

Comment 30

[Benjamin Smedberg]

Comment on attachment 146595 [details] [diff] [review]
blocks onload installs

>Index: mozilla/xpinstall/src/nsXPInstallManager.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v
>retrieving revision 1.122
>diff -u -p -r1.122 nsXPInstallManager.cpp
>--- mozilla/xpinstall/src/nsXPInstallManager.cpp	13 Mar 2004 00:25:34 -0000	1.122
>+++ mozilla/xpinstall/src/nsXPInstallManager.cpp	20 Apr 2004 16:38:30 -0000

>+    if (piWindow)
>+        piWindow->IsLoadingOrRunningTimeout(&isPageLoading);
>+
>+    if (isPageLoading)
>+        rv = NS_ERROR_FAILURE;

How about using NS_ERROR_DOM_SECURITY_ERR

>+        NS_NewURI(getter_AddRefs(uri), NS_ConvertUCS2toUTF8(item->mURL.get()).get());

Wow, that looks ugly. There are forms of NS_NewURI that take ACStrings so you
don't have to do all the .get() stuff.

>+        nsIStreamListener* listener = new CertReader(uri, nsnull, this);
>+        if (listener)
>+        {
>+            NS_ADDREF(listener);
>+            rv = NS_OpenURI(listener, nsnull, uri);
>+            NS_RELEASE(listener);
>+        }

You can also assign directly into a COMPtr, and avoid the NS_ADDREF/RELEASE
pair.

Other than those minor nits, looks good!

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 146595 [details] [diff] [review]
blocks onload installs

> NS_IMETHODIMP
> nsXPInstallManager::InitManager(nsIScriptGlobalObject* aGlobalObject, nsXPITriggerInfo* aTriggers, PRUint32 aChromeType)
> {
>+    if ( !aTriggers || aTriggers->Size() == 0 )
>+    {
>+        NS_WARNING("XPInstallManager called with no trigger info!");
>+        NS_RELEASE_THIS();
>+        return NS_ERROR_INVALID_POINTER;

Shutdown does this too, but how can it be right?  NS_RELEASE_THIS releases a
reference that the caller must have held in order to call InitManager, no?  Is
this just bad old code being shuffled?

Otherwise, what bsmedberg said.  Wouldn't hurt for danm to have a look too, at
this stage of 1.7.

/be

Comment 32

[jay garcia]

I assume (correctly I hope) that the fix for this bug will have no adverse
effect on clicking on "Install Theme", for instance, from Texturizer or one of
the other many theme or extension sites available.

Comment 33

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #32)
> I assume (correctly I hope) that the fix for this bug will have no adverse
> effect on clicking on "Install Theme", for instance, from Texturizer or one of
> the other many theme or extension sites available.
> 

You do assume correctly - the patch that has just been review+ is designed to
prevent the XPInstaller from being loaded via the onload, onmouseout, and
onmouseover page events (according to the summary).

But that sort of thing doesn't belong in this bug.

Comment 34

[Michael Lefevre]

AIUI, the patch, as its name indicates, blocks installs while the page is
loading.  As comment 28 says, bug 240552 should deal with the rest of the problem.

Comment 35

[Daniel Veditz [:dveditz]]

re comment 29 (cusser): see my check-in comment 28, this blocks simple abuse
only. Sufficient for now, easy and safe for this late in 1.7. Other bugs cover a
more complete solution.

re review comment 30 (bsmedberg): other than the return code (which I'll change)
the other nits were on shifted but otherwise untouched lines. I can clean those
up. I tend to skip cleanup I don't have to do when this close to a release, but
those /are/ pretty ugly.

re comment 31 (brendan): The original caller was a transient bit of javascript
on a page that might go away while someone on dialup was waiting for a download
to finish. The XPInstallManager needs to own itself.

re comment 32 (jaygarcia): this patch, no. bug 240552 will require install sites
to be explicitly whitelisted by the user. We may well include some popular
trusted sites on the default whitelist.

Comment 36

[Brendan Eich [:brendan]]

Comment on attachment 146595 [details] [diff] [review]
blocks onload installs

I can sr= for now, but I do not believe it's safe to NS_RELEASE_THIS() unless
you've done an NS_ADDREF_THIS() in the control flow leading up to the release. 
What guarantees that the JS reference is gone?	Why is there no double release
in that case, no matter who releases first (the second release would drive the
ref-count negative, if interpreted as signed).

Maybe I'm missing something in the XPInstall JS glue?

/be

Comment 37

[Daniel Veditz [:dveditz]]

The XPInstallManager constructor addrefs itself.

Comment 38

[Asa Dotzler [:asa]]

Comment on attachment 146595 [details] [diff] [review]
blocks onload installs

a=asa (on behalf of drivers) for checkin to 1.7.

Comment 39

[Daniel Veditz [:dveditz]]

Comment on attachment 146595 [details] [diff] [review]
blocks onload installs

a=asa for 1.7 (via IRC)

Comment 40

[Daniel Veditz [:dveditz]]

Checking in nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v <-- nsXPInstallManager.cpp
new revision: 1.122.2.1; previous revision: 1.122
done

Checking in nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v <-- nsXPInstallManager.cpp
new revision: 1.124; previous revision: 1.123
done

Fixed.

removing "privacy" keyword which doesn't quite fit.

Comment 41

[dan pritts]

I'm not sure that this matters given the proposed fix, but i'll throw my idea in
anyway.

It would be nice if you could permanently block a given XPI from being
installed.   I hit this music lyric site which tried to install the "content
access plugin" xpi - on *each* page load i had to deny it. 

It would have been better if I'd had the option to permanently deny it and not
have to worry about it any more.  

Comment 42

[malakai1911]

(In reply to comment #41)
> I'm not sure that this matters given the proposed fix, but i'll throw my idea 
in
> anyway.
> It would be nice if you could permanently block a given XPI from being
> installed.   

Internet Explorer has (i guess) a similar feature to what you propose called 
the ActiveX Kill bit. I think there should be "XPI Kill Bit's" enabled if at 
all possible.

Spyware blaster uses it. Its pretty nice.

Comment 43

[jeff oconnell]

(In reply to comment #28)
> Created an attachment (id=146595)
> blocks onload installs

one annoyance with this patch is that it passes the callback you've registered with
InstallTrigger.install() a status of -210, which is a nsInstall::USER_CANCELLED.
( http://lxr.mozilla.org/mozilla1.7/source/xpinstall/src/nsXPInstallManager.cpp#710 )

this makes it impossible to determine if a.) the user has actually clicked cancel, in which
case you may want to notify the user that some functionality may be lost, or b.) the install
has been blocked by the browser, in which case the web site owner will need to update
their pages to not attempt to install plugins via onLoad(), etc.

legitimate and honest pluging developers would love clear notification of what has prevented
their plugin from being installed.

similiarly, though un-related to this patch, if you set xpinstall.whitelist.enable.required to 'true',
javascrip calls to InstallTrigger.enabled() will return false.

in the past, this function only returned false if 'Software Installation' was disabled. 

again, overloading error codes and returned values only causes confusion for end-users 
and the developers trying to make their web-browsing experience as clear and intuitive
as possible.

Comment 44

[Víctor Fernández]

Another improvement might be to check in the contents of the XPI. If it contains
an .exe file, then reject it, perhaps showing a warning message to the user.

Comment 45

[Met - Martin Hassman]

(In reply to comment #44)
> If it contains an .exe file, then reject it

This would not be simple. The XPI can contain renamed exe and after extraction
renamed it back to exe.
And is is not only about *.exe files, also about *.js, *.vbs, *.wsh etc. Simple
file-suffix filter cannot handle it properly, I think.

Comment 46

[Daniel Veditz [:dveditz]]

This bug is fixed: stop commenting here!

Open new bugs (if you have specific bugs), or better, discussions belong in the
newsgroups.  news://news.mozilla.org/netscape.public.mozilla.xpinstall