184491 - Security Manager blocks invoking methods on some IDispatch based objects

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P1)

Description

[David Bradley]

XPConnect is blocked from invoking methods on objects that are wrapped by the
parameter conversion logic. This doesn't seem to be a problem for objects that
were created from the plugin, as they appear to be treated as DOM objects. The
proposed solution is to provide class info for wrapped IDispatch objects.

Comment 1

[David Bradley]

Attachment: Adds class info to IDispatch based objects

Ok, I think I got this right. Mitch can you take a look and just make sure I
didn't open any huge holes. This should just result in same origin access being
used on IDispatch objects.

Comment 2

[David Bradley]

Comment on attachment 108876 [details] [diff] [review]
Adds class info to IDispatch based objects

I'm not going to peg any one person for the review. I need mstoltz's input. And
Mitch if you want to r= that would be great, but I need to at least know from a
security standpoint if this is correct. Adam, or someone else can give a formal
r= of the code instead.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 108876 [details] [diff] [review]
Adds class info to IDispatch based objects

You probably want to return a string for GetClassDescription, not nsnull, since
caps uses the class description as an ID for setting policies for a class.
Unless GetClassDescription is going to be overridden, it should probably return
"IDispatch".

Comment 4

[David Bradley]

Attachment: Same as before but GetClassDescription returns a string

Mitch, I emulated what I saw elsewhere, there are other implementations of
nsIClassInfo that return null as well. Don't know if they are problems or not.

Comment 5

[Adam Lock]

Comment on attachment 109025 [details] [diff] [review]
Same as before but GetClassDescription returns a string

r=adamlock except for a couple of nits.

It seems pointless to say "1 * sizeof(nsIID*)"

Is nsIProgrammingLanguage::UNKNOWN the correct response in
GetImplementationLanguage? COM connect is C++ even if the object it is wrapping
may not be. It doesn't look as though anyone calls this method so maybe it
doesn't matter.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 109025 [details] [diff] [review]
Same as before but GetClassDescription returns a string

- In src/xpcwrappednative.cpp:

+#ifdef XPC_IDISPATCH_SUPPORT
+    // If this is an IDispatch wrapper and it didn't give us a class info
+    // we'll provide a default one
+    if (isIDispatch && !info)
+    {
+	 info = new XPCIDispatchClassInfo;

Don't you want this to be globally cached (one-item size cache, at least)? How
often will we hit this, do we need to malloc a new one for every wrapped
IDispatch object?

Comment 7

[David Bradley]

jst, yes, you're right! there's no reason not to cache this as far as I can tell.

adam, I originally had this as C++. I was just thinking that since we don't know
the true target language it might be best for it to be unknown. But I think
you're argument is valid, that as far as security goes, this probably should be
considered C++.

Comment 8

[Mitchell Stoltz (not reading bugmail)]

> Mitch, I emulated what I saw elsewhere, there are other implementations of
> nsIClassInfo that return null as well. Don't know if they are problems or not.

David, the other uses of nsIClassInfo are on classes that were never intended to
be scriptable from content, and I'm guessing your code is intended to be so. It
needs a ClassDescription so that we can turn it on and off using a security
policy, if necessary.

Comment 9

[David Bradley]

Just a quick update, I haven't forgotten this. In the process of getting the
changes in, I've encountered a strange bug with the prototype object of the
plugin object. A ref count cleanup issue. I thought it was the result of the
changes I made here, but it appears not. I've worked back to a clean tree, so
not sure what's the culprit. Just taken a bit of time after chasing ghosts to
focus in on the real problem.

Comment 10

[David Bradley]

Attachment: Now reuses the class info object

Also returns a string for the GetClassDescription implemention.

Comment 11

[Adam Lock]

Comment on attachment 109673 [details] [diff] [review]
Now reuses the class info object

r=adamlock

Comment 12

[Mitchell Stoltz (not reading bugmail)]

Sorry to nitpick, but in GetClassDescription, you should check the return value
from strdup, and if it's null, return NS_ERROR_OUT_OF_MEMORY. Otherwise an
out-of-memory condition could lead to a security breach.

Comment 13

[Mitchell Stoltz (not reading bugmail)]

Actually, please disregard the last comment. Turns out caps doesn't check the
return value.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 109673 [details] [diff] [review]
Now reuses the class info object

- In XPCIDispatchClassInfo::GetClassDescription():

+    *aClassDescription = nsCRT::strdup("IDispatch");
+    return NS_OK;

As Mitch said, this may not matter in the current code, but returning
NS_ERROR_OUT_OF_MEMORY if we're OOM is the right thing to do, so you might as
well do the right thing here.

sr=jst with that change.

Comment 15

[David Bradley]

Patch checked in, with the check for out of memory.

Comment 16

[Phil Schwartau]

Rubber-stamp vrfy -