Closed Bug 238684 Opened 16 years ago Closed 16 years ago

Onload XPI installs should be blocked by default

Categories

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

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: archonon, Assigned: dveditz)

References

()

Details

(Keywords: fixed1.7)

Attachments

(3 files)

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:
Attached image Proposed UI
Comment on attachment 144763 [details]
Proposed UI

just to be clear this isn't  a patch, it's a design for the UI
Attachment #144763 - Attachment description: Proposed patch → Proposed UI
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.
Severity: critical → normal
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.
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.
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?
I'll take this one
Assignee: xpi-engine → dveditz+bmo
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]
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.
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.
Keywords: privacy
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.
Sorry, I thought this was filed as a FireFox bug so my comment 10 is moot.
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.
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?
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).
Tweaked summary to cover other events non-intentionally triggered by the user.
Summary: Onload XPI-installs should be blocked by default → Onload (and onmouseover, onmouseout, etc.) XPI installs should be blocked by default
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...
is http://bugzilla.mozilla.org/show_bug.cgi?id=234068 a dupe of this or related
*** Bug 234068 has been marked as a duplicate of this bug. ***
sorry to spam.... but can't
http://bugzilla.mozilla.org/show_bug.cgi?id=234068#c2 be a simple implementable
solution 

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]. 
*** Bug 240509 has been marked as a duplicate of this bug. ***
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.
Flags: blocking1.7?
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?
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.
Severity: normal → major
Blocking 1.7.  ETA on patch?

/be
Flags: blocking1.7? → blocking1.7+
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).
Attachment #146595 - Flags: superreview?(brendan)
Attachment #146595 - Flags: review?(bsmedberg)
Attachment #146595 - Flags: approval1.7?
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 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!
Attachment #146595 - Flags: review?(bsmedberg) → review+
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
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.
(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.
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.
Summary: Onload (and onmouseover, onmouseout, etc.) XPI installs should be blocked by default → Onload XPI installs should be blocked by default
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.
Status: NEW → ASSIGNED
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
Attachment #146595 - Flags: superreview?(brendan) → superreview+
The XPInstallManager constructor addrefs itself.
Comment on attachment 146595 [details] [diff] [review]
blocks onload installs

a=asa (on behalf of drivers) for checkin to 1.7.
Comment on attachment 146595 [details] [diff] [review]
blocks onload installs

a=asa for 1.7 (via IRC)
Attachment #146595 - Flags: approval1.7? → approval1.7+
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: privacyfixed1.7
Resolution: --- → FIXED
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.  
(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.
(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.
Flags: blocking-aviary1.0?
Flags: blocking-aviary1.0?
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.
(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.

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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.