47354 - capabilities settings don't obey lexical closure of JS funcs

Status: VERIFIED FIXED

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

Description

[Dan Mosedale (:dmosedale, :dmose)]

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;
}

Comment 1

[Mike Shaver (:shaver emeritus)]

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.)

Comment 2

[John Bandhauer]

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.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 4

[Mitchell Stoltz (not reading bugmail)]

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?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 6

[Mike Shaver (:shaver emeritus)]

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.

Comment 7

[Mike Shaver (:shaver emeritus)]

Attachment: Patch to fix loader and nsScriptSecurityManager

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 9

[Mitchell Stoltz (not reading bugmail)]

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?

Comment 10

[Mike Shaver (:shaver emeritus)]

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.

Comment 11

[John Bandhauer]

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.

Comment 12

[Mike Shaver (:shaver emeritus)]

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.

Comment 13

[John Bandhauer]

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? 

Comment 14

[Mike Shaver (:shaver emeritus)]

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);

?

Comment 15

[Mike Shaver (:shaver emeritus)]

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.

Comment 16

[Brendan Eich [:brendan]]

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

Comment 17

[Mitchell Stoltz (not reading bugmail)]

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__?

Comment 18

[Mike Shaver (:shaver emeritus)]

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

Comment 19

[Mike Shaver (:shaver emeritus)]

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

Comment 20

[Brendan Eich [:brendan]]

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

Comment 21

[John Bandhauer]

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.

Comment 22

[Brendan Eich [:brendan]]

jband: obj.eval(s) is still with us, but it is equivalent to with(obj)eval(s).

/be

Comment 23

[John Bandhauer]

shaver: I forgot to mention that I don't think it's kosher to trim out IBM's 
copyright notice. Is it?

Comment 24

[Mike Shaver (:shaver emeritus)]

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.

Comment 25

[Mike Shaver (:shaver emeritus)]

Attachment: New patch, shinier and nicer-smelling

Comment 26

[John Bandhauer]

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?

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

The latest patch works for on the tip of nsLDAPDataSource.js.

Comment 28

[Brendan Eich [:brendan]]

a=[e-mail] modulo whitespace whining.

/be

Comment 29

[Mike Shaver (:shaver emeritus)]

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!

Comment 30

[John Unruh]

Verified per [e-mail]'s comments.

Comment 31

[Matthias Versen [:Matti]]

mid-air collision ? / bugzilla cleanup
Reopening (current State: verified and no resolution)

Comment 32

[Matthias Versen [:Matti]]

fixed

Comment 33

[Matthias Versen [:Matti]]

and verfied....

Comment 34

[Peter A Jonsson]

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). 

Comment 35

[Brendan Eich [:brendan]]

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