Closed
Bug 609990
Opened 14 years ago
Closed 14 years ago
JS_ASSERT(!initialVarObj->getOps()->defineProperty); triggered while browsing hulu.com
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: benjamin, Assigned: gal)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
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++
Reporter | ||
Comment 1•14 years ago
|
||
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
blocking2.0: --- → ?
Comment 2•14 years ago
|
||
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•14 years ago
|
||
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.
Reporter | ||
Comment 4•14 years ago
|
||
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•14 years ago
|
||
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?
Reporter | ||
Comment 6•14 years ago
|
||
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.
Assignee | ||
Comment 7•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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.
Group: core-security
Assignee | ||
Comment 10•14 years ago
|
||
Thanks waldo. I should have done that myself but I blanked. Sorry.
Comment 11•14 years ago
|
||
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
Updated•14 years ago
|
Whiteboard: [sg:high] xss possibility in comment 9
Updated•14 years ago
|
blocking2.0: ? → betaN+
Updated•14 years ago
|
Whiteboard: [sg:high] xss possibility in comment 9 → [sg:high][xss possibility in comment 9][softblocker]
Comment 12•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Whiteboard: [sg:high][xss possibility in comment 9][softblocker] → [sg:high][xss possibility in comment 9][hardblocker]
Assignee | ||
Comment 13•14 years ago
|
||
Another test cases from bug 608987:
http://blog.techsatish.net/2010/10/illavarasi-11-10-10.html
Updated•14 years ago
|
Assignee: general → gal
Assignee | ||
Comment 14•14 years ago
|
||
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.
Attachment #501551 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #501551 -
Flags: review? → review?(brendan)
Assignee | ||
Comment 15•14 years ago
|
||
Brendan is overloaded.
Attachment #501551 -
Attachment is obsolete: true
Attachment #501552 -
Flags: review?(lw)
Attachment #501551 -
Flags: review?(brendan)
Comment 16•14 years ago
|
||
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
Assignee | ||
Comment 17•14 years ago
|
||
r=brendan face to face
Attachment #501552 -
Attachment is obsolete: true
Attachment #501567 -
Flags: review+
Attachment #501552 -
Flags: review?(lw)
Assignee | ||
Comment 18•14 years ago
|
||
Whiteboard: [sg:high][xss possibility in comment 9][hardblocker] → fixed-in-tracemonkey
Assignee | ||
Comment 20•14 years ago
|
||
(rationale: safe crash in betas and in 3.6 this simply throws)
Comment 21•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•