See bug 217195. When content defines eval as a setter for a property in a content DOM object that chrome calls, when eval runs it should use its "object principals", which come unalterably from its global object via the parent link. See bug 213946 for a typedef, +typedef JSPrincipals * +(* JS_DLL_CALLBACK JSGetObjectPrincipals)(JSContext *cx, JSObject *obj); that seems helpful here, if used to declare a JSRuntime member set by caps code. If set, obj_eval (jsobj.c) would call that hook on the parent of the eval funobj being called (argv[-2]), and prefer the result to the caller's principals. Can anyone think of native functions other than eval that need such treatment? Ah -- Script.prototype.compile and the Script constructor, of course. /be

Function, setTimeout, setInterval?

Taking. /be

So we need a getObjectPrincipals callback, probably per-context (so privileged contexts don't need to pay a price). If caps code sets this for DOM contexts, then we can kill this bird and the other one (bug 213946) with the same stone. There are several places in the JS engine that pass caller->script->principals on to callee. That's wrong on two counts: one, a native may be calling the callee, so it's necessary to loop (see jsfun.c, Function, which gets this right); two, the caller->script->principals should be trumped by caller function object principals in the case of a cloned function object (e.g. brutal sharing). Patch for 1.6b coming soon. /be

Note one difference from the patch in bug 213946: the callback to find an object principal from an object on a given cx is JSObjectPrincipalsFinder (the function pointer typedef), and it returns a weak ref to the JSPrincipals (so the impl. must not JSPRINCIPALS_HOLD). I took this opportunity to fix JSPRINCIPALS_HOLD and JSPRINCIPALS_DROP to use atomic ops #ifdef JS_THREADSAFE. diff -w version in a few seconds (I got rid of some trailing whitespace, and the indentation changed in a few places). /be

Comment on attachment 134083 [details] [diff] [review] diff -w version of last patch >+JS_PUBLIC_API(JSPrincipals *) >+JS_StackFramePrincipals(JSContext *cx, JSStackFrame *fp) >+{ >+ if (fp->fun && cx->findObjectPrincipals) { >+ JSObject *callee = JSVAL_TO_OBJECT(fp->argv[-2]); >+ >+ if (fp->fun->object != callee) >+ return cx->findObjectPrincipals(cx, callee); >+ /* FALL THROUGH */ >+ } Should we sample cx->fOP to a local, and then test that local, to protect against a race to unset it? >+/* >+ * Return a weak reference to the fp's principals. A null return does not >+ * mean error, it means no principals. >+ */ >+extern JS_PUBLIC_API(JSPrincipals *) >+JS_StackFramePrincipals(JSContext *cx, JSStackFrame *fp); Is it safe to return a weak reference from JS_StackFramePrincipals? Can another thread not race to destroy it, leaving the JS_SFP caller with a dangling pointer? (Or does the frame pointer, whose lifetime is guaranteed by the caller, hold a ref to the prins? I forget.) >@@ -1048,17 +1049,17 @@ obj_eval(JSContext *cx, JSObject *obj, u > ok = JS_FALSE; > goto out; > } > > #if !JS_BUG_EVAL_THIS_SCOPE > #if JS_HAS_SCRIPT_OBJECT > if (argc < 2) > #endif >- { >+ if (caller) { > /* Execute using caller's new scope object (might be a Call object). */ > scopeobj = caller->scopeChain; > } > #endif Let's give |if (caller)| its very own set of braces, and leave the existing set to optionally bound the EVAL_THIS_SCOPE/SCRIPT_OBJECT argc check. (I had to think about how an else appended to the caller-check's then would bind, and I don't like having to think about that sort of thing.) >+ * Return a weak reference to the principals associated with obj, possibly via >+ * the immutable parent chain leading from obj to a top-level container (e.g., >+ * a window object in the DOM level 0). If there are no principals associated >+ * with obj. Null does not mean an error was reported; in no event should an >+ * error or exception be thrown by this callback's implementation. >+ */ >+typedef JSPrincipals * >+(* JS_DLL_CALLBACK JSObjectPrincipalsFinder)(JSContext *cx, JSObject *obj); Again, should we ask cx->fOP implementors to JS_HOLD_PRINCIPALS the return value, so that it's not a weak ref? sr=shaver with the braces added, and the weak refs either explained away or made strong.

> Should we sample cx->fOP to a local, and then test that local, to protect > against a race to unset it? Nope, cx is per-thread, no racey. > Is it safe to return a weak reference from JS_StackFramePrincipals? Can > another thread not race to destroy it, leaving the JS_SFP caller with a > dangling pointer? (Or does the frame pointer, whose lifetime is guaranteed by > the caller, hold a ref to the prins? I forget.) The stack frame is still active, so its script can't be GC'd, so the script's strong ref to its principals lives till the stack unwinds. In the cloned function object case, the function object's object-principals trump the script's, but again those live as long as the function object (fp->argv[-2]) lives, because the object principals are owned by the parent of the funobj (the window object, in the DOM embedding). So refcounting sucks, and should be avoided where not needed. Just ask bryner, who deCOMtaminated nsIDocument recently. There is no need for a strong ref from the object-principals finder, and making it return one adds not only needless bumping and dropping (threadsafe, yet), but it cruds up the callers in jsfun.c, jsobj.c, and jsscript.c, who must remember to drop on all exit paths from the function, once the held ref is returned (were we to change fOP to hold). No C++ auto-class helpers! I'll fix the bracing gaffe, thanks for pointing that out. /be

Comment on attachment 134083 [details] [diff] [review] diff -w version of last patch >Index: jsapi.c >=================================================================== >RCS file: /cvsroot/mozilla/js/src/jsapi.c,v >retrieving revision 3.152 >diff -p -u -8 -w -r3.152 jsapi.c >--- jsapi.c 22 Oct 2003 06:26:03 -0000 3.152 >+++ jsapi.c 25 Oct 2003 01:32:55 -0000 >@@ -2869,24 +2869,48 @@ JS_SetReservedSlot(JSContext *cx, JSObje > JSMSG_RESERVED_SLOT_RANGE); > return JS_FALSE; > } > slot = JSSLOT_START(clasp) + index; > OBJ_SET_REQUIRED_SLOT(cx, obj, slot, v); > return JS_TRUE; > } > >+#ifdef JS_THREADSAFE >+JS_PUBLIC_API(uintN) It seems that jsrefcount is signed, so this probably should match. Why is jsrefcount signed though? Would it make more sense to change that to unsigned? >+JS_HoldPrincipals(JSContext *cx, JSPrincipals *principals) >+{ >+ return JS_ATOMIC_INCREMENT(&principals->refcount); >+} >+ >+JS_PUBLIC_API(uintN) Same as above. >+JS_DropPrincipals(JSContext *cx, JSPrincipals *principals) >+{ >+ return JS_ATOMIC_DECREMENT(&principals->refcount); >+} >+#endif >+ >Index: jsapi.h >=================================================================== >RCS file: /cvsroot/mozilla/js/src/jsapi.h,v >retrieving revision 3.80 >diff -p -u -8 -w -r3.80 jsapi.h >--- jsapi.h 21 Oct 2003 18:03:04 -0000 3.80 >+++ jsapi.h 25 Oct 2003 01:32:56 -0000 >@@ -1097,29 +1097,43 @@ JS_SetReservedSlot(JSContext *cx, JSObje > * Security protocol. > */ > struct JSPrincipals { > char *codebase; > void * (* JS_DLL_CALLBACK getPrincipalArray)(JSContext *cx, JSPrincipals *); > JSBool (* JS_DLL_CALLBACK globalPrivilegesEnabled)(JSContext *cx, JSPrincipals *); > > /* Don't call "destroy"; use reference counting macros below. */ >- uintN refcount; >+ jsrefcount refcount; > void (* JS_DLL_CALLBACK destroy)(JSContext *cx, struct JSPrincipals *); > }; > >-#define JSPRINCIPALS_HOLD(cx, principals) \ >- ((principals)->refcount++) >+#ifdef JS_THREADSAFE >+#define JSPRINCIPALS_HOLD(cx, principals) JS_HoldPrincipals(cx,principals) >+#define JSPRINCIPALS_DROP(cx, principals) JS_DropPrincipals(cx,principals) >+ >+extern JS_PUBLIC_API(uintN) >+JS_HoldPrincipals(JSContext *cx, JSPrincipals *principals); >+ >+extern JS_PUBLIC_API(uintN) >+JS_DropPrincipals(JSContext *cx, JSPrincipals *principals); >+ >+#else >+#define JSPRINCIPALS_HOLD(cx, principals) ((principals)->refcount++) > #define JSPRINCIPALS_DROP(cx, principals) \ >- ((--((principals)->refcount) == 0) \ >+ ((--(principals)->refcount == 0) \ > ? (*(principals)->destroy)((cx), (principals)) \ > : (void) 0) Do we want to make it possible to do jsrefcount foo = JSPRINCIPALS_DROP(cx, principals) and get the expected refcnt? It looks like it is currently possible to do this for JSPRINCIPALS_HOLD. I think that the addref/release stuff in caps which shares the nsIPrincipal refcnt with the jsprincipals would be able to make good use of this functionality. >+#endif > > extern JS_PUBLIC_API(JSPrincipalsTranscoder) > JS_SetPrincipalsTranscoder(JSRuntime *rt, JSPrincipalsTranscoder px); >+ >+extern JS_PUBLIC_API(JSObjectPrincipalsFinder) >+JS_SetObjectPrincipalsFinder(JSContext *cx, JSObjectPrincipalsFinder fop); > > /************************************************************************/ > > /* > * Functions and scripts. > */ > extern JS_PUBLIC_API(JSFunction *) > JS_NewFunction(JSContext *cx, JSNative call, uintN nargs, uintN flags, >Index: jsdbgapi.h >=================================================================== >RCS file: /cvsroot/mozilla/js/src/jsdbgapi.h,v >retrieving revision 3.17 >diff -p -u -8 -w -r3.17 jsdbgapi.h >--- jsdbgapi.h 19 Mar 2002 04:28:20 -0000 3.17 >+++ jsdbgapi.h 25 Oct 2003 01:32:56 -0000 >@@ -140,28 +140,41 @@ extern JS_PUBLIC_API(JSStackFrame *) > JS_FrameIterator(JSContext *cx, JSStackFrame **iteratorp); > > extern JS_PUBLIC_API(JSScript *) > JS_GetFrameScript(JSContext *cx, JSStackFrame *fp); > > extern JS_PUBLIC_API(jsbytecode *) > JS_GetFramePC(JSContext *cx, JSStackFrame *fp); > >-extern JS_PUBLIC_API(JSBool) >-JS_IsNativeFrame(JSContext *cx, JSStackFrame *fp); >+/* >+ * Get the closest scripted frame below fp. If fp is null, start from cx->fp. >+ */ >+extern JS_PUBLIC_API(JSStackFrame *) >+JS_GetScriptedCaller(JSContext *cx, JSStackFrame *fp); >+ >+/* >+ * Return a weak reference to the fp's principals. A null return does not >+ * mean error, it means no principals. I think you have a gratuitous "the" in the above comment's first sentence, and I think the second could probably be formed a little better. "A null return does not denote an error; it means there are no principals." perhaps? >+ */ >+extern JS_PUBLIC_API(JSPrincipals *) >+JS_StackFramePrincipals(JSContext *cx, JSStackFrame *fp); >Index: jsobj.c >@@ -1019,25 +1019,26 @@ obj_eval(JSContext *cx, JSObject *obj, u > /* From here on, control must exit through label out with ok set. */ > #endif > > #if JS_BUG_EVAL_THIS_SCOPE > /* An old version used the object in which eval was found for scope. */ > scopeobj = obj; > #else > /* Compile using caller's current scope object. */ >+ if (caller) > scopeobj = caller->scopeChain; > #endif > } > > str = JSVAL_TO_STRING(argv[0]); >- if (caller->script) { >+ if (caller) { > file = caller->script->filename; Are we guaranteed that caller->script is non-null here? > line = js_PCToLineNumber(cx, caller->script, caller->pc); >- principals = caller->script->principals; >+ principals = JS_StackFramePrincipals(cx, caller); > } else { > file = NULL; > line = 0; > principals = NULL; > } > > fp->flags |= JSFRAME_EVAL; > script = JS_CompileUCScriptForPrincipals(cx, scopeobj, principals, >@@ -1048,17 +1049,17 @@ obj_eval(JSContext *cx, JSObject *obj, u > ok = JS_FALSE; > goto out; > } > > #if !JS_BUG_EVAL_THIS_SCOPE > #if JS_HAS_SCRIPT_OBJECT > if (argc < 2) > #endif >- { >+ if (caller) { > /* Execute using caller's new scope object (might be a Call object). */ > scopeobj = caller->scopeChain; > } > #endif > ok = js_Execute(cx, scopeobj, script, caller, JSFRAME_EVAL, rval); > JS_DestroyScript(cx, script); > > out: >Index: jspubtd.h >=================================================================== >RCS file: /cvsroot/mozilla/js/src/jspubtd.h,v >retrieving revision 3.37 >diff -p -u -8 -w -r3.37 jspubtd.h >--- jspubtd.h 21 Oct 2003 18:03:04 -0000 3.37 >+++ jspubtd.h 25 Oct 2003 01:32:58 -0000 >@@ -540,12 +540,22 @@ typedef struct JSPrincipals JSPrincipals > * JSXDR_ENCODE, in which case *principalsp should be encoded; or JSXDR_DECODE, > * in which case implementations must return a held (via JSPRINCIPALS_HOLD), > * non-null *principalsp out parameter. Return true on success, false on any > * error, which the implementation must have reported. > */ > typedef JSBool > (* JS_DLL_CALLBACK JSPrincipalsTranscoder)(JSXDRState *xdr, > JSPrincipals **principalsp); >+ >+/* >+ * Return a weak reference to the principals associated with obj, possibly via >+ * the immutable parent chain leading from obj to a top-level container (e.g., >+ * a window object in the DOM level 0). If there are no principals associated >+ * with obj. Huh? :-) Null does not mean an error was reported; in no event should an >+ * error or exception be thrown by this callback's implementation. >+ */ >+typedef JSPrincipals * >+(* JS_DLL_CALLBACK JSObjectPrincipalsFinder)(JSContext *cx, JSObject *obj); > > JS_END_EXTERN_C > > #endif /* jspubtd_h___ */

> It seems that jsrefcount is signed, so this probably should match. Why is > jsrefcount signed though? Would it make more sense to change that to unsigned? jsrefcount is int32 (aka PRInt32) because that's what PR_AtomicIncrement takes a pointer to, same for PR_AtomicDecrement. I fixed the return types for the new JS API entry points. > Do we want to make it possible to do > > jsrefcount foo = JSPRINCIPALS_DROP(cx, principals) > > and get the expected refcnt? Oops. And I blew JSPRINCIPALS_HOLD in the !JS_THREADSAFE case -- it needs to preincrement, to match PR_AtomicIncrement. Here are the new non-threadsafe versions of the macros: #define JSPRINCIPALS_HOLD(cx, principals) (++(principals)->refcount) #define JSPRINCIPALS_DROP(cx, principals) \ ((--(principals)->refcount == 0) \ ? ((*(principals)->destroy)((cx), (principals)), 0) \ : (principals)->refcount) > I think you have a gratuitous "the" in the above comment's first sentence, > and I think the second could probably be formed a little better. Fixed, thanks. > Are we guaranteed that caller->script is non-null here? Yes, because caller was set to JS_GetScriptedCaller's return value. If it's non-null, then caller->script must be non-null. > Huh? :-) Fixed that too -- thanks! /be

> >- if (caller->script) { > >+ if (caller) { > > file = caller->script->filename; >Are we guaranteed that caller->script is non-null here? Yes we are, by way of JS_GetScriptedCaller above.

This is the diff -w, as I'm just looking for review stamps. /be

Comment on attachment 134287 [details] [diff] [review] fixed per caillon's comments So close -- dazzled by the minimal and beauteous changes and API extensions here, we all seem to have forgotten that the point of this bug is to make sure eval's native function object principals trump its caller's principals, so that content can't stick eval as the setter for a link href and cause a javascript: url to be eval'ed as a labeled expression statement that runs with chrome privs, due to chrome setting href naively (as in hyatt's patch for instantaneous visited link coloring). Whew. I will have a new patch later today. What's here is good, it just doesn't go the final distance. /be

Comment on attachment 134287 [details] [diff] [review] fixed per caillon's comments Looks better to me, but I think we all know that caillon's review is where it's at for this patch. r=shaver

Oops. Yeah, I got caught up in jsapi land studying the means and forgot about the way. Thanks for bringing us all back into check!

I added JS_EvalFramePrincipals to jsdbgapi.h, used by eval and friends in conjunction with JS_GetScriptedCaller. JS_EvalFramePrincipals prefers the object principals for fp->argv[-2] (which is the "callee" jsval for eval, Function, or Script) to the caller's JS_StackFramePrincipals if cx->findObjectPrincipals is non-null. Embeddings that don't use principals, or that don't set cx->fOP, work as before. Mozilla, once we change caps to set cx->fOP, will run eval with *that particular eval method's* object principals (for example, w.eval for content window w loading evil content, where content script has set its window's eval method as the setter of a link.href, in order to run a javascript: URL as a chrome script -- bug 217195). /be

Passes the js testsuite, btw. /be

Comment on attachment 134434 [details] [diff] [review] diff -w version of last patch *stamp*; sr=shaver

Comment on attachment 134434 [details] [diff] [review] diff -w version of last patch Looks good from where I stand. r=caillon.

Checked in. Caillon, can you patch caps to provide the needed fOP callback? Thanks, /be

This patch broke the build, thanks to the following caller of JSPRINCIPALS_HOLD which is now passing a variable that doesn't exist to a function instead of just a macro: NS_IMETHODIMP nsPrincipal::GetJsPrincipals(JSPrincipals **jsprin) { NS_PRECONDITION(mJSPrincipals.nsIPrincipalPtr, "mJSPrincipals is uninitalized!"); *jsprin = &mJSPrincipals; // JSPRINCIPALS_HOLD does not use its first argument. // Just use a dummy cx to save the codesize. JSPRINCIPALS_HOLD(cx, *jsprin); return NS_OK; }

Comment on attachment 134434 [details] [diff] [review] diff -w version of last patch >+JS_PUBLIC_API(jsrefcount) >+JS_DropPrincipals(JSContext *cx, JSPrincipals *principals) >+{ >+ return JS_ATOMIC_DECREMENT(&principals->refcount); >+} Doesn't this (or the macro) need to do something if the refcount hits zero?

I temporarily changed the dummy variables to null to fix the bustage, but I'm not happy with that fix, and I hope somebody changes it soon.

Sorry, flu still upon me, thanks to dbaron for cleaning up the mess. I fixed JS_DropPrincipals, but left it to caillon to re-parameterize GetJsPrincipals to take a JSContext* argument. /be

Since JSPRINCIPALS_HOLD and JSPRINCIPALS_DROP have been in jsapi.h since 1997 or so, I don't think we can change them to drop the cx parameter. So I'm hoping it's not to hard to change nsIPrincipal.idl: readonly attribute JSPrincipals jsPrincipals; to be a method. The somewhat odd "Js" spelling can be rectified. Aw heck, I should help. The files to change, from http://lxr.mozilla.org/mozilla/search?string=GetJSPrincipal, are caps/src/nsJSPrincipals.cpp caps/src/nsPrincipal.cpp caps/src/nsSystemPrincipal.cpp dom/src/base/nsJSEnvironment.cpp embedding/browser/activex/src/plugin/LegacyPlugin.cpp js/src/liveconnect/nsCLiveconnect.cpp js/src/xpconnect/loader/mozJSComponentLoader.cpp js/src/xpconnect/loader/mozJSSubScriptLoader.cpp modules/oji/src/lcglue.cpp security/manager/ssl/src/nsCrypto.cpp Patch anon. /be

Comment on attachment 134637 [details] [diff] [review] better fix for the missing cx param problem - readonly attribute JSPrincipals jsPrincipals; + JSPrincipals GetJSPrincipals(in JSContext cx); Pretend I spelled that method name "getJSPrincipals", which is how it's spelled now in my tree. Just for IDL interCaps (vs. C++ InterCaps) purity and symmetry with the rest of the methods in this non-scriptable interface. /be

Comment on attachment 134637 [details] [diff] [review] better fix for the missing cx param problem Forgot a struct JSContext; in nsIPrincipal.idl's %{C++ section. /be

This patch also fixes nsSystemPrincipal.cpp's JSPRINCIPALS_HOLD call to pass cx. /be

Comment on attachment 134672 [details] [diff] [review] working fix for the missing cx param problem Thanks for the quick turnaround on the patch. r=caillon.

I checked in attachment 134672 [details] [diff] [review]. /be

we should mark this fixed, and open a new bug for continuing work, right? that is the way to get things tested.... or is this all done?

Caillon, can you close, filing any followup bugs? I don't know of any, but I'm not thinking hard ATM. /be

This is fixed, no one has thought of any follow-on issues (which should go in a new bug, besides)

I've been asked to verify this on the 1.7 branch. Is there test case(s) available?

So we forgot to call JS_SetObjectPrincipalsFinder anywhere! Fortunately, jst did that in patching bug 289074. D'oh! Oh well, finally fixed for real. /be