Closed Bug 1149891 (CVE-2015-2731) Opened 9 years ago Closed 9 years ago

crash in CSPService::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) from userscript

Categories

(Core :: DOM: Security, defect)

36 Branch
x86_64
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 + verified
firefox38.0.5 --- wontfix
firefox40 + verified
firefox41 + verified
firefox-esr31 --- unaffected
firefox-esr38 39+ verified
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: her34her34, Assigned: smaug)

References

Details

(5 keywords, Whiteboard: [adv-main39+][adv-esr38.1+])

Crash Data

Attachments

(4 files, 1 obsolete file)

Attached file Crash.Default.zip (obsolete) —
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150122214805

Steps to reproduce:

Test profile attached.

Install profile.

On bookmark toolbar there is one saved bookmark "firefox crash". Click bookmark.


Actual results:

Firefox will crash as page is loading. If page finishes loading then click bookmark again. Crash usually happens by 3rd try.


Expected results:

Firefox should not crash. Previous versions of firefox do not crash.
No need to provide your profile.

Just give us better steps to reproduce. Which page did you bookmark to reproduce the crash?
Flags: needinfo?(her34her34)
We also need the crash report ID - if there is a crash listed in about:crashes, please paste it here. Thanks!
Flags: needinfo?(her34her34)
1)create new profile
2)install greasemonkey
3)install userscript firefox37crash@name.user.zip 
4)visit url: http://www.freewebarcade.com/game/the-power-of-love/
5)firefox should crash while loading page
6)if page finishes loading, reload page. Repeat reloading until crash.
Confirmed.

CR with FF40:
https://crash-stats.mozilla.com/report/index/44f5f100-163e-4b2e-bb46-b01f52150406
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ CSPService::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) ]
Component: Untriaged → DOM: Security
Ever confirmed: true
Keywords: crash, reproducible
Product: Firefox → Core
Summary: Firefox 37 crash from userscript → crash in CSPService::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) from userscript
Attached file test.html
NoScript、Ghostery can also crash firefox if you open test.html

https://crash-stats.mozilla.com/report/index/7e994356-2556-443b-9b72-b1ede2150411
Xu, thank for the testcase, it crashes FF even without any add-on.

I got a various crash signature with the latest Nightly:
https://crash-stats.mozilla.com/report/index/da13c11f-03cd-43d8-8a08-5696d2150411

Regression range:
good=2014-03-25
bad=2014-03-26
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5c0673441fc8&tochange=140ac04d7454
Crash Signature: [@ CSPService::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) ] → [@ CSPService::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) ] [@ nsContentPolicy::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupport…
Keywords: regression
Version: 37 Branch → 31 Branch
Using testcase of Comment 6, I get a different regression window:

Firefox35.0.1 do not crash.
bp-af6ce1cb-dc81-4a80-9ce5-504b42150411 on Firefox36.0.4
bp-9a10573c-8741-46c1-b8cb-49a282150411 on Firefox37.0.1
bp-f01f73ea-1e11-490d-8d3a-60c372150411 on Beta38.0b3
bp-85497c28-140a-493c-a57b-321772150411 on Aurora39.0a2
bp-bde93daf-b9dd-4d34-9532-b839d2150411 on Nightly39.0a1 without e10s
bp-6037afd1-f2be-456a-991f-0e0972150411 on Nightly39.0a1 with e10s tab crashed

Pushlog: 
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=70a6139f5517&tochange=044d2a98a497

triggered by: Bug 1081038
In fact, even if no NoScript or Ghostery, run testcase of Comment 6 twice, also can get a crash.
(In reply to Alice0775 White from comment #8)
> Using testcase of Comment 6, I get a different regression window:


Ok, I got the crash history of this bug. A crash in the past has been fixed, and now, it regressed a 2nd time.

1st Regression:
good1=2014-03-25
bad1=2014-03-26
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5c0673441fc8&tochange=140ac04d7454

This regression has been fixed in the past:
bad1=2014-08-22
good2=2014-08-23
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b9dd32d1e16&tochange=64c4ec2df3d4

Then it regressed again (this current bug). 2nd regression:
good2=2014-11-14
bad2=2014-11-15
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7f0d92595432&tochange=acbd7b68fa8c
with Bug 1081038 in the range, like you.
Blocks: 1081038
Flags: needinfo?(gkrizsanits)
Version: 31 Branch → 36 Branch
I don't think this crash is related to the microtask work I did in the mentioned patch. Why do you guys think that it was the reason for the crash? I see patches in the range those changing code in LoadInfo for example... Anyway, someone should debug this and see what's going wrong. I don't think I will have the time for it this week.
Flags: needinfo?(gkrizsanits)
[Tracking Requested - why for this release]:
Tracking for 39+ since this is a relatively recent regression - from Firefox 36. 

Christoph, is this something you might have time to look at?
Flags: needinfo?(mozilla)
Mhm, nothing obvious goes wrong here, but I can reproduce on Linux in e10s and also non e10s mode by only running the provided testcase from Comment 6. Please find a stacktrace underneath. Interestingly, it doesn't fail within nsCSPService::ShouldLoad() but in nsContentPolicy::ShouldLoad(), but all the arguments seem to be correct from a first glance.

I am not sure at this time, but maybe Bill can help, because shouldLoad is enforcing a "simple-content-policy", which was introduced with bug 1128798 and landed in 38. Given that 37 is also affected (see Comment 12) most likely indicates that Bug 1128798 is not directly related.

Bill, what do you think?


#6  0x00007fffefe529b1 in nsContentPolicy::ShouldLoad (this=0x7fffd9974600, contentType=5, contentLocation=0x7fffd8fc6f00, requestingLocation=0x7fffd99fb000, 
    requestingContext=0x7fffe4bc0800, mimeType=..., extra=0x0, requestPrincipal=0x7fffdb671c80, decision=0x7fffffffb2bc)
    at /home/ckerschb/moz/mc/dom/base/nsContentPolicy.cpp:243
#7  0x00007fffef7f9bd4 in NS_CheckContentLoadPolicy (contentType=5, contentLocation=0x7fffd8fc6f00, originPrincipal=0x7fffdb671c80, context=0x7fffe4bc0800, mimeType=..., 
    extra=0x0, decision=0x7fffffffb2bc, policyService=0x7fffd9974600, aSecMan=0x7fffe26c7290) at ../../dist/include/nsContentPolicyUtils.h:218
#8  0x00007fffefd38f65 in nsObjectLoadingContent::CheckLoadPolicy (this=0x7fffe4bc0890, aContentPolicy=0x7fffffffb2bc)
    at /home/ckerschb/moz/mc/dom/base/nsObjectLoadingContent.cpp:1487
#9  0x00007fffefd35935 in nsObjectLoadingContent::LoadObject (this=0x7fffe4bc0890, aNotify=true, aForceLoad=false, aLoadingChannel=0x0)
    at /home/ckerschb/moz/mc/dom/base/nsObjectLoadingContent.cpp:2131
#10 0x00007fffefd352b3 in nsObjectLoadingContent::LoadObject (this=0x7fffe4bc0890, aNotify=true, aForceLoad=false)
    at /home/ckerschb/moz/mc/dom/base/nsObjectLoadingContent.cpp:2003
#11 0x00007ffff11bca55 in mozilla::dom::HTMLSharedObjectElement::StartObjectLoad (this=0x7fffe4bc0800, aNotify=true)
    at /home/ckerschb/moz/mc/dom/html/HTMLSharedObjectElement.cpp:331
#12 0x00007ffff11d454a in mozilla::dom::HTMLSharedObjectElement::StartObjectLoad (this=0x7fffe4bc0800) at ../../dist/include/mozilla/dom/HTMLSharedObjectElement.h:83
#13 0x00007ffff11d8b23 in nsRunnableMethodArguments<>::apply<mozilla::dom::HTMLSharedObjectElement, void (mozilla::dom::HTMLSharedObjectElement::*)()>(mozilla::dom::HTMLSharedObjectElement*, void (mozilla::dom::HTMLSharedObjectElement::*)()) (this=0x7fffd8facb30, o=0x7fffe4bc0800, 
    m=(void (mozilla::dom::HTMLSharedObjectElement::*)(mozilla::dom::HTMLSharedObjectElement * const)) 0x7ffff11d4530 <mozilla::dom::HTMLSharedObjectElement::StartObjectLoad()>) at ../../dist/include/nsThreadUtils.h:602
#14 0x00007ffff11d895c in nsRunnableMethodImpl<void (mozilla::dom::HTMLSharedObjectElement::*)(), true>::Run (this=0x7fffd8facb00)
    at ../../dist/include/nsThreadUtils.h:809
#15 0x00007fffefc76cce in nsContentUtils::RemoveScriptBlocker () at /home/ckerschb/moz/mc/dom/base/nsContentUtils.cpp:5087
#16 0x00007fffefeb1d50 in nsDocument::EndUpdate (this=0x7fffdd84e000, aUpdateType=1) at /home/ckerschb/moz/mc/dom/base/nsDocument.cpp:5004
#17 0x00007ffff1229019 in nsHTMLDocument::EndUpdate (this=0x7fffdd84e000, aUpdateType=1) at /home/ckerschb/moz/mc/dom/html/nsHTMLDocument.cpp:2537
#18 0x00007fffef82e3c0 in nsHtml5DocumentBuilder::EndDocUpdate (this=0x7fffd8f0d000) at /home/ckerschb/moz/mc/parser/html/nsHtml5DocumentBuilder.h:81
#19 0x00007fffef859ba2 in nsHtml5TreeOpExecutor::DidBuildModel (this=0x7fffd8f0d000, aTerminated=false) at /home/ckerschb/moz/mc/parser/html/nsHtml5TreeOpExecutor.cpp:120
#20 0x00007fffef87b591 in nsHtml5TreeOperation::Perform (this=0x7fffd8f0fe78, aBuilder=0x7fffd8f0d000, aScriptElement=0x7fffffffbef0)
    at /home/ckerschb/moz/mc/parser/html/nsHtml5TreeOperation.cpp:819
#21 0x00007fffef85a746 in nsHtml5TreeOpExecutor::RunFlushLoop (this=0x7fffd8f0d000) at /home/ckerschb/moz/mc/parser/html/nsHtml5TreeOpExecutor.cpp:448
#22 0x00007fffef877b95 in nsHtml5ExecutorFlusher::Run (this=0x7fffd99ffc80) at /home/ckerschb/moz/mc/parser/html/nsHtml5StreamParser.cpp:127
#23 0x00007fffee689638 in nsThread::ProcessNextEvent (this=0x7fffe4bc5400, aMayWait=false, aResult=0x7fffffffc17e) at /home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:868
#24 0x00007fffee6dbfa7 in NS_ProcessNextEvent (aThread=0x7fffe4bc5400, aMayWait=false) at /home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:265
#25 0x00007fffeeccedeb in mozilla::ipc::MessagePump::Run (this=0x7fffe4b51e70, aDelegate=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:95
#26 0x00007fffeeccfc9b in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x7fffe4b51e70, aDelegate=0x7fffffffc5b8)
    at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:289
#27 0x00007fffeec557c5 in MessageLoop::RunInternal (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:233
#28 0x00007fffeec556f5 in MessageLoop::RunHandler (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:226
#29 0x00007fffeec556cd in MessageLoop::Run (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:200
#30 0x00007ffff1c56613 in nsBaseAppShell::Run (this=0x7fffde368800) at /home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:165
#31 0x00007ffff2c1b763 in XRE_RunAppShell () at /home/ckerschb/moz/mc/toolkit/xre/nsEmbedFunctions.cpp:738
#32 0x00007fffeeccfab6 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x7fffe4b51e70, aDelegate=0x7fffffffc5b8)
    at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:259
#33 0x00007fffeec557c5 in MessageLoop::RunInternal (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:233
#34 0x00007fffeec556f5 in MessageLoop::RunHandler (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:226
---Type <return> to continue, or q <return> to quit---
#35 0x00007fffeec556cd in MessageLoop::Run (this=0x7fffffffc5b8) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:200
#36 0x00007ffff2c1afe2 in XRE_InitChildProcess (aArgc=3, aArgv=0x7fffffffdb98, aGMPLoader=0x0) at /home/ckerschb/moz/mc/toolkit/xre/nsEmbedFunctions.cpp:575
#37 0x0000000000417850 in content_process_main (argc=5, argv=0x7fffffffdb98) at /home/ckerschb/moz/mc/ipc/app/../contentproc/plugin-container.cpp:236
#38 0x0000000000417942 in main (argc=6, argv=0x7fffffffdb98) at /home/ckerschb/moz/mc/ipc/app/MozillaRuntimeMain.cpp:11
Flags: needinfo?(mozilla) → needinfo?(wmccloskey)
Well, the problem seems to be that nsObjectLoadingContent::mURI has been freed and now we're using it. If you look at the Greasemonkey script, it installs a mutation observer that immediately removes any <object> element right after it's added to the DOM. It sounds like we have a UAF on some object related to nsObjectLoadingContent.

I guess the question is whether we're running the mutation observer when we shouldn't be (which could be caused by Gabor's bug, since it fiddles with microtasks, and those sound like they might be able to run mutation observers), OR whether we just have a pre-existing UAF in plugin code that was exposed by Gabor's bug.

I'm going to close this bug for now just to be safe. It sounds pretty exploitable.

I'm not sure who should look into this. Maybe Aaron or Bobby will have some ideas.
Group: core-security
Flags: needinfo?(wmccloskey) → needinfo?(aklotz)
This is definitely a regression from Bug 1081038.

Before that when parser created DOM, MutationObserver was notified at the end of the current task, as expected.
Now it happens whenever there happens to be a JS call, even a C++ to chrome JS one.
And nsIContentPolicy implementations aren't supposed to
modify DOM, but this surprising MutationObserver callback does that now.
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIContentPolicy.idl?rev=9ba87714f5cf&mark=64-79#64


#2  0x00007ffff0389044 in nsObjectLoadingContent::UnloadObject (this=0x7fffcf3b33d0, aResetState=true) at /home/smaug/mozilla/hg/inbound2/dom/base/nsObjectLoadingContent.cpp:2584
#3  0x00007ffff0388e0b in nsObjectLoadingContent::UnbindFromTree (this=0x7fffcf3b33d0, aDeep=true, aNullParent=true) at /home/smaug/mozilla/hg/inbound2/dom/base/nsObjectLoadingContent.cpp:693
#4  0x00007ffff181a657 in mozilla::dom::HTMLSharedObjectElement::UnbindFromTree (this=0x7fffcf3b3340, aDeep=true, aNullParent=true) at /home/smaug/mozilla/hg/inbound2/dom/html/HTMLSharedObjectElement.cpp:173
#5  0x00007ffff05aabbc in nsINode::doRemoveChildAt (this=0x7fffc3ee9d80, aIndex=1, aNotify=true, aKid=0x7fffcf3b3340, aChildArray=...) at /home/smaug/mozilla/hg/inbound2/dom/base/nsINode.cpp:1643
#6  0x00007ffff0409245 in mozilla::dom::FragmentOrElement::RemoveChildAt (this=0x7fffc3ee9d80, aIndex=1, aNotify=true) at /home/smaug/mozilla/hg/inbound2/dom/base/FragmentOrElement.cpp:1151
#7  0x00007ffff05a72c6 in nsINode::RemoveChild (this=0x7fffc3ee9d80, aOldChild=..., aError=...) at /home/smaug/mozilla/hg/inbound2/dom/base/nsINode.cpp:537
#8  0x00007ffff09d229b in mozilla::dom::NodeBinding::removeChild (cx=0x7fffd18e3e30, obj=..., self=0x7fffc3ee9d80, args=...) at ./NodeBinding.cpp:732
#9  0x00007ffff1477024 in mozilla::dom::GenericBindingMethod (cx=0x7fffd18e3e30, argc=1, vp=0x7fffe0d09270) at /home/smaug/mozilla/hg/inbound2/dom/bindings/BindingUtils.cpp:2609
#10 0x00007ffff4308d3b in js::CallJSNative (cx=0x7fffd18e3e30, native=0x7ffff1476d90 <mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/smaug/mozilla/hg/inbound2/js/src/jscntxtinlines.h:235
#11 0x00007ffff429f69f in js::Invoke (cx=0x7fffd18e3e30, args=..., construct=js::NO_CONSTRUCT) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:727
#12 0x00007ffff42ba7cf in Interpret (cx=0x7fffd18e3e30, state=...) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:2955
#13 0x00007ffff42acece in js::RunScript (cx=0x7fffd18e3e30, state=...) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:677
#14 0x00007ffff429f7bc in js::Invoke (cx=0x7fffd18e3e30, args=..., construct=js::NO_CONSTRUCT) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:746
#15 0x00007ffff42ba7cf in Interpret (cx=0x7fffd18e3e30, state=...) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:2955
#16 0x00007ffff42acece in js::RunScript (cx=0x7fffd18e3e30, state=...) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:677
#17 0x00007ffff429f7bc in js::Invoke (cx=0x7fffd18e3e30, args=..., construct=js::NO_CONSTRUCT) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:746
#18 0x00007ffff4287724 in js::Invoke (cx=0x7fffd18e3e30, thisv=..., fval=..., argc=2, argv=0x7fffffff9620, rval=...) at /home/smaug/mozilla/hg/inbound2/js/src/vm/Interpreter.cpp:783
#19 0x00007ffff4978a46 in JS::Call (cx=0x7fffd18e3e30, thisv=..., fval=..., args=..., rval=...) at /home/smaug/mozilla/hg/inbound2/js/src/jsapi.cpp:4401
#20 0x00007ffff0990417 in mozilla::dom::MutationCallback::Call (this=0x7fffcc019850, cx=0x7fffd18e3e30, aThisVal=..., mutations=..., observer=..., aRv=...) at ./MutationObserverBinding.cpp:617
#21 0x00007ffff0532b5b in mozilla::dom::MutationCallback::Call<nsDOMMutationObserver*> (this=0x7fffcc019850, thisVal=@0x7fffffff9948: 0x7fffd56601b0, mutations=..., observer=..., aRv=..., 
    aExecutionReason=0x7ffff4ebe972 <.L.str970> "MutationCallback", aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions, aCompartment=0x0)
    at ../../dist/include/mozilla/dom/MutationObserverBinding.h:178
#22 0x00007ffff04eb76f in nsDOMMutationObserver::HandleMutation (this=0x7fffd56601b0) at /home/smaug/mozilla/hg/inbound2/dom/base/nsDOMMutationObserver.cpp:778
#23 0x00007ffff04eb93a in nsDOMMutationObserver::HandleMutationsInternal () at /home/smaug/mozilla/hg/inbound2/dom/base/nsDOMMutationObserver.cpp:818
#24 0x00007ffff02de23a in nsDOMMutationObserver::HandleMutations () at /home/smaug/mozilla/hg/inbound2/dom/base/nsDOMMutationObserver.h:527
#25 0x00007ffff02ce07f in nsContentUtils::PerformMainThreadMicroTaskCheckpoint () at /home/smaug/mozilla/hg/inbound2/dom/base/nsContentUtils.cpp:5182
#26 0x00007ffff02ce00b in nsContentUtils::LeaveMicroTask () at /home/smaug/mozilla/hg/inbound2/dom/base/nsContentUtils.cpp:5151
#27 0x00007ffff0452a9d in mozilla::dom::AutoEntryScript::~AutoEntryScript (this=0x7fffffffa508) at /home/smaug/mozilla/hg/inbound2/dom/base/ScriptSettings.cpp:561
#28 0x00007fffefb3a05e in nsXPCWrappedJSClass::CallMethod (this=0x7fffd03e00b0, wrapper=0x7fffd2e62e00, methodIndex=7, info_=0x7fffe304a5f8, nativeParams=0x7fffffffa750)
    at /home/smaug/mozilla/hg/inbound2/js/xpconnect/src/XPCWrappedJSClass.cpp:1423
#29 0x00007fffefb3767b in nsXPCWrappedJS::CallMethod (this=0x7fffd2e62e00, methodIndex=7, info=0x7fffe304a5f8, params=0x7fffffffa750) at /home/smaug/mozilla/hg/inbound2/js/xpconnect/src/XPCWrappedJS.cpp:525
#30 0x00007fffeed04411 in PrepareAndDispatch (self=0x7fffd192cb80, methodIndex=7, args=0x7fffffffa8a0, gpregs=0x7fffffffa820, fpregs=0x7fffffffa850)
    at /home/smaug/mozilla/hg/inbound2/xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp:122
#31 0x00007fffeed0340b in SharedStub () from /home/smaug/mozilla/hg/inbound2/ff_build/dist/bin/libxul.so
#32 0x00007ffff31c815a in nsBrowserStatusFilter::OnSecurityChange (this=0x7fffd0388f70, aWebProgress=0x7fffd0492028, aRequest=0x0, aState=33)
    at /home/smaug/mozilla/hg/inbound2/toolkit/components/statusfilter/nsBrowserStatusFilter.cpp:264
#33 0x00007ffff31c81ad in non-virtual thunk to nsBrowserStatusFilter::OnSecurityChange(nsIWebProgress*, nsIRequest*, unsigned int) ()
    at /home/smaug/mozilla/hg/inbound2/toolkit/components/statusfilter/nsBrowserStatusFilter.cpp:265
#34 0x00007fffefdc9fff in nsDocLoader::OnSecurityChange (this=0x7fffd0492000, aContext=0x7fffcf3b3340, aState=33) at /home/smaug/mozilla/hg/inbound2/uriloader/base/nsDocLoader.cpp:1452
#35 0x00007ffff2dda8ae in nsDocShell::OnSecurityChange (this=0x7fffd0492000, i_Context=0x7fffcf3b3340, state=33) at /home/smaug/mozilla/hg/inbound2/docshell/base/nsDocShell.h:197
#36 0x00007ffff2ddac15 in non-virtual thunk to nsDocShell::OnSecurityChange(nsISupports*, unsigned int) () at Unified_cpp_docshell_base0.cpp:197
#37 0x00007ffff1c16a3a in nsMixedContentEvent::Run (this=0x7fffcc019eb0) at /home/smaug/mozilla/hg/inbound2/dom/security/nsMixedContentBlocker.cpp:106
#38 0x00007ffff02cc3b1 in nsContentUtils::AddScriptRunner (aRunnable=0x7fffcc019eb0) at /home/smaug/mozilla/hg/inbound2/dom/base/nsContentUtils.cpp:5134
#39 0x00007ffff1c1171d in nsMixedContentBlocker::ShouldLoad (aHadInsecureImageRedirect=false, aContentType=5, aContentLocation=0x7fffcf16ff00, aRequestingLocation=0x7fffcb2da000, 
    aRequestingContext=0x7fffcf3b3340, aMimeGuess=..., aExtra=0x0, aRequestPrincipal=0x7fffcaf6bfc0, aDecision=0x7fffffffbb2c) at /home/smaug/mozilla/hg/inbound2/dom/security/nsMixedContentBlocker.cpp:766
#40 0x00007ffff1c10149 in nsMixedContentBlocker::ShouldLoad (this=0x7fffd5d05380, aContentType=5, aContentLocation=0x7fffcf16ff00, aRequestingLocation=0x7fffcb2da000, aRequestingContext=0x7fffcf3b3340, 
    aMimeGuess=..., aExtra=0x0, aRequestPrincipal=0x7fffcaf6bfc0, aDecision=0x7fffffffbb2c) at /home/smaug/mozilla/hg/inbound2/dom/security/nsMixedContentBlocker.cpp:311
#41 0x00007ffff04b6eec in nsContentPolicy::CheckPolicy (this=0x7fffd6fdf290, 
    policyMethod=&virtual nsIContentPolicy::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*), 
    simplePolicyMethod=&virtual table offset 24, contentType=5, contentLocation=0x7fffcf16ff00, requestingLocation=0x7fffcb2da000, requestingContext=0x7fffcf3b3340, mimeType=..., extra=0x0, 
    requestPrincipal=0x7fffcaf6bfc0, decision=0x7fffffffbb2c) at /home/smaug/mozilla/hg/inbound2/dom/base/nsContentPolicy.cpp:127
#42 0x00007ffff04ab668 in nsContentPolicy::ShouldLoad (this=0x7fffd6fdf290, contentType=5, contentLocation=0x7fffcf16ff00, requestingLocation=0x7fffcb2da000, requestingContext=0x7fffcf3b3340, mimeType=..., 
    extra=0x0, requestPrincipal=0x7fffcaf6bfc0, decision=0x7fffffffbb2c) at /home/smaug/mozilla/hg/inbound2/dom/base/nsContentPolicy.cpp:237
#43 0x00007fffefe503a4 in NS_CheckContentLoadPolicy (contentType=5, contentLocation=0x7fffcf16ff00, originPrincipal=0x7fffcaf6bfc0, context=0x7fffcf3b3340, mimeType=..., extra=0x0, decision=0x7fffffffbb2c, 
    policyService=0x7fffd6fdf290, aSecMan=0x7fffe42809c0) at ../../dist/include/nsContentPolicyUtils.h:218
#44 0x00007ffff0390226 in nsObjectLoadingContent::CheckLoadPolicy (this=0x7fffcf3b33d0, aContentPolicy=0x7fffffffbb2c) at /home/smaug/mozilla/hg/inbound2/dom/base/nsObjectLoadingContent.cpp:1491
---Type <return> to continue, or q <return> to quit---
#45 0x00007ffff038cbb5 in nsObjectLoadingContent::LoadObject (this=0x7fffcf3b33d0, aNotify=true, aForceLoad=false, aLoadingChannel=0x0)
    at /home/smaug/mozilla/hg/inbound2/dom/base/nsObjectLoadingContent.cpp:2138
#46 0x00007ffff038c533 in nsObjectLoadingContent::LoadObject (this=0x7fffcf3b33d0, aNotify=true, aForceLoad=false) at /home/smaug/mozilla/hg/inbound2/dom/base/nsObjectLoadingContent.cpp:2010
gabor, bholley, would you have time to look at this? We effectively don't want to pretend
random xpcom callback/js call as a microtask entry, but limit microtask for web phasing APIs
(good thing here is that everything should be just webidl callbacks, so things will just work).
So, back out bug 1081038, and fix that by making devtools to explicitly enter/leave microtask whenever
executing scripts.

If you don't have time, I'll take this. This is very much critical.
Flags: needinfo?(gkrizsanits)
Flags: needinfo?(bobbyholley)
Flags: needinfo?(aklotz)
If you could take this that would be great, I just don't have a deep understanding of this area, so I would prefer someone else fixing this to be honest.
Flags: needinfo?(gkrizsanits)
I'm busy with other things, sorry. :-(
Flags: needinfo?(bobbyholley)
Assignee: nobody → bugs
Why is it taking so long to fix a known reproducible crash? The crash is now in 2 versions of firefox release channel and also in ESR. If fixing crashes is such a low priority then mozilla should at least revert the patch that caused this in the first place.
sorry, I'll write the patch this week, which includes reverting the patch which caused this.
(In reply to her34her34 from comment #22)
> Why is it taking so long to fix a known reproducible crash? The crash is now
> in 2 versions of firefox release channel and also in ESR. If fixing crashes
> is such a low priority then mozilla should at least revert the patch that
> caused this in the first place.

Sometimes patches take a little while to fix. Comments like this aren't really helpful to the process. Gently asking developers about it if it is taking too long is more appropriate but patches can take a month or two, easily.
Attached patch v1 (obsolete) — Splinter Review
enter microtask only in case of web/dom related script execution.
MessageManager is considered DOM related, and the worker stuff is very bogus anyway, and not considered "DOM" in this case.
(of some worker runnable might run scripts, it should explicitly use AutoEntryScript. The current setup where all the WorkerRunnables have that is wrong, at least from microtask point of view.)

This regresses bug 1081038. That is something devtools need to deal with explicitly, but I think regressing that case is totally fine, since web components aren't enabled by default. The issue is that devtools use
JS APIs to evaluate scripts, and those know nothing about microtasks.
Attachment #8608402 - Flags: review?(bobbyholley)
Hm, this patch doesn't give any indication of what value the caller should pass. When do we want to enter a microtask, exactly? Can we detect that and do it automatically? Does it depend on whether the target code is chrome or not? Or maybe on whether it's running in a DOM Window?

I'm not happy about adding an extra argument here, especially one without clear guidance of what the caller should pass. I'd probably prefer to just back out bug 1081038 rather than landing this.
Flags: needinfo?(bugs)
(In reply to Bobby Holley (:bholley) from comment #26)
> Hm, this patch doesn't give any indication of what value the caller should
> pass. When do we want to enter a microtask, exactly?
When we're doing DOM-y stuff.

> Can we detect that and
> do it automatically?
I don't know how

 Does it depend on whether the target code is chrome or
> not? Or maybe on whether it's running in a DOM Window?
well, if xpconnect stuff happens to run DOM Window code, that would be still wrong.


> 
> I'm not happy about adding an extra argument here, especially one without
> clear guidance of what the caller should pass. I'd probably prefer to just
> back out bug 1081038 rather than landing this.
I can do that too, though we'll need to do something similar to the patch in this bug eventually to have proper microtask handling.
Flags: needinfo?(bugs)
Attachment #8608402 - Flags: review?(bobbyholley)
Attachment #8608402 - Attachment is obsolete: true
Attached patch backout part 1Splinter Review
Attachment #8609736 - Flags: review?(bobbyholley)
Attached patch backout part 2Splinter Review
Attachment #8609737 - Flags: review?(bobbyholley)
(In reply to Olli Pettay [:smaug] (review overload. No new review requests before May 24, please) from comment #27)
> (In reply to Bobby Holley (:bholley) from comment #26)
> > Hm, this patch doesn't give any indication of what value the caller should
> > pass. When do we want to enter a microtask, exactly?
> When we're doing DOM-y stuff.

Can we define that better? What's actual thing we care about? That it's in a spec somewhere, and some spec editor is supposed to have thought about all of the ways that running script might cause problems? I don't have a great understanding of microtasks, but my gut feeling is that there needs to be a deeper model for this stuff.

Boris, do you have any thoughts?
Flags: needinfo?(bzbarsky)
(In reply to Bobby Holley (:bholley) from comment #30)
> Can we define that better? What's actual thing we care about? That it's in a
> spec somewhere,
It is in HTML spec.

> and some spec editor is supposed to have thought about all
> of the ways that running script might cause problems?
Of course spec authors shouldn't need to care about browser engine internal behavior.
The issue we have here is that bug 1081038 made us run microtask callbacks when there is no microtask from web point of view.
It is all Gecko specific. Sure, we may want to explicitly treat chrome scripts somehow differently.
 


> I don't have a great
> understanding of microtasks, but my gut feeling is that there needs to be a
> deeper model for this stuff.
The model is rather clear from the web point of view.  In general "end-of-microtask is right after the outermost script execution or end of a task"[1]. From the point of view scripts running on the web, bug 1081038 changed that quite a bit.

And ContentPolicy happens to be error prone, since scripts using it shouldn't touch DOM synchronously.



[1] there are some special cases, but not really relevant here.
(In reply to Olli Pettay [:smaug] from comment #25)
> This regresses bug 1081038. That is something devtools need to deal with
> explicitly

I don't think that this problem should be solved by devtools folks. And it's
not scratchpad related. This bug means that webcomponents (and in general
anything that relies on microtasks) will work differently in sandboxes
than in regular web-content. We have sandboxes all over the place
for various things, so we should have a clear story for their scripts as well,
or else this will bite us again. (We should totally back out the patches ASAP
regardless, I just don't think we're _done_ here or that we should just hack it
around)

So how are we going to solve the original problem? Is my original idea to just
simply add an nsAutoMicroTask to evalInSandbox instead of trying to do it
in AutoEntryScript is acceptable?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #32)
> (In reply to Olli Pettay [:smaug] from comment #25)
> > This regresses bug 1081038. That is something devtools need to deal with
> > explicitly
> 
> I don't think that this problem should be solved by devtools folks.
devtools uses JS APIs to run scripts, and those APIs are missing the web specific behavior of running scripts.

> So how are we going to solve the original problem? Is my original idea to
> just
> simply add an nsAutoMicroTask to evalInSandbox instead of trying to do it
> in AutoEntryScript is acceptable?
that is possibly fine, though, devtools don't use evalInSandbox, right?
(In reply to Olli Pettay [:smaug] from comment #33)
> that is possibly fine, though, devtools don't use evalInSandbox, right?

As far as I know scratchpad is a sandbox, and to run code in it evalInSandbox
is called. (a window is hooked up in the sandbox's prototype, that's how the
DOM is reachable)
ok, in that case nsAutoMicroTask around somewhere there sounds the right approach, but I'd prefer it to be enabled explicitly.
> Boris, do you have any thoughts?

This is really only an issue when script is invoked via some non-webidl-callback mechanism, since webidl callbacks automatically do microtask stuff, right?

In terms of specs, the only such mechanism I'm aware of is execution of <script> elements.  So if you're not doing that or some moral equivalent of that and you're not calling a web idl callback you should not be doing a microtask.

For the case of scratchpad (and console, and possibly debugger evaluation at a breakpoint, though that one is less clear), I would argue that each evaluation is the moral equivalent of a <script> execution and should be explicitly wrapped in the microtask stuff.
Flags: needinfo?(bzbarsky)
Comment on attachment 8609736 [details] [diff] [review]
backout part 1

Review of attachment 8609736 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking care of this smaug, sorry for the delay.
Attachment #8609736 - Flags: review?(bobbyholley) → review+
Comment on attachment 8609737 [details] [diff] [review]
backout part 2

Review of attachment 8609737 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsJSUtils.cpp
@@ +218,5 @@
>    // EvaluateString() which calls this function with
>    // aCompileOptions.noScriptRval set to true.
>    aRetValue.setUndefined();
>  
> +  nsAutoMicroTask mt;

Per the discussion in the bug, would it make more sense to hoist this into the callers that actually want it (basically just nsScriptLoader, and possible nsGlobalWindow)?

::: dom/xbl/nsXBLProtoImplField.cpp
@@ +392,5 @@
>    if (IsEmpty()) {
>      return NS_OK;
>    }
>  
> +  nsAutoMicroTask mt;

Seems like we don't actually want a microtask here, right?

::: dom/xbl/nsXBLProtoImplMethod.cpp
@@ +292,5 @@
>    if (!global) {
>      return NS_OK;
>    }
>  
> +  nsAutoMicroTask mt;

And here.
Attachment #8609737 - Flags: review?(bobbyholley) → review+
I just did the backout here. Either that, or the original approach.
(In reply to Bobby Holley (:bholley) from comment #38)
> Per the discussion in the bug, would it make more sense to hoist this into
> the callers that actually want it (basically just nsScriptLoader, and
> possible nsGlobalWindow)?
Don't understand why.


> > +  nsAutoMicroTask mt;
> 
> Seems like we don't actually want a microtask here, right?
That is where we had mt before. (and we probably do want it here)
bholley, so I don't now know whether you want changes to the patch 2 (which would mean it wasn't a backout anymore) or keep the patch as is?
Flags: needinfo?(bobbyholley)
(In reply to Olli Pettay [:smaug] from comment #40)
> (In reply to Bobby Holley (:bholley) from comment #38)
> > Per the discussion in the bug, would it make more sense to hoist this into
> > the callers that actually want it (basically just nsScriptLoader, and
> > possible nsGlobalWindow)?
> Don't understand why.

Because this whole bug is about how we only want to do MicroTask for HTML stuff, and several of the callers of nsJSUtils::EvaluateString are non-HTML.

(In reply to Olli Pettay [:smaug] from comment #41)
> bholley, so I don't now know whether you want changes to the patch 2 (which
> would mean it wasn't a backout anymore) or keep the patch as is?

Either way is fine. If this needs to be backported, pure backout + followup is probably the best way to go.
Flags: needinfo?(bobbyholley)
yeah, I was thinking pure backout + followup.
Comment on attachment 8609736 [details] [diff] [review]
backout part 1

Approval Request Comment
[Feature/regressing bug #]: bug 1081038
[User impact if declined]: Crashes
[Describe test coverage new/current, TreeHerder]: this is not very well test-coveraged
[Risks and why]: somewhat risky, changes the time when MutationObserver callbacks are called, but should mostly affect chrome code.
[String/UUID change made/needed]: NA
Attachment #8609736 - Flags: approval-mozilla-release?
Attachment #8609736 - Flags: approval-mozilla-esr38?
Attachment #8609736 - Flags: approval-mozilla-beta?
Attachment #8609736 - Flags: approval-mozilla-aurora?
Attachment #8609737 - Flags: sec-approval?
Attachment #8609737 - Flags: approval-mozilla-esr38?
Attachment #8609737 - Flags: approval-mozilla-beta?
Attachment #8609737 - Flags: approval-mozilla-aurora?
Attachment #8609736 - Flags: approval-mozilla-release? → sec-approval?
Comment on attachment 8609736 [details] [diff] [review]
backout part 1

a=dveditz for sec-approval and all the branches
Attachment #8609736 - Flags: sec-approval?
Attachment #8609736 - Flags: sec-approval+
Attachment #8609736 - Flags: approval-mozilla-esr38?
Attachment #8609736 - Flags: approval-mozilla-esr38+
Attachment #8609736 - Flags: approval-mozilla-beta?
Attachment #8609736 - Flags: approval-mozilla-beta+
Attachment #8609736 - Flags: approval-mozilla-aurora?
Attachment #8609736 - Flags: approval-mozilla-aurora+
Comment on attachment 8609737 [details] [diff] [review]
backout part 2

Please use a check-in comment about backing out the other bug (as you did in part 1) rather than mention this bug.

a=dveditz for sec-approval and all the branches
Attachment #8609737 - Flags: sec-approval?
Attachment #8609737 - Flags: sec-approval+
Attachment #8609737 - Flags: approval-mozilla-esr38?
Attachment #8609737 - Flags: approval-mozilla-esr38+
Attachment #8609737 - Flags: approval-mozilla-beta?
Attachment #8609737 - Flags: approval-mozilla-beta+
Attachment #8609737 - Flags: approval-mozilla-aurora?
Attachment #8609737 - Flags: approval-mozilla-aurora+
When these patches land are we going to re-open bug 1081038 and patch it there, or should we expect another patch on this bug for that? Who is going to do that part?
Flags: needinfo?(bugs)
I'll file a new bug...Bug 1169307
(and sheriffs, I'll land this to m-i)
Flags: needinfo?(bugs)
Comment on attachment 8609736 [details] [diff] [review]
backout part 1

FYI, applying this to esr needs some --fuzz
https://hg.mozilla.org/mozilla-central/rev/9aad1ac832f7
https://hg.mozilla.org/mozilla-central/rev/8717afeef3c6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Flags: qe-verify+
Reproduced the initial crash using both steps from comment 4 and 6 on Firefox 38 beta 3
bp-5e1f85f9-5f8a-4da8-8faa-cccbe2150602
bp-b3928d54-5f1c-4d94-892c-9bb6c2150602

Verified that using 39 beta 2 across platforms (windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) this issue does not reproduce.
Depends on: 1170484
Whiteboard: [adv-main39+][adv-esr38.1+]
Alias: CVE-2015-2731
Reproduced the crash with Firefox 38 beta 3.
Verified as fixed using Firefox 38.1.0 esr under Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.9.5.
Verified as fixed using Firefox 40 beta 1 and Aurora 41.0a2 2015-07-02 build under Win 7 x64, Ubuntu 14.04 LTS x64 and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Depends on: 1183966
No longer depends on: 1183966
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.