Closed
Bug 371858
Opened 17 years ago
Closed 17 years ago
[FIX]Pushing null JSContext on the stack doesn't prevent bogus subject principals
Categories
(Core :: Security, defect, P1)
Core
Security
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha5
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
()
Details
(Keywords: fixed1.8.0.13, verified1.8.1.5)
Attachments
(4 files, 2 obsolete files)
242 bytes,
text/html
|
Details | |
26.76 KB,
text/plain
|
Details | |
6.05 KB,
patch
|
dveditz
:
approval1.8.1.5+
|
Details | Diff | Splinter Review |
8.95 KB,
patch
|
jst
:
review+
dveditz
:
approval1.8.0.13+
|
Details | Diff | Splinter Review |
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....
Flags: blocking1.9?
Assignee | ||
Comment 1•17 years ago
|
||
Oh, this was originally reported by Michal Zalewski in bug 370445 comment 46.
Assignee | ||
Comment 2•17 years ago
|
||
Assignee | ||
Comment 3•17 years ago
|
||
Assignee | ||
Comment 4•17 years ago
|
||
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?
Comment 5•17 years ago
|
||
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?
Assignee | ||
Comment 6•17 years ago
|
||
> 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.
Assignee | ||
Comment 7•17 years ago
|
||
Attachment #261915 -
Flags: superreview?(brendan)
Attachment #261915 -
Flags: review?(jst)
Assignee | ||
Updated•17 years ago
|
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 8•17 years ago
|
||
Comment on attachment 261915 [details] [diff] [review] Like so? Looks good, r=jst
Attachment #261915 -
Flags: review?(jst) → review+
Assignee | ||
Updated•17 years ago
|
Target Milestone: mozilla1.9alpha4 → mozilla1.9alpha5
Comment 9•17 years ago
|
||
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
Attachment #261915 -
Flags: superreview?(brendan) → superreview+
Assignee | ||
Comment 10•17 years ago
|
||
Attachment #261915 -
Attachment is obsolete: true
Assignee | ||
Comment 11•17 years ago
|
||
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?
Assignee | ||
Comment 12•17 years ago
|
||
This got checked in on trunk at 2007-05-04 22:55.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 13•17 years ago
|
||
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
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+
Comment 14•17 years ago
|
||
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...
Keywords: fixed1.8.1.5
Assignee | ||
Comment 15•17 years ago
|
||
Attachment #273075 -
Flags: review?(jst)
Comment 16•17 years ago
|
||
(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•17 years ago
|
||
Ah, yes, Boris attached the wrong patch, he accidentally attached the patch for bug 388579.
Assignee | ||
Comment 18•17 years ago
|
||
Attachment #273075 -
Attachment is obsolete: true
Attachment #273312 -
Flags: review?(jst)
Attachment #273075 -
Flags: review?(jst)
Updated•17 years ago
|
Flags: blocking1.8.0.13?
Updated•17 years ago
|
Attachment #273312 -
Flags: review?(jst) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #273312 -
Flags: approval1.8.0.13?
Updated•17 years ago
|
Attachment #263833 -
Flags: approval1.8.0.13+
Comment 19•17 years ago
|
||
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
Attachment #273312 -
Flags: approval1.8.0.13? → approval1.8.0.13+
Updated•17 years ago
|
Flags: blocking1.8.0.13? → blocking1.8.0.13+
Comment 21•16 years ago
|
||
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.5 → verified1.8.1.5
You need to log in
before you can comment on or make changes to this bug.
Description
•