188229 - [AxPlugin] Security policy should be per-domain configurable

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Adam Lock]

The nsAxSecurityPolicy object, nsIDispatchSupport::GetHostingFlags and
nsIActiveXSecurityPolicy::GetHostingFlags should allow the current domain to be
specified in the call in addition to the context string. The domain could be
supplied in the form of a nsIURI with nsnull denoting the default settings.

It should be reasonably straightforward for the plugin to obtain the current
domain through the NPAPI. I'm not sure how COM connect would manage it, but
presumably there is a information in the JS context that would allow it do the same.

Comment 1

[Adam Lock]

Attachment: Work in progress

Patch adds domain parameter to security policy, cleans up nsAxSecurityPolicy
and makes it a pref observer. I have yet to determine how per-domain settings
are stored.

Comment 2

[Adam Lock]

Attachment: Patch

Patch now allows per domain hosting flags via an rdf file specified in the
"security.xpconnect.activex.domain_list_uri" pref. The file can be stored
locally or remotely. The policy object will read this list in and look up
domains to determine if they have specific hosting flag values. If they don't
it will fall back on the global hosting flags value. I'll attach a sample RDF
file.

The global hosting flags value is read from the
"security.xpconnect.activex.global.hosting_flags" pref. In the absence of this
pref, the security policy object will return the kDefaultGlobalHostingFlags
const settings which can be tweaked by editting the js.

Comment 3

[Adam Lock]

Attachment: Sample rdf file

Sites requiring special hosting flags may be registered in a list like this.

Comment 4

[David Bradley]

Adam, I'm going to take this over since this should be satisfied by the security
manager code I've been working on.

Comment 5

[David Bradley]

Attachment: Added per domain security for IDispatch via caps

This uses caps to do the per domain security checks. I had to modify caps to
handle checking the CID. There was only base logic there for checking. This has
the side effect of not only checking IDispatch CID's but also XPConnect. This
check is only invoked if the CheckXPCPermission fails, so I think this
mitigates the performance impact. (And this is only done if IDispatch is
enabled).

So this patch consists of the changes to caps to check the security. An example
of the prefs entry:
user_pref("capability.policy.default.ClassID.{FA3662C3-B8E8-11D6-A667-0010B556D978}",
"allAccess");

This patch also adds logic to security check ActiveXObject and COMObject. An
example of this prefs entry is:
user_pref("capability.policy.default.Window.ActiveXObject", "noAccess");

And some cleanup in files touch to remove _retval, as names starting with an
underscore are reserved for the compiler.

Comment 6

[David Bradley]

Comment on attachment 115899 [details] [diff] [review]
Added per domain security for IDispatch via caps

Looking for reviews, I'll be asking Mitch as well.

Comment 7

[David Bradley]

Mitch I need to get your r= as well for the changes in caps and from a general
security issue.

Comment 8

[Adam Lock]

Comment on attachment 115899 [details] [diff] [review]
Added per domain security for IDispatch via caps

Here are some issues, a few of which I talked previously about via AIM:

1. The pref string for the classid includes the curly braces. It looks weird
even if the pref lib doesn't mind.

2. The caps check behaviour acts like a whitelist. Any control not listed is
denied access by default. It should be the other way around by default with
perhaps a pref to flip the behaviour from ACCEPT/DENY being the default.

3. Is the MozAxAutoPushJSContext in the plugin necessary? Is it possible to
obtain the docshell via nsIDOMWindow etc. from the plugin and obtain the
current script context from there? The docshell does implement the
nsIScriptGlobalObjectOwner interface and does supply a nsIScriptGlobalObject
via GetInterface too.

Comment 9

[Mitchell Stoltz (not reading bugmail)]

This patch seems to be duplicating a lot of code from CheckPropertyAccessImpl -
it would be great if we could refactor these two functions to avoid duplication.
I need to make some pretty significant changes to nsScriptSecurityManager
because of another bug, so I might as well incorporate your caps changes too.
I'm going to try to do that next week.

Pushing the context on the context stack is necessary before calling into caps
(if it isn't on the context stack already) since that's where caps looks for it.

Comment 10

[David Bradley]

Attachment: Pretty much same as before minus the caps part

This is the same patch as before minus CAPS. Mitch is going to work those
changes into a bug he is working on. If we can get this reviewed and have it
ready, I'll check it in after Mitch gets his patch checked in. Mitch also has
the changes to allow choosing default behavior of CID's not found (disallow vs
allow).

Comment 11

[David Bradley]

Comment on attachment 116196 [details] [diff] [review]
Pretty much same as before minus the caps part

Looking for reviews, see previous comment for details.

Comment 12

[Adam Lock]

Comment on attachment 116196 [details] [diff] [review]
Pretty much same as before minus the caps part

#include "nsIScriptGLobalObject.h".  Capitalized 'GL' in the middle should be
'Gl'. 

Is MozAxAutoPushJSContext needed now that you're grabbing the js context from
the window that the plugin is running inside?

Is the same call not required when MozAxPlugin::GetValue is called requesting
back the IDispatch scripting interface?

XPCDispObject.cpp should probably set its classExists and ok to default values
since it doesn't check the return code of IsClassSafeToHost to know whether
they are set by the call.

Does this code work without the work from Mitch? i.e. caps allows all controls
to be hosted and scripted.

Otherwise I don't have any issues.

Comment 13

[David Bradley]

I somehow missed the bugmail with Adam's comment. 

Yes MozAxAutoPushJSContext is needed so that we can push that context onto the
frame.

I think MozAxPlugin::GetValue may be calling the wrong function. It creates the
object and calls IsObjectSafeForScripting and then calls
IsClassMarkedSafeForScripting. So that leaves me wonder what the original intent
of the two functions, IsClassSafeToHost and IsClassMarkedSafeForScripting.
IsClassMarkedSafeForScripting seems to just test the category.

Comment 14

[Adam Lock]

I think I was confusing things myself concerning MozAxPlugin::GetValue. It
should call IsObjectSafeForScripting which is what it already does.

IsClassSafeToHost tests the CID is blacklist / whitelists to see if the object
can be created. IsClassMarkedSafeForScripting tests whether the CID is marked as
a safe for scripting object and IsObjectSafeForScripting tests the running
object for its IObjectSafety impl.

Some controls use categories for safe for scripting while others use the
interface which is where there are two methods. The IsObjectSafeForScripting
needs to be called after instantiating the control if the
IsClassMarkedSafeForScripting returned false before it was created.

Comment 15

[David Bradley]

Attachment: V3

New patch addressing Adam's issues.

As far as how this works without the caps changes, I think it probably blocks
everything. Originally it let everything through, but I wasn't using the proper
context. So I'm pretty sure now that the code is using the proper context it
would deny everything. In any case, this patch won't go in until the caps patch
goes. I talked with Mitch and it's ready, so should go in tomorrow.

Comment 16

[Adam Lock]

Comment on attachment 116825 [details] [diff] [review]
V3

r=adamlock with some minor nits

"*classExists = true" in nsDispatchSupport::IsClassSafeToHost should be
"*classExists = PR_TRUE"

XPCDispObject::COMCreateInstance should set "classExists" and "ok" to defaults
since the return code from the call is not checked.

Comment 17

[David Bradley]

I'll fix the true/PR_TRUE;

I thought I fixed the initialization in XPCDispObject::COMCreateInstance:

+    PRBool classExists = PR_FALSE;
+    PRBool ok = PR_FALSE;

Comment 18

[Adam Lock]

Sorry, I was looking at the wrong patch when I was checking for that.

Comment 19

[David Bradley]

Comment on attachment 116825 [details] [diff] [review]
V3

Alecf, jst, I think has been busy. Would you mind taking a look at this?

Comment 20

[Mitchell Stoltz (not reading bugmail)]

Attachment: New Caps ppart of the patch

Here's the Caps portion, with a few changes.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 116905 [details] [diff] [review]
New Caps ppart of the patch

>+    // Reformat the CID string so it's suitable for prefs
>+    nsCAutoString cidTemp(aCID.ToString());

This leaks the result of aCID.ToString(), maybe nsXPIDLCString::Adopt() would
help

>+    cidTemp.StripChars("{}");
>+    cidTemp.ReplaceChar('-','_');
>+    ToUpperCase(cidTemp);
>+    nsCAutoString cid("CID");
>+    cid += cidTemp;

You could stuff the results into a single AutoString before the StripChars etc,
avoiding an extra copy. And since you know the format of the cid instead of
copying the "{....}" and then shifting over one to strip the "{" you could use
a substring directly. Something like (not compiled or tested)...

  nsXPIDLCString cidTemp;
  cidTemp.Adopt(aCID.ToString());
  nsCAutoString cid( NS_LITERAL_CSTRING("CID") + Substring(cidTemp, 1,
cidTemp.Length()-2 );
  cid.ReplaceChar('-','_');
  ToUpperCase(cid);

I don't know how often this will get called, but our performance in general is
pretty heavily dinged by extra string allocation and manipulation.

Comment 22

[Mitchell Stoltz (not reading bugmail)]

Attachment: Caps patch v3 - addresses dveditz's comments

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 116914 [details] [diff] [review]
Caps patch v3 - addresses dveditz's comments

r=dveditz

Comment 24

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 116914 [details] [diff] [review]
Caps patch v3 - addresses dveditz's comments

Verbal sr from heikki

Comment 25

[Mitchell Stoltz (not reading bugmail)]

I've checked the caps patch in.

Comment 26

[Alec Flett]

Comment on attachment 116825 [details] [diff] [review]
V3

sr=alecf

except you're using 'true' when you should be using PR_TRUE (not all compilers
support 'true')

Comment 27

[David Bradley]

V3 patch checked in