Need an scriptable IDL interface to nsXPInstallManager

RESOLVED FIXED in mozilla1.7final

Status

P1
normal
RESOLVED FIXED
15 years ago
3 years ago

People

(Reporter: bugs, Assigned: bugs)

Tracking

Trunk
mozilla1.7final
Bug Flags:
blocking1.7 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-aviary1.0)

Attachments

(1 attachment)

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.
Attachment #147217 - Flags: superreview?(brendan)
Attachment #147217 - Flags: review?(dveditz)

Updated

15 years ago
Summary: Need an XPIDL interface to nsXPInstallManager → Need an scriptable IDL interface to nsXPInstallManager
Cc'ing people who can think about security, in addition to reviewing the patch.

/be
see bug 228812, which has a patch as well.
No longer blocks: 240553
Status: NEW → ASSIGNED
Flags: blocking1.7?
Priority: -- → P1
Target Milestone: --- → mozilla1.7final
Comment on attachment 147217 [details] [diff] [review]
patch to implement

r/sr=dveditz
Attachment #147217 - Flags: review?(dveditz) → review+
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.
Attachment #147217 - Flags: review+ → review-
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 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
Whiteboard: fixed-aviary1.0

Comment 8

15 years ago
- 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...
Flags: blocking1.7? → blocking1.7-

Updated

11 years ago
Attachment #147217 - Flags: superreview?(brendan)
This landed as part of the big aviary merge
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 228812

Updated

9 years ago
QA Contact: xpi-engine
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.