609990 - JS_ASSERT(!initialVarObj->getOps()->defineProperty); triggered while browsing hulu.com

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Smedberg]

I was browsing hulu.com in a debug build (today's trunk) an hit an assertion:

    JS_ASSERT(!initialVarObj->getOps()->defineProperty);


initialVarObj->name = "Proxy" clasp=j::ObjectProxyClass and it defineProperty is js::proxy_DefineProperty.

Stack:

 	mozjs.dll!JS_Assert(s=0x6d1904ec, file=0x6d1904a0, ln=0x000003c3)  Line 73	C++
>	mozjs.dll!js::Execute(cx=0x04ef9c30, chain=0x10d8c048, script=0x125503d0, prev=0x00000000, flags=0x00000000, result=0x003dc630)  Line 963	C++
 	mozjs.dll!JS_EvaluateUCScriptForPrincipals(cx=0x04ef9c30, obj=0x10d8c048, principals=0x0f9e33a4, chars=0x1221ea48, length=0x0000004e, filename=0x0fae4fa0, lineno=0x00000000, rval=0x003dc630)  Line 4819	C++
 	mozjs.dll!JS_EvaluateUCScriptForPrincipalsVersion(cx=0x04ef9c30, obj=0x10d8c048, principals=0x0f9e33a4, chars=0x1221ea48, length=0x0000004e, filename=0x0fae4fa0, lineno=0x00000000, rval=0x003dc630, version=JSVERSION_DEFAULT)  Line 4795	C++
 	xul.dll!nsJSContext::EvaluateStringWithValue(aScript={...}, aScopeObject=0x10d8c048, aPrincipal=0x0f9e33a0, aURL=0x0fae4fa0, aLineNo=0x00000000, aVersion=0x00000000, aRetValue=0x003dc820, aIsUndefined=0x00000000)  Line 1533	C++
 	xul.dll!mozilla::plugins::parent::_evaluate(npp=0x0cd625a4, npobj=0x0733dad0, script=0x003dc888, result=0x003dc860)  Line 1682	C++
 	xul.dll!mozilla::plugins::PluginScriptableObjectParent::AnswerNPN_Evaluate(aScript={...}, aResult=0x003dcd24, aSuccess=0x003dcd23)  Line 1234	C++
 	xul.dll!mozilla::plugins::PPluginScriptableObjectParent::OnCallReceived(__msg={...}, __reply=0x00000000)  Line 692	C++
 	xul.dll!mozilla::plugins::PPluginModuleParent::OnCallReceived(__msg={...}, __reply=0x00000000)  Line 601	C++
 	xul.dll!mozilla::ipc::RPCChannel::DispatchIncall(call={...})  Line 517	C++
 	xul.dll!mozilla::ipc::RPCChannel::Incall(call={...}, stackDepth=0x00000000)  Line 504	C++
 	xul.dll!mozilla::ipc::RPCChannel::OnMaybeDequeueOne()  Line 434	C++
 	xul.dll!DispatchToMethod<mozilla::ipc::RPCChannel,bool (__thiscall mozilla::ipc::RPCChannel::*)(void)>(obj=0x04204228, method=0x67829adc, arg={...})  Line 384	C++
 	xul.dll!RunnableMethod<mozilla::ipc::RPCChannel,bool (__thiscall mozilla::ipc::RPCChannel::*)(void),Tuple0>::Run()  Line 307	C++
 	xul.dll!mozilla::ipc::RPCChannel::RefCountedTask::Run()  Line 449	C++
 	xul.dll!mozilla::ipc::RPCChannel::DequeueTask::Run()  Line 474	C++
 	xul.dll!MessageLoop::RunTask(task=0x0726f6d0)  Line 344	C++
 	xul.dll!MessageLoop::DeferOrRunPendingTask(pending_task={...})  Line 354	C++
 	xul.dll!MessageLoop::DoWork()  Line 451	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(aDelegate=0x00649728)  Line 114	C++
 	xul.dll!MessageLoop::RunInternal()  Line 220	C++
 	xul.dll!MessageLoop::RunHandler()  Line 203	C++
 	xul.dll!MessageLoop::Run()  Line 177	C++
 	xul.dll!nsBaseAppShell::Run()  Line 187	C++
 	xul.dll!nsAppShell::Run()  Line 243	C++
 	xul.dll!nsAppStartup::Run()  Line 191	C++
 	xul.dll!XRE_main(argc=0x00000005, argv=0x00272280, aAppData=0x0063f5c0)  Line 3682	C++

Comment 1

[Benjamin Smedberg]

Nominating for blocking because I don't know how serious this assertion is.

<jorendorff> thanks. this is something we definitely used to permit; we probably need to hack it so it'll work somehow

Comment 2

[Luke Wagner [:luke]]

I hit this assertion today with a debug m-c build on the beta 7 branch.  Its also with the ObjectProxyClass.  I have it under gdb if anyone in MV wants to take a look...

Comment 3

[Luke Wagner [:luke]]

Blake looked at it.  The reason for the assert is that we do things like js_DefineProperty on the var obj which assume that the var obj is a native object.  In this case, JS_EvaluateUCScriptForPrincipal is being called from the NPAPI with a cross-origin-wrapped scope/this object.  Rather than trying to perform some outer/inner/wrap/unwrap-erizing of this thing, it seems more sensible just to define such a situation as an API usage violation and report an error.

Comment 4

[Benjamin Smedberg]

Sure, but that still involves fixing NPAPI to do the right thing. It's not clear to me what the right thing is in this case.

Comment 5

[Luke Wagner [:luke]]

As Blake explained it to me, NPAPI was simply carrying out the request of an NPAPI client, in which case, there is nothing to fix in NPAPI, its just an error.  Looking at the above call stack, it looks like the offending varobj ultimately comes form the mObject of the PluginScriptableObjectParent on which AnswerNPN_Evaluate was called.  I know practically nothing about NPAPI, but I'm guessing this means that the client is doing "o.evaluate()" on some cross-domain wrapped object o?

Comment 6

[Benjamin Smedberg]

Well, it previously worked, so if it doesn't work now I think we need to be very clear about what new restriction we're placing on it. I don't know what hulu/flash is doing in this case, but it's certainly possible that they are trying to evaluate in the context of a subframe with an accessible-but-different subframe.

Comment 7

[Andreas Gal :gal]

The new wrapper layer is more coherent than the previous wrappers and inject wrappers automatically. This might have worked previously due to an information leak/security bug. We should try to understand exactly whats going on here and then we can decide whether we have to fix this, or this not working is a feature. If its the latter we need to properly error out instead of getting this far though.

Comment 8

[Luke Wagner [:luke]]

Blake showed that the varobj was a cross-origin wrapper (hulu.com and some other domain).  In non-NPAPI, is it an error to do "w.eval()" for some cross-domain window w?  I thought I remember it was.  If so, it follows that it should also be an error if the analogous thing is done from NPAPI.

Comment 9

[Jeff Walden [:Waldo]]

It's an error if the two are not same-origin and if they have not joined their principals by setting document.domain to the same value.  But I have a feeling we don't want those semantics, given the mess they are in normal scripts.  ;-)  I won't speculate as to what semantics are desirable, in the event that previous semantics are unacceptable.

Comment 7 seems right that this may well be an information leak/security bug (an XSS hole, to be more specific), so I'm marking security-sensitive out of an abundance of caution.

Comment 10

[Andreas Gal :gal]

Thanks waldo. I should have done that myself but I blanked. Sorry.

Comment 11

[Brendan Eich [:brendan]]

However we get a non-native varobj, there should be an assertion as soon as the relevant member of JSObject * type is stored, or just before the the store, that varobj->isNative(). At that point, is there a way to unwrap and use the underlying object, or fail the attempt if there's a security threat?

/be

Comment 12

[David Mandelin [:dmandelin]]

Andreas, what do you want to do with this bug? It's been sitting for a while, and it's not clear to me what the bug is or whether it's reproducible, but it seems you had a plan for it.

Comment 13

[Andreas Gal :gal]

Another test cases from bug 608987:

http://blog.techsatish.net/2010/10/illavarasi-11-10-10.html

Comment 14

[Andreas Gal :gal]

Attachment: patch

It seems only flash triggers this. It tries to run code on a cross origin window. In particular, that script is trying to define functions on that cross origin window, which doesn't work anyway and would throw. So we just throw a little earlier here.

Comment 15

[Andreas Gal :gal]

Attachment: patch

Brendan is overloaded.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 501552 [details] [diff] [review]
patch

You have a patch below this on your q with JSMSG_NON_NATIVE_SCOPE added to js.msg?

/be

Comment 17

[Andreas Gal :gal]

Attachment: patch

r=brendan face to face

Comment 18

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/353e10fb0d11

Comment 19

[Andreas Gal :gal]

I don't think this is sg any more.

Comment 20

[Andreas Gal :gal]

(rationale: safe crash in betas and in 3.6 this simply throws)

Comment 21

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/353e10fb0d11