Onload XPI installs should be blocked by default

RESOLVED FIXED

Status

defect
--
major
RESOLVED FIXED
15 years ago
4 years ago

People

(Reporter: archonon, Assigned: dveditz)

Tracking

({fixed1.7})

Bug Flags:
blocking1.7 +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments)

Reporter

Description

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

Comment 1

15 years ago
Reporter

Comment 2

15 years ago
Posted image Proposed UI

Comment 3

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

Comment 4

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

Comment 5

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

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

Comment 8

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

Comment 10

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

Comment 12

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

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

Comment 14

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

Comment 16

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

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

Comment 18

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

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

Comment 20

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

Comment 21

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

Comment 22

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

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

Comment 24

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

Comment 25

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

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

Comment 28

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

Updated

15 years ago
Attachment #146595 - Flags: superreview?(brendan)
Attachment #146595 - Flags: review?(bsmedberg)
Attachment #146595 - Flags: approval1.7?

Comment 29

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

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

Updated

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

Comment 32

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

Comment 34

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

Comment 35

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

Comment 37

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

Comment 39

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

a=asa for 1.7 (via IRC)
Attachment #146595 - Flags: approval1.7? → approval1.7+
Assignee

Comment 40

15 years ago
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
Last Resolved: 15 years ago
Keywords: privacyfixed1.7
Resolution: --- → FIXED

Comment 41

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

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

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

Updated

15 years ago
Flags: blocking-aviary1.0?

Updated

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

Assignee

Comment 46

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