Closed Bug 371858 Opened 13 years ago Closed 13 years ago

[FIX]Pushing null JSContext on the stack doesn't prevent bogus subject principals


(Core :: Security, defect, P1)






(Reporter: bzbarsky, Assigned: bzbarsky)




(Keywords: fixed1.8.0.13, verified1.8.1.5)


(4 files, 2 obsolete files)


1) Load attached testcase.
2) Click the button

EXPECTED RESULTS:  An alert and an error page about '/' being an invalid URI or some such.

ACTUAL RESULTS:  The error page is broken.

DETAILS:  The problem is that the JSContext lives on the _outer_ window, not the inner one.  As a result, when we go to compile script for new document (the error page), we push the JSContext for its window on the stack... and this JSContext has a non-null fp, and fp->down->script is the script that put up the alert.  So we get a subject principal that's not allowed to touch the inner window of the error page, an exception gets thrown, and the error page doesn't work right.

We should discuss this at the on-site.  We need a better setup for getting subject principals.

In case people care, the stack to the security check is:

#1  0x009c7a07 in nsScriptSecurityManager::CheckPropertyAccess (this=0x958e528, 
    cx=0xb5e771d8, aJSObject=0x9fa5360, aClassName=0x6f76fc0 "Window", 
    aProperty=156657820, aAction=2)
    at ../../../mozilla/caps/src/nsScriptSecurityManager.cpp:519
#2  0x06d325ea in nsDOMClassInfo::doCheckPropertyAccess (this=0xb5e66f70, cx=0xb5e771d8, 
    obj=0x9fa5360, id=156657820, wrapper=0xa0cfce0, accessMode=2, isWindow=1)
    at ../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:4025
#3  0x06d33bbe in nsWindowSH::AddProperty (this=0xb5e66f70, wrapper=0xa0cfce0, 
    cx=0xb5e771d8, obj=0x9fa5360, id=156657820, vp=0xbffd4178, _retval=0xbffd40f8)
    at ../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:4594
#4  0x057f2913 in XPC_WN_Helper_AddProperty (cx=0xb5e771d8, obj=0x9fa5360, 
    idval=156657820, vp=0xbffd4178)
#8  0x008a0c52 in JS_InitClass (cx=0xb5e771d8, obj=0x9fa5360, parent_proto=0x9fa5280, 
    clasp=0x977e00, constructor=0x92ff56 <RegExp>, nargs=1, ps=0x977ca0, fs=0x977e60, 
    static_ps=0x977d00, static_fs=0x0) at ../../../mozilla/js/src/jsapi.c:2201
#9  0x00930071 in js_InitRegExpClass (cx=0xb5e771d8, obj=0x9fa5360)
    at ../../../mozilla/js/src/jsregexp.c:4127
#10 0x00904894 in js_GetClassObject (cx=0xb5e771d8, obj=0x9fa5360, key=JSProto_RegExp, 
    objp=0xbffd4338) at ../../../mozilla/js/src/jsobj.c:2571

Presumably we're compiling one of the regexp literals in neterror.xhtml....
Flags: blocking1.9?
Oh, this was originally reported by Michal Zalewski in bug 370445 comment 46.
Attached file Testcase
Attached file Full stack
Depends on: 377090
So it seems that the right approach is to null out cx->fp when another context is pushed on top of cx on the JSContext stack, then restore it when the pop happens.

Filed bug 377090 on JS engine API for this.
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Is this realistically a release-blocker if it's depending on a JS API change that hasn't happened on trunk? The neterror testcase isn't all that serious, are there worse examples that say we can't wait?

We'll approve a patch for this, but unless there's additional info I don't think it's a blocker. You're already on the hook for other more serious bugs in this release.
Assignee: dveditz → bzbarsky
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
> are there worse examples

We haven't thought of any yet.  But we're not completely sure, to be honest.

I'm fine with just approving the patch if we get to it, yeah.
Attached patch Like so? (obsolete) — Splinter Review
Attachment #261915 - Flags: superreview?(brendan)
Attachment #261915 - Flags: review?(jst)
Flags: in-testsuite?
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Summary: Pushing null JSContext on the stack doesn't prevent bogus subject principals → [FIX]Pushing null JSContext on the stack doesn't prevent bogus subject principals
Target Milestone: --- → mozilla1.9alpha4
Flags: blocking1.9? → blocking1.9+
Comment on attachment 261915 [details] [diff] [review]
Like so?

Looks good, r=jst
Attachment #261915 - Flags: review?(jst) → review+
Target Milestone: mozilla1.9alpha4 → mozilla1.9alpha5
Comment on attachment 261915 [details] [diff] [review]
Like so?

> /* JSContext peek (); */
> XPCJSContextStack::Peek(JSContext * *_retval)
> {
>-    *_retval = (JSContext*) mStack.Peek();
>+    if(mStack.IsEmpty())
>+        *_retval = nsnull;
>+    else
>+        *_retval = mStack[mStack.Length() - 1].cx;

Nit/suggestion: common the LHS via *_retval = ... ? ... : ...;

>+    if(idx > 0) {
>+        --idx; // Advance to new top of the stack
>+        JSContextAndFrame & e = mStack[idx];
>+        NS_ASSERTION(!e.frame ||, "Shouldn't have frame without a cx!");
>+        if( {

Nit: brace style deviates here, but if( crunching follows jband style. Either use if (...) { or else use jband style consistently. Other cases follow in the patch.

sr=me with nits addressed.

Attachment #261915 - Flags: superreview?(brendan) → superreview+
Attachment #261915 - Attachment is obsolete: true
Comment on attachment 263833 [details] [diff] [review]
Updated to comments

Worth considering on branch...  Not sure how to evaluate the risk, though.  Basically, if we don't crash I think we should work right.  ;)
Attachment #263833 - Flags: approval1.8.1.5?
Attachment #263833 - Flags: approval1.8.0.13?
This got checked in on trunk at 2007-05-04 22:55.
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 263833 [details] [diff] [review]
Updated to comments

approved for and, a=dveditz for release-drivers
Attachment #263833 - Flags: approval1.8.1.5?
Attachment #263833 - Flags: approval1.8.1.5+
Attachment #263833 - Flags: approval1.8.0.13?
Attachment #263833 - Flags: approval1.8.0.13+
I landed this for, but for the 1.8.0 branch we'll need a new patch as there's no nsTArray on that branch. Working on that...
Keywords: fixed1.8.1.5
Attached patch 1.8.0 branch fix (obsolete) — Splinter Review
Attachment #273075 - Flags: review?(jst)
(In reply to comment #15)
> Created an attachment (id=273075) [details]
> 1.8.0 branch fix

Is it me or is this patch unrelated ?
Ah, yes, Boris attached the wrong patch, he accidentally attached the patch for bug 388579.
Attachment #273075 - Attachment is obsolete: true
Attachment #273312 - Flags: review?(jst)
Attachment #273075 - Flags: review?(jst)
Flags: blocking1.8.0.13?
Attachment #273312 - Flags: review?(jst) → review+
Attachment #273312 - Flags: approval1.8.0.13?
Depends on: 390488
Attachment #263833 - Flags: approval1.8.0.13+
Comment on attachment 273312 [details] [diff] [review]
The right 1.8.0 patch

approved for, a=dveditz for release-drivers

We're closing the tree soon for
Attachment #273312 - Flags: approval1.8.0.13? → approval1.8.0.13+
Fixed for
Keywords: fixed1.8.0.13
Flags: blocking1.8.0.13? → blocking1.8.0.13+
v.fixed branch and trunk

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv: Gecko/2008040413 Firefox/

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre
You need to log in before you can comment on or make changes to this bug.