304048 - xpconnect getters/setters don't have principals until after they pass or fail their security check

Status: NEW

(Core :: Security: CAPS, defect)

Description

[timeless]

I have a jscomponent which binds to window.location the way that nsSidebar.js 
binds to window.sidebar.

for many things, it works, but for getting href, it fails. there are two 
security checks for poking href:
### CanAccess(Location JS Component.href, 1) ClassLookup sameOrigin  GRANTED.
this first check happens in the context of the content window that's trying to 
manipulate the href propert of window.location. My component implements 
SecurityCheckedComponent and answers appropriately.

### CanAccess(Object.href, 1) ClassLookup sameOrigin CheckXPCPerms DENIED.
this second check happens inside the xpconnect impl of the component as js is 
about to allow access to the internal href property.

Unfortunately, the xpconnect component isn't on the jsstack until *after* the 
security check, so the js stack is:
[native frame - xpconnect hosting jscomponent]
+	{,,js3250.dll}(*((*((JSContext*)0x027c02e0)).fp)).script
	0x00000000 {...}	JSScript *
[gmail - untrusted content frame]
+	{,,js3250.dll}(*((*((JSContext*)0x027c02e0)).fp)).down->script->filename
	0x028942e1 "http://mail.google.com/mail/?..."	const char *
	{,,js3250.dll}(*((*((JSContext*)0x027c02e0)).fp)).down->script->lineno
	3	unsigned int

the native call stack is approximately:
caps.dll!nsScriptSecurityManager::CheckPropertyAccessImpl [there is not yet a 
stack frame for the trusted js at this time]
caps.dll!nsScriptSecurityManager::CheckObjectAccess
js3250.dll!js_InternalGetOrSet
js3250.dll!js_GetProperty
js3250.dll!JS_GetProperty
xpc3250.dll!nsXPCWrappedJSClass::CallMethod [implementation of window.location -
 this code should be trusted]
xpc3250.dll!nsXPCWrappedJS::CallMethod
xpcom_core.dll!PrepareAndDispatch
xpcom_core.dll!SharedStub [dispatch]
xpcom_core.dll!XPTC_InvokeByIndex
xpc3250.dll!XPCWrappedNative::CallMethod [reflection of window.location]
xpc3250.dll!XPC_WN_GetterSetter [a security check happened and passed before 
this call]
js3250.dll!js_Invoke [untrusted gmail]

I'm not really sure whose fault this is, it could be mine, caps, xpconnect or 
even js. I think it's fairly hard for me to blame caps for failing to find 
security principals that no one has stuck on the stack. i think that what i'm 
doing is mostly reasonable, although my implementation uses syntactic sugar get 
href(){} which i think is what's causing me pain. switching it from get href() 
{} to getHref: function getHref(){} resolves this problem.

I'm not sure if there isn't already a report about this, I seem to recall 
hitting this at times in xpcshell, so it's possible i've already filed this....

Since I have a workaround, this isn't a blocker for me, although i did lose a 
lot of time hunting it down :(.

I can provide two versions of hsLocation.js for people to experiment with, it 
helps to define

Comment 1

[timeless]

DEBUG_CAPS_HACKER (bug 304054)

Comment 2

[Brendan Eich [:brendan]]

Why doesn't check 2 succeed on account of your component implementing
SecurityCheckedComponent ?

/be

Comment 3

[timeless]

check 2 doesn't call my object at all, afaik it's spidermonkey doing precisely 
what it says, asking caps if it's ok to access Object(generic).href

Comment 4

[Johnny Stenback (:jst)]

The problem here is that caps' checkAccess() hook doesn't deal with
nsIScriptSecurityCheckedComponent at all. Not sure what the best way to make it
do that is, but I'll attach a proposal that we can ponder...

Comment 5

[Johnny Stenback (:jst)]

Attachment: Make caps' checkAccess() hook deal with nsISecurityCheckedComponent.

Comment 6

[Brendan Eich [:brendan]]

Calling this a caps bug, with xpconnect support needed for the fix.  Confirming.
 Should be reassigned -- timeless?

jst, can you do a diff -w, and also narrate a bit?

/be

Comment 7

[Alex Vincent [:WeirdAl]]

I managed to break DOM Level 1 Core quite nicely with a custom protocol handler
& channel implemented in JS:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Permission denied to get property Object.protocolFlags' when
calling method: [nsIProtocolHandler::protocolFlags]"  nsresult: "0x8057001e
(NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS
frame :: virtual://chrome//verbosio/content/virtualTestHost4.xul :: <TOP_LEVEL>
:: line 23"  data: no]
************************************************************

The line in question was:

foo = document.getElementById("foo");

A little elf suggested this patch might fix the problem, and yes, it did.  A
couple interesting points, possibly for another bug:

When I had this set for the styling:

#foo {
  -moz-binding: url("chrome://verbosio/content/virtualTestHost4.xml#binding-foo");
}

the binding worked and document.getElementById() did as well.

When I had this:

#foo {
  -moz-binding:
url("virtual:chrome://verbosio/content/virtualTestHost4.xml#binding-foo");
}

getElementById() was busted in a build without this patch, but worked in a build
with the patch.  The actual binding itself remained busted.

Comment 8

[timeless]

we need this for our product which is based on 1.8

Comment 9

[timeless]

Comment on attachment 192410 [details] [diff] [review]
Make caps' checkAccess() hook deal with nsISecurityCheckedComponent.

>Index: js/src/xpconnect/src/xpcwrappedjs.cpp
>@@ -306,18 +306,71 @@ return_wrapper:
>+// static
>+nsresult
>+nsXPCWrappedJS::GetUsedOnly(XPCCallContext& ccx,
>+                            JSObject* aJSObj,
>+                            REFNSIID aIID,
>+                            nsISupports* aOuter,
>+                            nsXPCWrappedJS** wrapperResult)
>+{
...
this doesn't work:
>+    // always find the root JSObject
this always returns non null (even in cases of catastrophic failure like bug
307451):
>+    rootJSObj = clazz->GetRootJSObject(ccx, aJSObj);
>+
>+    NS_RELEASE(clazz);
which means this isn't reachable:
>+    if(!rootJSObj)
>+        return NS_ERROR_FAILURE;
and the code will unfortunately either spiral to death by stack overflow. or
out of control if someone pinches the stack.

Comment 10

[Asa Dotzler [:asa]]

not going to block on this but if you come up with a patch that actually fixes
the problem and didn't introduce a lot of risk, we'd re-evaluate.

Comment 11

[timeless]

Comment on attachment 192410 [details] [diff] [review]
Make caps' checkAccess() hook deal with nsISecurityCheckedComponent.

Index: js/src/xpconnect/src/XPCDispInlines.h

for the record, the change in here was rolled into cvs both for trunk and 1.8
branch as part of some other bug. (just in case someone else tries to keep
track of changes between patch sets and gets confused - as we did today).

Comment 12

[timeless]

this is something that plasticmillion hit. it's a fairly major problem at times.

Comment 13

[Boris Zbarsky [:bzbarsky]]

We can't tell what this bug is about, which is why it got minused.  Is the request for a better replacement for nsISecurityCheckedComponent that plays well with classinfo?  If not, can you clearly explain what the issue here is?

Comment 14

[timeless]

the issue is that checks are happening at the wrong time or against the wrong objects guaranteeing that they'll fail.

where's the blocking2.0 flag?

for the record. i'm not involved in the project that was hurt by this bug, so assigning it to me isn't necessarily a great idea.

i'm really sorry, the bug as stated in comment 0 is actually well worded imo it includes a detailed description of what does and does not work, and the people who can affect this bug have commented indicating that it's a bug. there's a patch which doesn't quite work giving a hint about the solution, and lastly the summary is actually a summary of what's wrong. perhaps it should say that "javascript objects can't implement getters/setters because caps asks the wrong object if that's ok".

Comment 15

[timeless]

*** Bug 360207 has been marked as a duplicate of this bug. ***

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not blocking since this is not affecting firefox.

Comment 17

[John Gaunt (redfive)]

Attachment: first patch updated to trunk (3/26/07)

jst's first patch fixed our issue with not being able to access properties. This patch is his patch updated to apply to the current trunk code. If folks don't see their problem fixed with this patch please speak up. This is pretty critical for us to be able to expose security checked compoents to the web via:

myFunc(foo.bar);
foo.bar = "test";

versus

myFunc(foo.getBar());
foo.setBar("test");

(this patch also made all the classDescription() errors in the jsconsole go away! Expectedly)

Comment 18

[Johnny Stenback (:jst)]

I'm of the opinion that if my old patch here fixes real problems for some users then we should take it, even if it doesn't completely fix this bug as filed. Timeless, do you have anything against that?

I still need to review this as it's been a while and I don't quite remember this patch in detail any more :) And sorry for not doing that earlier, emergency travel etc came in the way here for me...

Comment 19

[timeless]

jst: no objections, a followup bug to deal w/ the problems i indicated would be fine.

Comment 20

[Jonathan Watt [:jwatt]]

This bug is a problem for Joost so we'd be quite happy to see it reviewed and landed too.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

timeless has some valid concerns in comment 9. He and I played with this patch a little last night and I think we may have a solution. Testing now.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, no strict warnings

This is a slightly tweaked version that won't spit out strict warnings when you try to QI JSObjects that don't have a QI function. See bug 383524 and bug 383330.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 267506 [details] [diff] [review]
Patch, no strict warnings

r=jst, but since I more or less wrote this, another pair of eyes on this would be great. Maybe bz or brendan could be talked into sr'ing this?

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 267506 [details] [diff] [review]
Patch, no strict warnings

bz: can you sr this?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 267506 [details] [diff] [review]
Patch, no strict warnings

>Index: js/src/xpconnect/src/xpcconvert.cpp
>+                                     PRBool createNew,

I'd call this allowCreateNew.  Same in xpcprivate.h

>Index: caps/src/nsScriptSecurityManager.cpp
>+            // and it's up to the implementation to decide if it wants
>+            // to permit access or not.

s/if/whether or not/.  s/ or not//.

sr=bzbarsky with that.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks, Boris!

Johnny, do you want me to drive this? Or are you going to take it?

Comment 27

[Johnny Stenback (:jst)]

Ben, if you're unable to I can, but please do if you have the cycles!

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch

Updated to address bz's comments. Carrying forward r/sr+, requesting 1.9 approval.

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 278652 [details] [diff] [review]
Patch

Thanks Ben! a=jst

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed on trunk.

Comment 31

[Stuart Parmenter]

could this have caused a performance regression on the pageload tests?

Comment 32

[Stuart Parmenter]

nevermind, numbers seem to be back down.

Comment 33

[Robert Sayre]

This regressed Txul on linux, mac, and windows.

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #33)

I don't see anything obvious in the patch... Although this does introduce a small critical section. Johnny, bz, any ideas?

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

I bet the problem is that this makes already-expensive security checks even more expensive (some of them, including done while computing 'this' in some cases) will now have to go back through XPConnect to call QueryInterface.

Looking at the stack in comment 0, I wonder if the patch sitting in bug 352791 would help here as well -- though someone is going to have to figure out why it causes massive leaks (or if it actually still does, it's been a while since I last looked).

Comment 36

[Brendan Eich [:brendan]]

Is there a fast path for caps, based on more narrow logic than we can encode in the fat XPConnect types, by which we could get the JSClass pointer from the source (obj) object in nsScriptSecurityManager::CheckObjectAccess, test for the JSCLASS_HAS_PRIVATE and JSCLASS_PRIVATE_IS_NSISUPPORTS flags, JS_GetPrivate and we are done?

If we are only seeking the root nsISupports*, and we don't have to grovel up the prototype chain and across class boundaries, or deal with XOWs, then that would be the fast path.

If this is not reliable, can we use it as *a*, if not *the* fast path and bail to the patch's slower path?

mrbkap: can you 'splain how bug 352791's patch might help here? Thanks.

/be

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Johnny, bz, any ideas?

I'd profile to see what's up...

Comment 38

[Brendan Eich [:brendan]]

Profiling would be good, but I think Blake nailed it: the patch added new code paths that are slow enough, with locking as Ben pointed out, where there was no such code executing on each CheckObjectAccess previously.

In general, we should cast a colder eye on adding code as the patch does. We have too many layers and too much wrapper speciation already. Wrappers (XOWs as Blake has implemented recently) are good when they proxy hard cases and tax only those cases with security checks. When they result in common, same-origin code paths suffering performance hits, they are evil.

The patch for bug 352791 might help, and it adds less code, and aligns XPConnect with DOM scope/context affinity. Probably we should back this bug's patch out and try to fix that bug's patch, or prove that it no longer creates leaks (which I say must entail proving why it once did).

/be

Comment 39

[Boris Zbarsky [:bzbarsky]]

> The patch for bug 352791 might help

I don't see how it could.  Certainly not for the uses of nsISecurityCheckedComponent that this patch also fixes.  Now I think that nsISecurityCheckedComponent is badly designed, but it's what we have so far.

How commonly is CheckObjectAccess called?  Can we avoid constructing an XPCCallContext here?  Those are pretty expensive to construct...

Comment 40

[Brendan Eich [:brendan]]

(In reply to comment #39)
> > The patch for bug 352791 might help
> 
> I don't see how it could.  Certainly not for the uses of
> nsISecurityCheckedComponent that this patch also fixes.  Now I think that
> nsISecurityCheckedComponent is badly designed, but it's what we have so far.

Blake will comment when he can, he said over IRC (he's in class now).

> How commonly is CheckObjectAccess called?

For every interpreted getter or setter function invocation. See js_InternalGetOrSet. And for things like watchpoint setting (rare, not the Txul issue). And (Blake mentioned this) for default |this| parameter binding (see ComputeGlobalThis). The last may be the one popping up here, more than non-native getter/setter function invocation.

We may not need these access checks now that XOWs have landed, however. As I wrote in comment 38 second paragraph, the fast path for same-origin/chrome-only should not entail any security check overhead, and the proxies or wrappers should be automatically deployed for all the cross-trust-domain edges. So it would be great to get mrbkap's views on this too.

/be

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #36)
> mrbkap: can you 'splain how bug 352791's patch might help here? Thanks.

From the stack in comment 0:
caps.dll!nsScriptSecurityManager::CheckPropertyAccessImpl
[Nothing to cause a scripted frame on cx]
xpc3250.dll!nsXPCWrappedJSClass::CallMethod

In this case, XPCWJSC::CallMethod will find the safe JSContext, push it, and use that to execute the function (as well as do security checks, etc). Because there is no scripted code, we'll use the null-principals global object for that context as the subject, and fail security checks. bug 352791's patch makes us find a context with a privileged global object, which will render the nsISecurityCheckComponent code moot, since it's trumped by the chrome principals on the stack.

Comment 42

[Boris Zbarsky [:bzbarsky]]

For that case.  But in general, when the caller is NOT chrome, this code still has a problem, no?

Comment 43

[Boris Zbarsky [:bzbarsky]]

Yes, ComputeGlobalThis seems like a likely culprit.

Comment 44

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm working on coercing Shark to give me some readable profiling data, FWIW. Or do you guys think it is unnecessary?

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

It's probably worth it in order to confirm or refute our current theories. I'm not objecting to this patch, just pointing out related work.

Comment 46

[Brendan Eich [:brendan]]

We may want this patch, even though I'm against it on general principles, but we need to do something to restore the check-free fast path. That may mean removing the OBJ_CHECK_ACCESS call in ComputeGlobalThis.

/be

Comment 47

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: TXUL data

I collected three runs of TXUL data for each of three builds:

1. The current live code (attachment 278652 [details] [diff] [review] checked in)
2. Reverted attachment 278652 [details] [diff] [review].
3. Attachment 278652 [details] [diff] checked in along with this patch: http://people.mozilla.com/~mrbkap/patch

Doesn't look like mrbkap's patch helps that much... but it does a little!

Comment 48

[Ben Turner (not reading bugmail, use the needinfo flag!)]

And in one of my shark profiles (but only one) it looks like the JS_HasProperty call that was added to avoid a JS strict warning (see bug 383524) is taking up about 36% of the js_ComputeThis call's time.

That data is here: http://pastebin.mozilla.org/190647

Comment 49

[Robert Sayre]

(In reply to comment #46)
> We may want this patch, even though I'm against it on general principles, but
> we need to do something to restore the check-free fast path. That may mean
> removing the OBJ_CHECK_ACCESS call in ComputeGlobalThis.

Is anyone actively working on this? If not, we should back it out until we have time. A bunch of big XUL frontend changes just got checked in for EV. If they regress anything, we'll have checked in perf regressions on top of one another.

Comment 50

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok, I backed this puppy out pending further analysis. Guys, tell me what you'd like me to do?

Comment 51

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 278652 [details] [diff] [review]
Patch

Please rerequest approval when the perf issue is resolved.

Comment 52

[Mark Finkle (:mfinkle) (use needinfo?)]

Could the recent changes to JS_HasProperty and ComputeGlobalThis have had a positive impact on the pref regressions seen here?

This bug and bug 360207 would be nice to fix for those adding APIs to the web content (songbird, joost, prism and extensions too).

Comment 53

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

It should help a huge amount, yes;  Ben, can you re-profile and find out?

Comment 54

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I won't have cycles to look at this for a while.

-> Default owner.

Comment 55

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: previous patch updated to trunk

I had trouble patching the previous patch to trunk, so here is the updated version

Comment 56

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Updated Txul times

Here is Txul test results using current trunk, with and without the patch. I wanted to see what affect the recent JS changes might have had on the performance.

A regression still exists

Comment 57

[Matthew Gertner]

Yeah I got the same results when I tested this. Shaver suggested sharking it and seeing if JS_HasProperty is still hogging as many cycles. I've gotten as far as making a shark-enabled build so I'll try to measure this once I get back into the office.

Comment 58

[timeless]

Attachment: for debate

i haven't tested this (sadly, and oddly, none of weirdal, plasticmillion or I have provided a testcase in these bugs - we all have them, but I'd have to find one), but i believe it should work, and its performance characteristics should be much happier.

Comment 59

[Boris Zbarsky [:bzbarsky]]

You can use the extension in bug 493495 with the /foo url in the extension component pointed to a document like the one in bug 493495 comment 19 and with the component changed to implement nsISecurityCheckedComponent, no?  At least if I understand this bug correctly..

Comment 60

[Brendan Eich [:brendan]]

(In reply to comment #57)
> Yeah I got the same results when I tested this. Shaver suggested sharking it
> and seeing if JS_HasProperty is still hogging as many cycles. I've gotten as
> far as making a shark-enabled build so I'll try to measure this once I get back
> into the office.

There are shark builds available:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-tracemonkey/

/be

Comment 61

[Johnny Stenback (:jst)]

Comment on attachment 379463 [details] [diff] [review]
for debate

Clearing out old reviews. If this is still relevant, please re-request review for this patch.