241922 - Need an scriptable IDL interface to nsXPInstallManager

Status: RESOLVED FIXED

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

Description

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

I need to be able to invoke an install of various XPIs without showing the
standard UI, from chrome-privileged content only - this is in my update wizard
in Firefox. 

I am supplying a new interface - nsIXPInstallManager - which takes an array of
urls to install from, and a nsIXPIProgressDialog listener which receives install
notifications.

Comment 1

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

Attachment: patch to implement

Comment 2

[Brendan Eich [:brendan]]

Cc'ing people who can think about security, in addition to reviewing the patch.

/be

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

see bug 228812, which has a patch as well.

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 147217 [details] [diff] [review]
patch to implement

r/sr=dveditz

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 147217 [details] [diff] [review]
patch to implement

Sorry, revoking r=

This patch bypasses the install pref check. Since this is designed to work from
chrome we don't have to worry about per-site permission manager stuff, but if a
user has turned off updates, updates should be off.

Yes, evil chrome could change the pref first, evil chrome could do a lot of
things. Honoring the pref prevents future chrome users of this interface from
being accidentally evil if they don't know they need to check the pref.

Comment 6

[Daniel Veditz [:dveditz]]

This patch makes it significantly easier for chrome to install things without
imposing notification to the user. I do trust Ben's use and we can police any
future callers in the moz tree.

Note that this doesn't allow anything chrome couldn't already do in pieces,
we're not really any less safe. But it was harder before this. Not hard enough
to stop a malicious extension but hard enough to discourage legitimate use. (The
last step was not scriptable before, but not hard for a motivated attacker to
write a small binary component).

I'm uneasy, though. Not worried about intentional misuse but that bad extension
design might lead to exploits, and the ability to silently install things makes
a pretty juicy target.

Honoring the xpinstall.enable pref will at least give us emergency protection
should someone find a future exploitable extension and create a worm.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 147217 [details] [diff] [review]
patch to implement

>+NS_IMETHODIMP
>+nsXPInstallManager::InitManagerFromChrome(const PRUnichar **aURLs, PRUint32 aURLCount, 
>+                                          nsIXPIProgressDialog* aListener)
>+{
>+    mTriggers = new nsXPITriggerInfo();
>+    if (!mTriggers)
>+        return NS_ERROR_OUT_OF_MEMORY;
>+
>+    for (PRInt32 i = 0; i < aURLCount; ++i) {
>+        nsXPITriggerItem* item = new nsXPITriggerItem(0, aURLs[i], nsnull);
>+        if (item)
>+            mTriggers->Add(item);
>+        else {
>+            delete mTriggers; // nsXPITriggerInfo frees any alloc'ed nsXPITriggerItems
>+            mTriggers = nsnull;
>+            return NS_ERROR_OUT_OF_MEMORY;
>+        }
>+    }

The loop body control flow is arbitrarily backward -- it should cast out the
'if (!item)' error case, which is quite abnormal, with the early return in the
'then' clause, and the mTriggers->Add(item) indented normally, in the loop body
one level from the for/close-brace.

Nit, but I had to say something.  This kind of backwardness matters, especially
over time and when combined with more logic levels.

dveditz, what's the install check pref name?  Should we do more to defend in
depth than just have a pref to turn things off, if the balloon goes up?

/be

Comment 8

[chris hofmann]

- for 1.7.  at some point we may want to synch 1.7 and aviary 1.0 branches for
this but that can happen later...

Comment 9

[Dave Townsend [:mossop]]

This landed as part of the big aviary merge