179573 - Active-X plugin prefs

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Peter Lubczynski]

This bug is being open to track any special preferences for the ActiveX plugin.

One desired preference is the ability to disable downloading/installing of
controls but still allow running exsiting installed controls.

Comment 1

[Adam Lock]

Off the top of my head we need prefs for:

Disable hosting of controls (i.e. activex plugin does nothing at all)
Enable hosting controls not marked safe for scripting
Disable scripting of controls marked safe for scripting
Enable scripting of controls (in general)
Disable download and installation of controls
Enable download and installation of controls not marked safe for scripting

Some of these are probably the same pref but with different values

Comment 2

[rubydoo123]

dropping this into 1.3 beta and adding the appropriate markers

Comment 3

[Adam Lock]

The ActiveX plugin already has a PrefObserver so I'm getting it in shape for
this. So far I believe the flags I'll need will be these:

const PRUint32 HOSTING_FLAGS_HOST_NOTHING          = 0x00000000;
const PRUint32 HOSTING_FLAGS_HOST_SAFE_OBJECTS     = 1;
const PRUint32 HOSTING_FLAGS_HOST_UNSAFE_OBJECTS   = 2;
const PRUint32 HOSTING_FLAGS_DOWNLOAD_CONTROLS     = 4;
const PRUint32 HOSTING_FLAGS_SCRIPT_SAFE_OBJECTS   = 8;
const PRUint32 HOSTING_FLAGS_SCRIPT_UNSAFE_OBJECTS = 16;

The pref where these will be stored is nominally called
"security.xpconnect.activex.general.hosting_flags"" and probably serve mine and
David's requirements.

Comment 4

[Adam Lock]

Attachment: Work in progress

Patch adds plugin code to to determine which controls it is allowed to host and
script.

Comment 5

[Peter Lubczynski]

Since plugins are enabled by default in mail/news, we also need a pref to
disable Active-X in mail/news.

Comment 6

[Adam Lock]

Attachment: Work in progress 2

A big second patch. This adds a methods such as IsObjectSafeForScripting to
nsIDispatchSupport to share safe for scripting checks between the plugin and
COM connect and changes the code in both places to use them. The IDL also has a
GetHostingFlags which returns a bunch of flags controlling the various
behaviours. 

The principle changes to XPConnect involve throwing away all the category
checking stuff in favour of the service. I've also changed the meaning of a
bool arg on nsIDispatchSupport::CreateObject from test safe for scripting to
enforce security.

On the plugin side of things I've implemented an xpc_GetHostingFlags and a pref
observer class. The LegacyPlugin.cpp and XPConnect.cpp files now use the
nsIDispatchSupport service too. I have to add one more check to add yet to
ensure the newly created control is really safe for hosting. I expect to do one
more patch at least since I've only semi-tested the changes so far.

David can you have a look at the COM Connect and the new IDL to make sure I'm
doing the right thing here?

Comment 7

[David Bradley]

I wonder how expensive the pref check is? I wonder if we shouldn't cache the
value, and make it something people have to restart the client in order to
change. I also have the IsIDispatchEnabled method on the nsXPConnect that I had
planned to use for this check. That either needs to be hooked in or removed all
together. We don't need two things mechanisms to confuse things. There is
another entry point on the COM to JS side, where a JSObject is viewed through
the IDispatch interface. That code uses the the IsIDispatchEnabled call to
decide if to even bother to check for a QI to IDispatch. If this is not going to
be cached and expensive then that check should be removed. I don't think having
that enabled all the time presents any problems, since you'd need a COM/ActiveX
object to even get that far, and you couldn't do that without ActiveXObject working.

I'm not sure it's such a good idea to use xpc_ prefix in your code. This is a
prefix used in XPConnect to define things that are private to the DLL, but
shared among the files of XPConnect.

And in nsDispatchSupport::IsClassSafeToHost, in one open use use a string
constant directly, and others you declare the constants as variables. Probably
should do it the same way in all cases, for consistency. 

Also the string constants you declared the char's const but not the pointers.
Probably should declare them as const TCHAR/char * const.  That way the compiler
can be sure that pointer won't be changed.

I already have a function in the XPCDispInlines.h, which will be named
XPCDispnsCID2CLSID after my patch, which does pretty much the same thing as
nsID2UUID. This should be changed to return a UUID instead of a GUID, and the
name of my functions should be changed as well.

Trivial style issues, in XPConnect code there isn't any space between if and (,
or for (, etc. it's if(. And single line statements in if/else statements aren't
surrounded by braces.

Why not create a helper function for the code in the
nsDispatchSupport::IsClassSafeToHost, example below (uncompiled hack)

CControlSite's constructor makes me wince a bit. I just always like initializer
lists to avoid initing data twice, but these all appear to be POD's so it's not
a big deal.

static PRBool ControlIsListed(const CLSID & clsid, const TCHAR * controlKey,
PRBool & found)
{
    found = FALSE;
    CRegKey keyDeny;
    if (keyDeny.Open(HKEY_LOCAL_MACHINE, controlKey, KEY_READ) == ERROR_SUCCESS)
    {
        // Enumerate CLSIDs looking for this one
        int i = 0;
        do {
            USES_CONVERSION;
            TCHAR szCLSID[64];
            const DWORD nLength = sizeof(szCLSID) / sizeof(szCLSID[0]);
            if (::RegEnumKey(keyDeny, i++, szCLSID, nLength) != ERROR_SUCCESS)
            {
                return PR_FALSE;
            }
            szCLSID[nLength - 1] = TCHAR('\0');
            CLSID clsidToCompare = GUID_NULL;
            if (FAILED(::CLSIDFromString(T2OLE(szCLSID), &clsidToCompare))
            {
                return PR_FALSE;
            }
            else if(::IsEqualCLSID(clsid, clsidToCompare))
            {
                found = PR_TRUE;
                return PR_TRUE;
            }
        } while (1);
        keyDeny.Close();
    }
    return PR_TRUE;
}

Comment 8

[Adam Lock]

I'll submit a patch that addresses most of these issues, however there are a
couple of things I need clarification on:

1. IsIDispatchEnabled is for global activation / deactivation of the COM
support. My patch just covers prefs for hosting object instances and how they
are scripted. Do you want an explicit master switch pref that
IsIDispatchEnabled() initially reads from?

2. Caching prefs is a little complicated. The
nsIDispatchSupport::GetHostingFlags accepts a string value which allows the
caller to ask for the hosting flags in a particular context. For example, if the
caller wants "mailnews" hosting flags then GetHostingFlags reads
"security.xpconnect.activex.mailnews.hosting_flags". The default context is
"global" and I could cache that, but other contexts might have to be read on the
fly. Actually I prefer not doing this either since that would stop safe for
scripting flags being changed dynamically by the user. I could make
nsIDispatchSupport into a pref nsIObserver and update the global pref that way.
Would this be acceptable?

3. A note to myself - I still have to add code to LegacyPlugin.cpp to kill a
control if it is instantiated and doesn't meet the hosting flag criteria.

Comment 9

[David Bradley]

>1. IsIDispatchEnabled is for global activation / deactivation of the COM
>support. My patch just covers prefs for hosting object instances and how they
>are scripted. Do you want an explicit master switch pref that
>IsIDispatchEnabled() initially reads from?

That's a good point, I'm just worried this is getting a little complex. I guess
I can see a need for an independant on/off switch based on some other pref or
configuration option. So I guess leave this as is.

2. Since I've agreed with 1. I don't this is as big an issue, since it is only
called for ActiveX object construction which will be pretty costly anyway. And I
agree with the points you site.

Comment 10

[Adam Lock]

Attachment: Patch part 1

Updated patch for the XPCOM stuff.

Comment 11

[Adam Lock]

Attachment: Patch part 2

This patch contains the plugin work. Addresses comments and cleans up
LegacyPlugin.cpp. The changes overlapped a little with work for merging control
& plugin IWebBrowser impls, so I omitted a small diff to XPDocument.cpp that
renamed xpc_GetServiceProvider to MozAxPlugin::GetServiceProvider.

Both patches have had their default hosting flag values locked down so by
default controls can neither be hosted or scripted. This pref enables
everything:

pref("security.xpconnect.activex.global.hosting_flags", 31);

Comment 12

[David Bradley]

Comment on attachment 108044 [details] [diff] [review]
Patch part 1

r=dbradley

If you think of it when you check in, you could make these pointers const as
well

+    const char *kActiveXHostingPref1 = "security.xpconnect.activex.";
+    const char *kActiveXHostingDefaultContext = "global";
+    const char *kActiveXHostingPref2 = ".hosting_flags";

Comment 13

[Adam Lock]

Comment on attachment 108044 [details] [diff] [review]
Patch part 1

Brendan, can you sr this please?

I'll sort out the const issue mentioned by David at checkin.

Comment 14

[Adam Lock]

Comment on attachment 108046 [details] [diff] [review]
Patch part 2

Rick can you sr this please. This patch makes the plugin use the
nsIDispatchSupport safe for scripting checks when XPC_IDISPATCH_SUPPORT is
defined.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 108044 [details] [diff] [review]
Patch part 1

>+++ mozilla/js/src/xpconnect/idl/nsIDispatchSupport.idl	3 Dec 2002 17:30:46 -0000
>@@ -39,6 +39,7 @@
> #include "nsIVariant.idl"
> 
> %{ C++
>+#include "jsapi.h"

Why is this needed?  Could you get by with only jspubtd.h, or even nothing
(forcing only some, not all, C++ includers to include a js*.h file explicitly)?

>     /**
>-     * Instantiate a COM component
>-     * This checks to see if the component is scriptable. If it is, it instantiates it
>-     * @param className contract ID or class ID of the desired component to instantiate 
>-     * @return The IDispatch interface pointer if instantiated
>+     * Instantiate a COM component.

Why lose the @param and @return doc-comments rather than fix them?

>      */

Nit, here and throughout (this style prevailed, but if dbradley agrees, fix
it): an extra newline between doc-commented method declarations would make the
file seem less crowded vertically, aiding readers like me.

>-    IDispatch CreateInstance(in string className, in PRBool testScriptability);
>+    IDispatch createInstance(in string className);
>+    /**
>+     * Test if the class is safe to host.
>+     * @param clsid The nsID representation of the CLSID to test.
>+     * @param classExists Returns containing PR_FALSE if the class is not registered.
>+     */
>+    boolean isClassSafeToHost(in nsCIDRef cid, out boolean classExists);
>+    /**
>+     * Test if the specified class is marked safe for scripting.
>+     * @param clsid The nsID representation of the CLSID to test.

The formal name below is cid, not clsid.

>+     * @param classExists Returns containing PR_FALSE if the class is not registered.
>+     */
>+    boolean isClassMarkedSafeForScripting(in nsCIDRef cid, out boolean classExists);
>+    /**
>+     * Test if the instantiated object is safe for scripting on the specified interface.

Nit: does xpconnect coding style respect the 80th column as the limit?

>+     * @param theObject The object to test (an IUnknown cast into a void *).
>+     * @param iid The interface to test if it is safe for scripting on.
>+     */
>+    boolean isObjectSafeForScripting(in voidPtr theObject, in nsIIDRef id);
>+
>+    /** Host nothing at all */
>+    const unsigned long HOSTING_FLAGS_HOST_NOTHING          = 0x00000000;
>+    /** Allow hosting of safe for scripting objects */
>+    const unsigned long HOSTING_FLAGS_HOST_SAFE_OBJECTS     = 1;
>+    /** Allow any object to be hosted*/

Nit: space before */.

>+    const unsigned long HOSTING_FLAGS_HOST_ALL_OBJECTS      = 2;
>+    /** Allow objects to be downloaded and installed */
>+    const unsigned long HOSTING_FLAGS_DOWNLOAD_CONTROLS     = 4;
>+    /** Allow objects marked safe for scripting to be scripted */
>+    const unsigned long HOSTING_FLAGS_SCRIPT_SAFE_OBJECTS   = 8;
>+    /** Allow any object to be scripted */
>+    const unsigned long HOSTING_FLAGS_SCRIPT_ALL_OBJECTS    = 16;

Nit: why use 0x00000000 for NOTHING, then decimal for the flags?  Also, maybe
use PRUint32 instead of unsigned long in IDL (requires nsrootidl.idl)?

>+++ mozilla/js/src/xpconnect/src/Makefile.in	3 Dec 2002 17:30:47 -0000
>@@ -54,6 +54,7 @@
> 		  string \
> 		  js \
> 		  caps \
>+		  pref \

Is there any way to avoid this?  Mozilla shouldn't be a ball of string where
the more primitive layers depend on the less primitive ones.  If I want to use
xpcom and xpconnect in an apache module, I probably don't want to drag pref
along.	jband took pains over a long course of development to avoid making
xpconnect depend on pref and profile in the non-DEBUG case.  We should find a
better way than to misorder (fully connect) dependencies and (to boot) make it
that much harder for apache module authors (they exist) to use xpcom and
xpconnect with js and nspr, but nothing else.

Alas, caps already got entrained at some point.  I believe the only reason, and
not a good one, was so nsISecurityCheckedComponent.h could come from a .idl
file that lives under caps/idl.  This is strange in view of the effort taken in
the "other direction" to derive nsIScriptSecurityManager (a caps extension)
from nsIXPCSecurityManager (the most-primitive xpconnect base class).  As caps
is less primitive than xpconnect, this nsISecurityCheckedComponent gaffe should
be fixed.  I've filed bug 183321 on that.

>+    USES_CONVERSION;

rs=brendan on all the Windows-specific stuff, but can you educate me about what
this macro does?  I'm curious; thanks.

>+    HRESULT hr = CoCreateInstance(CLSID_StdComponentCategoriesMgr, NULL,
>+        CLSCTX_INPROC_SERVER, __uuidof(ICatInformation), (LPVOID*) &catInfo);

Nit: does prevailing style indent arguments that overflow the first line of a
call expression to line up with the first argument?

>+    // Test the Internet Explorer black list
>+    const TCHAR * const kIEControlsBlacklist = _T("SOFTWARE\\Microsoft\\Internet Explorer\\ActiveX Compatibility");

This and other string constants should be declared const TCHAR [] if possible. 
Why waste four bytes on a const pointer to const chars?

>+    const char *kActiveXHostingPref1 = "security.xpconnect.activex.";
>+    const char *kActiveXHostingDefaultContext = "global";
>+    const char *kActiveXHostingPref2 = ".hosting_flags";

These should be const char arrays too, I think.

>+    // Create the pref name that is being requested
>+    nsCAutoString prefName(kActiveXHostingPref1);
>+    prefName += aContext ? aContext : kActiveXHostingDefaultContext;
>+    prefName += kActiveXHostingPref2;
>+
>+    // TODO it would be more efficient to observe pref changes, but this will
>+    // do for the time being.
>+    nsresult rv;
>+    nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+    if(NS_SUCCEEDED(rv) && prefs)
>+    {
>+        PRInt32 hostingFlags = kDefaultHostingFlags;
>+        rv = prefs->GetIntPref(prefName.get(), &hostingFlags);
>+        if(SUCCEEDED(rv))
>+        {
>+            *_retval = (PRUint32) hostingFlags;
>+            return NS_OK;
>+        }
>+    }

If this is the only place where prefs are used, move it to a less primitive
layer that cares, e.g. the embedding/activex layer, and call it via a more
primitive interface that xpconnect provides.

Nits are just suggestions, but I think the pref dependency should be avoided --
I'll sr= a patch that does that.

/be

Comment 16

[Adam Lock]

I could try jspubtd.h instead. It's required since the interface passes uses
jsval types.

Nits with comments will be fixed.

I used 0x00000000 to make it clear it meant all bits unset.

I'm not sure to do with the pref requirement. The COM connect as well as the
plugin needs to read this pref to ensure that only objects meeting the current
hosting flags are scriptable.

USES_CONVERSION is a macro used in conjunction with the T2OLE call to hold any
temporary variables when used for converting TCHARs into Unicode. TCHARs are
used by many of the Win32 APIs but CLSIDFromString takes an (wide) OLECHAR string.

consts will be corrected.

So the big problem appears to be the pref. Unless the COMObject and
ActiveXObject and nsDispatchSupport service code can be moved out of xpconnect I
think this stuff would have to stay, or something somewhere will have to call
the service at initialisation and explicitly set the value.

Comment 17

[Adam Lock]

Attachment: Patch part 1 updated

First patch updated to resolve most of the nits, comments, const declarations
etc.

The outstanding issue is the inclusion of prefs.

Comment 18

[David Bradley]

The pref's dependency had bothered me as well, especially when I was thinking of
it being used earlier in the startup code than it is now.

Other than Adam's solution, the only thing I can think of is to move the
ActiveXObject constructor code along with the nsIDispatchSupport code into its
own module. That module would then depend on prefs, leaving XPConnect with the
original XPConnect code and the code to invoke IDispatch methods.

Comment 19

[Adam Lock]

Attachment: Patch part 1 with prefs #if'd out

This is the same patch with the pref code #if 0'd out and the dependency
removed. The returned hosting flags are taken from the kDefaultHostingFlags
const.

Comment 20

[David Bradley]

Comment on attachment 108208 [details] [diff] [review]
Patch part 1 with prefs #if'd out

The patch is fine. There's still the question of when should we deal with the
prefs issue. I really hate to muck with my existing patch, I'd like to finish
the review and get it in. And then work on the part that Bug 183507 talks
about.

I'll also need to deal with the conflicts between Adam's patch and mine as
well.

Comment 21

[Peter Lubczynski]

reassign--->Adam's got the patch

Comment 22

[Brendan Eich [:brendan]]

Was my suggestion "If this is the only place where prefs are used, move it to a
less primitive layer that cares, e.g. the embedding/activex layer, and call it
via a more primitive interface that xpconnect provides" unclear, or unworkable?

You don't need yet another module (alecf will get you if you add a new dll; each
has considerable footprint cost).

Just factor and abstract slightly.  Maybe there's an existing interface that
xpconnect (or comconnect) provides that pref-dependent activex code could
implement.  Interfaces are cheap if used at cross-module boundaries with low
dynamic call frequency.

/be

Comment 23

[David Bradley]

I should have read your entire post, I stopped with the nits, and assumed you
had finished your input regarding prefs.

For my education, wouldn't a const char * const be treated more like an alias,
like const int, and not really take up any extra space? I got in this habbit
from a previous company's style, but trying to retrain myself to use array, I
like the notation better.

ActiveXObject may get called in the absence of any plugin on a page. I don't
know if Adam's code gets called when Mozilla starts up. If it does, then we
might be able to do as you suggest. If it doesn't then I don't think that will
work. Adam?

Comment 24

[Adam Lock]

Attachment: Patch part 1 again

Patch for XPConnect again. I've added an nsIActiveXSecurityPolicy interface
with a single getHostingFlags methods. The nsIDispatchSupport implementation
attempts to do_GetService for the policy object and thus removes the need for
the pref.

The ActiveX policy object can be implemented elsewhere, even in JS if desired.
I'll attach a sample of that next.

Comment 25

[Adam Lock]

Attachment: Sample ActiveX security policy object

A sample ActiveX policy object, written in JS.

Comment 26

[Adam Lock]

Comment on attachment 108407 [details] [diff] [review]
Patch part 1 again

sr=?

Pref dependency removed. The nsIDispatchSupport object asks the
nsIActiveXSecurityPolicy service (if there is one) for the hosting flags. For
the time being we can make do with a js impl of this object.

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 108407 [details] [diff] [review]
Patch part 1 again

Thanks, sorry I didn't dig more into the activex code.	I was wondering if
you'd have to make a service, or whether there weren't some existing object you
could extend. Given the need for a new object, a JS service impl is the way to
go so long as calling into it is not done all the time, and not otherwise on
any critical path.

/be

Comment 28

[David Bradley]

Comment on attachment 108407 [details] [diff] [review]
Patch part 1 again

r=dbradley

Comment 29

[David Bradley]

Comment on attachment 108046 [details] [diff] [review]
Patch part 2

r=dbradley

If m_pSecurityPolicy is null (allocation failed) should the default answer to
the security questions be yes?

Comment 30

[Adam Lock]

I was thinking that is there was no security policy service we go into complete
lockdown and don't allow anything. In normal use there will be a policy object
however, but erring on the side of caution seems right where activex is concerned.

BTW part 2 of the patch will be submitted soon and will contain the js object as
part of the plugin.

Comment 31

[David Bradley]

I'm not sure we're talking about the same thing. The comment was reguarding part 2:

+BOOL CControlSite::IsClassSafeToHost(const CLSID & clsid)
+{
+    if (m_pSecurityPolicy)
+        return m_pSecurityPolicy->IsClassSafeToHost(clsid);
+    return TRUE;
+}

I just wanted to double check that TRUE was the right default answer.

So you'll have another update to the second part, will that be identical to the
existing Part 2 patch, just with the JS source added?

Comment 32

[Adam Lock]

I was thinking of something else :) The control site's IsClassSafeToHost method
won't be called if XPC_IDISPATCH_SUPPORTS is defined so it should be safe.

The updated patch will contain the JS, #include nsIActiveXSecurityPolicy.h and
cleans up some extern declarations but is otherwise unchanged.

Comment 33

[Adam Lock]

Attachment: Patch part 2 updated

Patch 2 updated for nsIActiveXSecurityPolicy. Includes the new
nsAxSecurityPolicy.js component and changes to makefile to install it.

Comment 34

[Asa Dotzler [:asa]]

Does this need to make it into 1.3a or could it wait until after we relase (real
soon now). 

unrelated, but is the superreview request on patch part 2 still valid even
thought the patch has been resolved as obsolete? if not then can you unset the
request?

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 108407 [details] [diff] [review]
Patch part 1 again

a=asa for checkin to 1.3a

Comment 36

[Adam Lock]

Fix is checked in. Further work is required on the nsAxSecurityPolicy.js object
to read from prefs but I'll open another bug for that.