Closed Bug 47354 Opened 24 years ago Closed 24 years ago

capabilities settings don't obey lexical closure of JS funcs

Categories

(Core :: Security: CAPS, defect, P3)

Product:
Core
Component:
Security: CAPS
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: dmosedale, Assigned: shaver)

Details

Attachments

(2 files)

Patch to fix loader and nsScriptSecurityManager
9.85 KB, patch
Details | Diff | Splinter Review
New patch, shinier and nicer-smelling
10.07 KB, patch
Details | Diff | Splinter Review 
I have a JS component that is creating an anonymous JS function object, binding
it to some data with a closure, and handing it to a C++ object as a callback.
The JS component itself can call dangerous functions such as dump(), but the
anonymous callback cannot; it gets an exception:

"Access to XPConnect service denied. [nsILDAPMessageListener::onLDAPMessage]"
nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"

It seems like this ought to just work.  For what it's worth, even if I do add an
n.s.P.enablePrivilege() call at the indicated spot in the code below, it fails
with the exact same error as in bug 35116 ("ReferenceError: netscape is not
defined").  However, no amount of restarting works around that error, despite
the description in 35116.

The code:

function boundCallback() {}

boundCallback.prototype = {
    onLDAPMessage: {}
}

function GetTargets () {

    function generateBoundCallback()
    {
         cb = new boundCallback();

         cb.onLDAPMessage = function() {

             // enablePrivilege here

             if (DEBUG) {
                 dump("boundCallback() called with scope: \n\t" +
                 aSource.Value + "\n\t" + aProperty.Value +
                 "\n\t" + aTruthValue + "\n\n");
             }
         }

	 return cb;
    }

    this.getConnection(url.host, url.port, generateBoundCallback());

    return;
}
This looks like 39975, for which I don't have an accessible patch (the machine
with my Mozilla hackery is at home, sans internet).

(If this is really a dup of 39975, someone else should probably take it, too.)

Note that there is no security check for 'dump'. The property accesses are what 
is probably failing.

I don't know about whether the principles ought to get conveyed to the new 
closure function. In the cases where privileges come from an explicit call to 
the security manager then they certainly should not get conveyed. But in this 
case of priniciples that came with the original JSComponent I'm out of my depth.

The bug 35116 part is bad. The netscape.security.* stuff to *too* entertwined 
with the DOM JSContexts. I don't have a solution.
Naive question: why shouldn't they be conveyed to the closure function even in
the case of privs that come from an explicit call to the security managemer?
Looks like there are two issues here, which should probably be filed as separate
bugs. JS components should have the system principal, so they shouldn't even
have to call enablePrivilege in order to use XPConnect. As to why we aren't
getting the system principal in this case, I'm not sure.
The "netscape is not defined" issue is separate, I think. Dan, could you show me
how to reproduce this bug, or maybe I could sit down and look it over with you?
How important is this, is it nsbeta3?
Status: NEW → ASSIGNED
Target Milestone: --- → M18
Mitch: I think 35516 and 39975 may be the two bugs in question, or at least 
something like them.  shaver is actually in the middle of consing up a patch for 
me, so maybe the right thing to do is reassign to him.
OK, I think I nailed it.  In the patch I will attach below, I put a
system-principal-having object at the top of every JS component's scope chain,
where it can keep nsScriptSecurityManager happy.

I had to alter nsScriptSecurityManager::GetObjectPrincipal to handle the case
where the nsISupports private data was a wrapped native, because the
nsXPCWrappedNative doesn't pass QI through to the native it's wrapping.  I think
that's correct, and I'm pretty sure it's safe.

jband: I turned out to not need the InitClassesWithWrappedNative thing, so it
doesn't appear in the patch.
Assignee: mstoltz → shaver
Status: ASSIGNED → NEW
shaver's patches works great for me, with a slightly different test case: the 
current tip of directory/xpcom/datasource/nsLDAPDataSource.js, which is what I 
actually care about.
Looks alright to me, but I want to hear jband's take on any possible security 
implications of this change. Can you hold off checking it in until we hear from 
jband?

I'd really like to get it in before it rots, and I can't imagine how there are
security implications here -- an attacker who can put a wrapped object with
access to a system principal in place on the scope chain would seem to already
be able to subvert the current system -- but I can wait for jband to point out
what I'm missing.
Status: NEW → ASSIGNED
A few things biug and small...

- the lines
    *aPrincipal = mPrincipal;
    NS_ADDREF(*aPrincipal);

...can be...

   NS_ADDREF(*aPrincipal = mPrincipal);

This saves a pointer deref.

- You ended up with 2 JS_InitStandardClasses... blocks. The second one is 
unnecessary, no?

- not having your initializers for memeber in the same order as declared in 
mozJSComponentLoader will give a warning. When initizilers depend on each other 
then this can be bad.

- You end up with only one shred Components object. This should be OK. It does 
mean that any natives reflected into more than one one global space will be 
reflected using the same wrapper in each. At some future point when the 
__proto__ of the wrapper is of interest this could be bad. We can worry about 
that later I think.

- securitywise... I don't see any new holes or anything. I'd rather see this 
code try to QI to nsIScriptObjectPrincipal first and then only try to QI to 
nsIXPConnectWrappedNative if the former fails. This allows more flexibility as 
the DOM stuff moves to use xpconnect. no? This also puts the more likely case 
first and avoids an extra QI for DOM stuff.
I'll tidy up the first 2 stat, thanks.

I don't see the out-of-order initializers, though I think I can just remove the
#ifdef'd init for mSystemPrincipal: nsCOMPtrs get initialized implicitly to
nsnull, right?

Didn't I always have a single shared Components object in the super-global?
  323     rv = mXPC->InitClasses(mContext, mSuperGlobal);

Conceptually, I think of the WrappedNative thing being a wrapper that we should
remove before testing the characteristics of the object, but your
DOM-performance argument compels.  I'll fix.

New patch coming shortly.

A security issues to consider...

With the double-wrapping stuff a JS user of a JS Component can ask for the 
underlying JSObject using 'wrappedJSObject'. If the caller has 
UniversalXPConnect and the callee implemented wrappedJSObject then the 
underlying object is exposed. The if the callee then reparents some code then 
that code gets system princpals. Right? Is this bad? Any more powerful then just 
having UniversalXPConnect? 
Hmm.  I have another concern here: I now end up with the super-global in the
scope chain, which is not what I want.  jband, is it safe to do something like
this to the JSObject for the wrapped native?

  JS_SetParent(cx, obj, nsnull);
  JS_SetPrototype(cx, obj, mSuperGlobal);

?

Firstly, I don't think you can reparent code; __parent__ setting went the way of
obj.eval a little while back, for security reasons.  And then, if you already
have UniversalXPConnect, you're out of the box.

It might be more of a concern once we get partial-XPConnect privileges, because
they too could reach the magic privilege object via __parent__ walking from a
wrappedJSObject, but any partial-XPConnect priv system will have to deal with
containing that sort of access bleed -- just making wrappedJSObject require
UniversalXPConnect might be enough.

Am I seeing things, or does the patch mix nsnull with valid nsresult values in
return statements:

         return NS_ERROR_OUT_OF_MEMORY;
+
+#ifndef XPCONNECT_STANDALONE
+    NS_WITH_SERVICE(nsIScriptSecurityManager, secman, 
+                   NS_SCRIPTSECURITYMANAGER_PROGID, &rv);
+    if(NS_FAILED(rv) || !secman)
+        return nsnull;
+
+    rv = secman->GetSystemPrincipal(getter_AddRefs(mSystemPrincipal));
+    if(NS_FAILED(rv) || !mSystemPrincipal)
+        return nsnull;
+#endif
 
-    mSuperGlobal = JS_NewObject(mContext, &gGlobalClass, NULL, NULL);
+    mSuperGlobal = JS_NewObject(mContext, &gGlobalClass, nsnull, nsnull);
+
     if (!mSuperGlobal)
-        return NS_ERROR_OUT_OF_MEMORY;
+        return nsnull;

/be
We do have a partial-XPConnect privilege system, nsISecurityCheckedComponent. 

John, could the exploit that Shaver's describing work with a 

SecurityCheckedComponent object? What if we assume you can reassign __parent__?


Brendan: how embarrassing.  Fixed in my tree, patch awaiting jband input on the
proto/parent frob of the wrapped object.

If you can reassign __parent__, I think the jig is generally up for us, no?

Allowing assignment to __parent__ would wreak havoc with all scope-based
security checks, along with optimizations based on static analysis by compilers.
We don't allow it, so don't worry about it.

/be
shaver: yes you can change parent and proto of wrappers via the jsapi. At some 
future point xpconnect is going to manage shared protos for all wrappers around 
a given class (CLSID) within a give scope. At that point this will matter and we 
can revisit. bug 13425

mstoltz: I don't know that I'd call nsISecurityCheckedComponent a 
"partial-XPConnect privilege system". I think it is more of a "distribution of 
responsibility for security system". As far as I can see, the impact here is 
that it allows a JS Component to give away that system principal to any caller 
even if the caller does not have UniversalXPConnect. But that is the nature of 
the distributed security system - it just allows more code to do something 
wrong. I don't think the synergy here makes things especially less safe.

I didn't realize that obj.eval was fully gone and that there was no way to 
sneek into a parent chain. Good.
jband: obj.eval(s) is still with us, but it is equivalent to with(obj)eval(s).

/be
shaver: I forgot to mention that I don't think it's kosher to trim out IBM's 
copyright notice. Is it?
mkaply said that when modifying IBM-splattered files, it was OK to normalize
them to simply indicate their contributorship.  (IBM puts that stuff there
because they believe the license requires them to -- and it's not an
unreasonable interpretation -- not because they really care about having that
text in place, I believe.)

At an absolute minimum, it needs to be more respectful of the 80th column.

I'll put up the new patch, seeking r and a.

r=jband

I can't see anything wrong with it. It compiles without warnings on NT and seems 
to run and work. You're running testcases that shows this fixes the bug?
The latest patch works for on the tip of nsLDAPDataSource.js.
a=brendan@mozilla.org modulo whitespace whining.

/be
Yeah, dmose and I have verified that it gives privileges to his LDAP closures,
and stuff, and stepping around with the debugger I can see where it gets called
to check the principal.

Going in! Thanks, all!
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
QA Contact: czhang → junruh
Verified per shaver@mozilla.org's comments.
Status: RESOLVED → VERIFIED
mid-air collision ? / bugzilla cleanup
Reopening (current State: verified and no resolution)
Status: VERIFIED → REOPENED
fixed
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
and verfied....
Status: RESOLVED → VERIFIED
This patch adds the code: 

+    nsCOMPtr<nsIScriptObjectPrincipal> backstagePass =
+      new BackstagePass(mSystemPrincipal);

and if one just starts a browser with XPCOM_MEM_BLOAT_LOG and closes it, it will
show a leak of a BackstagePass. Is it certain that this doesn't leak? (It is the
only file that allocates BackstagePasses according to lxr). 
There's a well-known leak that entrains the backstage pass, but this bug's fix
did not introduce that leak.  Cc'ing stephend and dbaron.

/be
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: