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

NEW
Assigned to

Status

()

13 years ago
5 years ago

People

(Reporter: timeless, Assigned: dveditz)

Tracking

({helpwanted})

Trunk
x86
Windows XP
helpwanted
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b4 -
blocking1.8b5 -
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [prof-wanted])

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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
(Reporter)

Comment 1

13 years ago
DEBUG_CAPS_HACKER (bug 304054)
Why doesn't check 2 succeed on account of your component implementing
SecurityCheckedComponent ?

/be
(Reporter)

Comment 3

13 years ago
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
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...
Created attachment 192410 [details] [diff] [review]
Make caps' checkAccess() hook deal with nsISecurityCheckedComponent.
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
Status: UNCONFIRMED → NEW
Component: XPConnect → Security: CAPS
Ever confirmed: true
(Reporter)

Updated

13 years ago
Assignee: dbradley → jst
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.
(Reporter)

Updated

13 years ago
Attachment #192410 - Flags: superreview?(dveditz)
Attachment #192410 - Flags: review?(dbradley)
(Reporter)

Comment 8

13 years ago
we need this for our product which is based on 1.8
Flags: blocking1.8b5?
Flags: blocking1.8b4?

Updated

13 years ago
Flags: blocking1.8b4? → blocking1.8b4-
(Reporter)

Comment 9

13 years ago
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.
Attachment #192410 - Flags: superreview?(dveditz)
Attachment #192410 - Flags: review?(dbradley)
Attachment #192410 - Flags: review-

Updated

13 years ago
Flags: blocking1.8b5? → blocking1.8b5-

Comment 10

13 years ago
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.
(Reporter)

Comment 11

13 years ago
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).
Flags: blocking1.9a1?
(Reporter)

Comment 12

12 years ago
this is something that plasticmillion hit. it's a fairly major problem at times.
Keywords: helpwanted
Flags: blocking1.9a1? → blocking1.9-
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?
(Reporter)

Comment 14

12 years ago
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".
Flags: blocking1.9- → blocking1.9?
(Reporter)

Comment 15

12 years ago
*** Bug 360207 has been marked as a duplicate of this bug. ***
Not blocking since this is not affecting firefox.
Flags: blocking1.9? → blocking1.9-

Comment 17

12 years ago
Created attachment 259723 [details] [diff] [review]
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)
Attachment #259723 - Flags: review?(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...
(Reporter)

Comment 19

12 years ago
jst: no objections, a followup bug to deal w/ the problems i indicated would be fine.
This bug is a problem for Joost so we'd be quite happy to see it reviewed and landed too.
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.
Created attachment 267506 [details] [diff] [review]
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.
Attachment #259723 - Attachment is obsolete: true
Attachment #267506 - Flags: review?
Attachment #259723 - Flags: review?(jst)
Attachment #267506 - Flags: review? → review?(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?
Attachment #267506 - Flags: review?(jst) → review+
Comment on attachment 267506 [details] [diff] [review]
Patch, no strict warnings

bz: can you sr this?
Attachment #267506 - Flags: superreview?(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.
Attachment #267506 - Flags: superreview?(bzbarsky) → superreview+
Thanks, Boris!

Johnny, do you want me to drive this? Or are you going to take it?
Ben, if you're unable to I can, but please do if you have the cycles!
Created attachment 278652 [details] [diff] [review]
Patch

Updated to address bz's comments. Carrying forward r/sr+, requesting 1.9 approval.
Attachment #278652 - Flags: superreview+
Attachment #278652 - Flags: review+
Attachment #278652 - Flags: approval1.9?
Comment on attachment 278652 [details] [diff] [review]
Patch

Thanks Ben! a=jst
Attachment #278652 - Flags: approval1.9? → approval1.9+
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 31

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

Comment 32

11 years ago
nevermind, numbers seem to be back down.

Comment 33

11 years ago
This regressed Txul on linux, mac, and windows.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
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).
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
> Johnny, bz, any ideas?

I'd profile to see what's up...
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
> 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...
(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

(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.
For that case.  But in general, when the caller is NOT chrome, this code still has a problem, no?
Yes, ComputeGlobalThis seems like a likely culprit.
I'm working on coercing Shark to give me some readable profiling data, FWIW. Or do you guys think it is unnecessary?
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.
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
Created attachment 278858 [details]
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!
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

11 years ago
(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.
Ok, I backed this puppy out pending further analysis. Guys, tell me what you'd like me to do?
QA Contact: pschwartau → caps
Comment on attachment 278652 [details] [diff] [review]
Patch

Please rerequest approval when the perf issue is resolved.
Attachment #278652 - Flags: approval1.9+
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).
It should help a huge amount, yes;  Ben, can you re-profile and find out?
(Reporter)

Updated

11 years ago
Assignee: jst → bent.mozilla
Status: REOPENED → NEW
Whiteboard: [prof-wanted]
I won't have cycles to look at this for a while.

-> Default owner.
Assignee: bent.mozilla → dveditz
Created attachment 310159 [details] [diff] [review]
previous patch updated to trunk

I had trouble patching the previous patch to trunk, so here is the updated version
Created attachment 310160 [details]
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

11 years ago
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.
(Reporter)

Comment 58

10 years ago
Created attachment 379463 [details] [diff] [review]
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.
Attachment #379463 - Flags: review?(jst)
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..
(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

Updated

9 years ago
Blocks: 357052
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.
Attachment #379463 - Flags: review?(jst)

Updated

5 years ago
See Also: → bug 383330
You need to log in before you can comment on or make changes to this bug.