Last Comment Bug 371858 - [FIX]Pushing null JSContext on the stack doesn't prevent bogus subject principals
: [FIX]Pushing null JSContext on the stack doesn't prevent bogus subject princi...
: fixed1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9alpha5
Assigned To: Boris Zbarsky [:bz]
Depends on: 377090 390488
  Show dependency treegraph
Reported: 2007-02-26 18:49 PST by Boris Zbarsky [:bz]
Modified: 2008-05-07 15:06 PDT (History)
12 users (show)
jonas: blocking1.9+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.13+
dveditz: wanted1.8.0.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Testcase (242 bytes, text/html)
2007-02-26 18:51 PST, Boris Zbarsky [:bz]
no flags Details
Full stack (26.76 KB, text/plain)
2007-04-10 17:41 PDT, Boris Zbarsky [:bz]
no flags Details
Like so? (7.19 KB, patch)
2007-04-17 22:13 PDT, Boris Zbarsky [:bz]
jst: review+
brendan: superreview+
Details | Diff | Splinter Review
Updated to comments (6.05 KB, patch)
2007-05-04 22:52 PDT, Boris Zbarsky [:bz]
dveditz: approval1.8.1.5+
Details | Diff | Splinter Review
1.8.0 branch fix (1.26 KB, patch)
2007-07-19 19:31 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
The right 1.8.0 patch (8.95 KB, patch)
2007-07-22 09:50 PDT, Boris Zbarsky [:bz]
jst: review+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2007-02-26 18:49:16 PST

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....
Comment 1 Boris Zbarsky [:bz] 2007-02-26 18:50:49 PST
Oh, this was originally reported by Michal Zalewski in bug 370445 comment 46.
Comment 2 Boris Zbarsky [:bz] 2007-02-26 18:51:32 PST
Created attachment 256554 [details]
Comment 3 Boris Zbarsky [:bz] 2007-04-10 17:41:55 PDT
Created attachment 261200 [details]
Full stack
Comment 4 Boris Zbarsky [:bz] 2007-04-10 17:55:05 PDT
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.
Comment 5 Daniel Veditz [:dveditz] 2007-04-13 10:52:11 PDT
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.
Comment 6 Boris Zbarsky [:bz] 2007-04-13 19:45:38 PDT
> 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.
Comment 7 Boris Zbarsky [:bz] 2007-04-17 22:13:16 PDT
Created attachment 261915 [details] [diff] [review]
Like so?
Comment 8 Johnny Stenback (:jst, 2007-05-01 16:06:40 PDT
Comment on attachment 261915 [details] [diff] [review]
Like so?

Looks good, r=jst
Comment 9 Brendan Eich [:brendan] 2007-05-04 11:26:28 PDT
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.

Comment 10 Boris Zbarsky [:bz] 2007-05-04 22:52:56 PDT
Created attachment 263833 [details] [diff] [review]
Updated to comments
Comment 11 Boris Zbarsky [:bz] 2007-05-04 22:56:39 PDT
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.  ;)
Comment 12 Boris Zbarsky [:bz] 2007-05-07 10:00:50 PDT
This got checked in on trunk at 2007-05-04 22:55.
Comment 13 Daniel Veditz [:dveditz] 2007-06-22 11:36:32 PDT
Comment on attachment 263833 [details] [diff] [review]
Updated to comments

approved for and, a=dveditz for release-drivers
Comment 14 Johnny Stenback (:jst, 2007-06-22 17:34:06 PDT
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...
Comment 15 Boris Zbarsky [:bz] 2007-07-19 19:31:52 PDT
Created attachment 273075 [details] [diff] [review]
1.8.0 branch fix
Comment 16 Mike Hommey [:glandium] 2007-07-21 15:07:01 PDT
(In reply to comment #15)
> Created an attachment (id=273075) [details]
> 1.8.0 branch fix

Is it me or is this patch unrelated ?
Comment 17 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-07-21 15:23:00 PDT
Ah, yes, Boris attached the wrong patch, he accidentally attached the patch for bug 388579.
Comment 18 Boris Zbarsky [:bz] 2007-07-22 09:50:18 PDT
Created attachment 273312 [details] [diff] [review]
The right 1.8.0 patch
Comment 19 Daniel Veditz [:dveditz] 2007-08-07 11:31:02 PDT
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
Comment 20 Boris Zbarsky [:bz] 2007-08-07 20:53:38 PDT
Fixed for
Comment 21 Samuel Sidler (old account; do not CC) 2008-05-07 15:06:24 PDT
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

Note You need to log in before you can comment on or make changes to this bug.