Security Manager blocks invoking methods on some IDispatch based objects

VERIFIED FIXED in mozilla1.3beta

Status

()

Core
XPConnect
P1
normal
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: David Bradley, Assigned: David Bradley)

Tracking

Trunk
mozilla1.3beta
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

16 years ago
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.
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.3beta
(Assignee)

Comment 1

16 years ago
Created attachment 108876 [details] [diff] [review]
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.
(Assignee)

Comment 2

16 years ago
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.
Attachment #108876 - Flags: superreview?(jst)
Attachment #108876 - Flags: review?
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".
(Assignee)

Comment 4

16 years ago
Created attachment 109025 [details] [diff] [review]
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.
Attachment #108876 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #109025 - Flags: superreview?(jst)
Attachment #109025 - Flags: review?(adamlock)

Comment 5

16 years ago
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.
Attachment #109025 - Flags: review?(adamlock) → review+
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?
(Assignee)

Comment 7

16 years ago
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++.
> 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.
(Assignee)

Comment 9

16 years ago
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.
(Assignee)

Comment 10

16 years ago
Created attachment 109673 [details] [diff] [review]
Now reuses the class info object

Also returns a string for the GetClassDescription implemention.
Attachment #109025 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #109673 - Flags: superreview?(jst)
Attachment #109673 - Flags: review?(adamlock)

Comment 11

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

r=adamlock
Attachment #109673 - Flags: review?(adamlock) → review+
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.
Actually, please disregard the last comment. Turns out caps doesn't check the
return value.
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.
Attachment #109673 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 15

16 years ago
Patch checked in, with the check for out of memory.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 16

16 years ago
Rubber-stamp vrfy -
Status: RESOLVED → VERIFIED
(Assignee)

Updated

16 years ago
Attachment #108876 - Flags: superreview?(jst)
(Assignee)

Updated

16 years ago
Attachment #109025 - Flags: superreview?(jst)
(Assignee)

Updated

16 years ago
Attachment #108876 - Flags: review?
You need to log in before you can comment on or make changes to this bug.