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

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

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)
    at
    ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:944
....
#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 [:bzbarsky]]

Oh, this was originally reported by Michal Zalewski in bug 370445 comment 46.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Full stack

Comment 4

[Boris Zbarsky [:bzbarsky]]

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]]

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 [:bzbarsky]]

> 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 [:bzbarsky]]

Attachment: Like so?

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 261915 [details] [diff] [review]
Like so?

Looks good, r=jst

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 261915 [details] [diff] [review]
Like so?


> /* JSContext peek (); */
> NS_IMETHODIMP
> 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 || e.cx, "Shouldn't have frame without a cx!");
>+        if(e.cx) {

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.

/be

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 11

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

This got checked in on trunk at 2007-05-04 22:55.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 263833 [details] [diff] [review]
Updated to comments

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers

Comment 14

[Johnny Stenback (:jst)]

I landed this for 1.8.1.5, 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 [:bzbarsky]]

Attachment: 1.8.0 branch fix

Comment 16

[Mike Hommey [:glandium]]

(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 (dead)]

Ah, yes, Boris attached the wrong patch, he accidentally attached the patch for bug 388579.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: The right 1.8.0 patch

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 273312 [details] [diff] [review]
The right 1.8.0 patch

approved for 1.8.0.13, a=dveditz for release-drivers

We're closing the tree soon for 1.8.0.13

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8.0.13.

Comment 21

[Samuel Sidler (old account; do not CC)]

v.fixed branch and trunk

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre