Closed
Bug 1038243
Opened 10 years ago
Closed 10 years ago
crash in nsINode::OwnerDoc()
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
VERIFIED
FIXED
mozilla34
People
(Reporter: jbecerra, Assigned: away)
References
Details
(Keywords: crash, sec-high, sec-vector, Whiteboard: [fixed by bug 1051858])
Crash Data
Attachments
(2 obsolete files)
This bug was filed from the Socorro interface and is report bp-04f75275-0b26-4439-8893-8dcdc2140630. ============================================================= This signature has been around Fx32 for a while but it is moving up in rank to the top 10. Currently at #11 and going up. I am not sure where to file it. Most of these are happening on Windows 7 installations. There are no comments. I noticed a few of the reports had some dll entries which look suspicious, but the correlation reports didn't list anything obvious. 0 xul.dll nsINode::OwnerDoc() obj-firefox/dist/include/nsINode.h 1 xul.dll nsDOMAttributeMap::SetNamedItemInternal(mozilla::dom::Attr &,bool,mozilla::ErrorResult &) content/base/src/nsDOMAttributeMap.cpp 2 xul.dll nsDOMAttributeMap::SetNamedItem(mozilla::dom::Attr &,mozilla::ErrorResult &) content/base/src/nsDOMAttributeMap.h 3 xul.dll mozilla::dom::Element::SetAttributeNode(mozilla::dom::Attr &,mozilla::ErrorResult &) content/base/src/Element.cpp 4 xul.dll nsGenericHTMLElement::SetAttributeNode(nsIDOMAttr *,nsIDOMAttr * *) content/html/content/src/nsGenericHTMLElement.h 5 TowerTiltBAApp.dll TowerTiltBAApp.dll@0xb3d6 6 TowerTiltBAApp.dll TowerTiltBAApp.dll@0xb1d7 7 TowerTiltBAApp.dll TowerTiltBAApp.dll@0xc80e 8 xul.dll nsObserverList::NotifyObservers(nsISupports *,char const *,wchar_t const *) xpcom/ds/nsObserverList.cpp 9 xul.dll xul.dll@0x14fa31c 10 @0x700074 11 mozglue.dll arena_dalloc memory/mozjemalloc/jemalloc.c 12 mozglue.dll je_free memory/mozjemalloc/jemalloc.c 13 xul.dll nsRunnableMethodImpl<void ( mozilla::dom::XULDocument::*)(void),void,1>::`scalar deleting destructor'(unsigned int) 14 xul.dll nsRunnableMethodImpl<void ( mozilla::dom::BlobParent::*)(void),void,0>::Run() obj-firefox/dist/include/nsThreadUtils.h 15 xul.dll nsDocumentViewer::InitInternal(nsIWidget *,nsISupports *,nsIntRect const &,bool,bool,bool) layout/base/nsDocumentViewer.cpp 16 xul.dll nsDocumentViewer::Init(nsIWidget *,nsIntRect const &) layout/base/nsDocumentViewer.cpp 17 xul.dll nsDocShell::SetupNewViewer(nsIContentViewer *) docshell/base/nsDocShell.cpp 18 xul.dll nsDocShell::Embed(nsIContentViewer *,char const *,nsISupports *) docshell/base/nsDocShell.cpp 19 xul.dll nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal *,nsIURI *,bool) docshell/base/nsDocShell.cpp 20 xul.dll nsDocShell::EnsureContentViewer() docshell/base/nsDocShell.cpp 21 xul.dll nsDocShell::GetDocument() docshell/base/nsDocShell.cpp 22 xul.dll nsPIDOMWindow::MaybeCreateDoc() dom/base/nsGlobalWindow.cpp 23 xul.dll nsPIDOMWindow::GetDoc() dom/base/nsPIDOMWindow.h 24 xul.dll XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>,void const *,nsXPTType const &,nsID const *,tag_nsresult *) js/xpconnect/src/XPCConvert.cpp 25 xul.dll XPC_WN_GetterSetter(JSContext *,unsigned int,JS::Value *) js/xpconnect/src/XPCWrappedNativeJSOps.cpp 26 mozjs.dll js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) js/src/vm/Interpreter.cpp 27 mozjs.dll js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp 28 mozjs.dll JSObject::getGeneric(JSContext *,JS::Handle<JSObject *>,JS::Handle<JSObject *>,JS::Handle<jsid>,JS::MutableHandle<JS::Value>) js/src/jsobj.h 29 mozjs.dll js::jit::DoGetPropFallback js/src/jit/BaselineIC.cpp 30 @0x16115841 31 @0x161108f4 32 mozjs.dll js::jit::EnterBaselineMethod(JSContext *,js::RunState &) js/src/jit/BaselineJIT.cpp 33 mozjs.dll js::RunScript(JSContext *,js::RunState &) js/src/vm/Interpreter.cpp 34 mozjs.dll js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) js/src/vm/Interpreter.cpp 35 mozjs.dll js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp 36 mozjs.dll JS_CallFunctionValue(JSContext *,JS::Handle<JSObject *>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) js/src/jsapi.cpp 37 xul.dll nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *,unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) js/xpconnect/src/XPCWrappedJSClass.cpp 38 xul.dll nsXPCWrappedJS::CallMethod(unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) js/xpconnect/src/XPCWrappedJS.cpp 39 xul.dll nsDocShell::OnStateChange(nsIWebProgress *,nsIRequest *,unsigned int,tag_nsresult) docshell/base/nsDocShell.cpp 40 xul.dll SharedStub xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp 41 xul.dll nsDocLoader::DoFireOnStateChange(nsIWebProgress * const,nsIRequest * const,int &,tag_nsresult) uriloader/base/nsDocLoader.cpp 42 xul.dll nsDocLoader::FireOnStateChange(nsIWebProgress *,nsIRequest *,int,tag_nsresult) uriloader/base/nsDocLoader.cpp 43 xul.dll nsDocLoader::doStartDocumentLoad() uriloader/base/nsDocLoader.cpp 44 xul.dll nsDocLoader::OnStartRequest(nsIRequest *,nsISupports *) uriloader/base/nsDocLoader.cpp 45 xul.dll nsLoadGroup::AddRequest(nsIRequest *,nsISupports *) netwerk/base/src/nsLoadGroup.cpp 46 xul.dll mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener *,nsISupports *)
Updated•10 years ago
|
Group: core-security
Comment 1•10 years ago
|
||
Do all the crashes have FortunitasBAApp.dll? Since based on the stack trace this looks like that library is passing some bogus value to SetAttributeNode.
Flags: needinfo?(jbecerra)
Comment 2•10 years ago
|
||
Oh, there is also TowerTiltBAApp.dll
Comment 3•10 years ago
|
||
In other words, is there any crash where some_odd_dll isn't doing the SetAttributeNode call?
Comment 4•10 years ago
|
||
But I claim this is a regression from bug 851470. We just do a scary cast from nsIDOMAttr to mozilla::dom::Attr. Any reason to not QI to nsIAttribute and only then do the casting? That would possibly fix the crash (but those addons wouldn't still work).
Blocks: 851470
Flags: needinfo?(Ms2ger)
Yes, all of the crashes on 32 and 33 have suspicious DLLs on the stack. These DLLs have dozens of names ending with ...BAApp.dll and have timestamps of either Apr 30 or Jul 08. They also have a companion module in the same directory with a UUID filename. No version numbers. So, pretty obviously malware. Nonetheless, there is a clear beginning to these crashes with nightly build 20140530030202. Changes: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b85b57f05fda&tochange=e6f113c83095 Ms2ger: perhaps bug 741295?
Flags: needinfo?(jbecerra)
Though from the memory dumps, comment 4 is right: the thing they are passing is definitely not static_cast-compatible with mozilla::dom::Attr. Perhaps that's the root cause and something on 5/30 merely provoked it.
Comment 7•10 years ago
|
||
We assume we can cast builtinclass interfaces to implementation classes all over the place, so I'm not sure how useful it is to add a QI here.
Flags: needinfo?(Ms2ger)
Comment 8•10 years ago
|
||
Well, we should either do that or blacklist these DLLs, right?
tracking-firefox32:
--- → ?
tracking-firefox33:
--- → ?
This app is pretty clearly trying to avoid getting filtered. If we block it, I suspect the authors will just work around the blocks and potentially make things worse (like letting in other stuff we've previously blocked). We've had that happen twice in the past few weeks. Our blocklist can't be a full antimalware engine.
Comment 10•10 years ago
|
||
(In reply to :Ms2ger from comment #7) > We assume we can cast builtinclass interfaces to implementation classes all > over the place, so I'm not sure how useful it is to add a QI here. That assumption is valid only for JS implemented objects, clearly not for native code calling into us like this. I think we should do the QI.
Comment 11•10 years ago
|
||
Some of the crashes are rated as highly exploitable by our dump analysis in socorro. This bug definitely needs an owner.
Updated•10 years ago
|
tracking-firefox33:
? → ---
Comment 12•10 years ago
|
||
If we do the QI _here_ do we have to look for all other possible places we do this cast and do the same? That could be quite an auditing job.
Keywords: sec-high,
sec-vector
Updated•10 years ago
|
Assignee: nobody → continuation
Comment 13•10 years ago
|
||
How is this supposed to work, Olli? I see the cast in nsDOMAttributeMap::SetNamedItem{NS}, but you can't QI to Attr, right
Flags: needinfo?(bugs)
Comment 15•10 years ago
|
||
try: https://tbpl.mozilla.org/?tree=Try&rev=2c1fd7b1cd8c
Attachment #8466538 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8466538 -
Flags: review?(bugs) → review+
Comment 16•10 years ago
|
||
Comment on attachment 8466538 [details] [diff] [review] QI to nsIAttribute in SetNamedItem. [Security approval request comment] How easily could an exploit be constructed based on the patch? This seems like some kind of malware doing something bad, so probably not easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? It is only really a problem with Firefox interacting with a certain binary component, so no. Which older supported branches are affected by this flaw? Probably everything? It is hard to know. If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be easy to backport, the patch is pretty trivial. How likely is this patch to cause regressions; how much testing does it need? There's a low chance of regressions, but it would be nice to know if it fixes these crashes.
Attachment #8466538 -
Flags: sec-approval?
Assignee | ||
Comment 17•10 years ago
|
||
This covers foo.attributes.setNamedItem, right? The crash reports are for foo.SetAttributeNode, which does the cast here: http://dxr.mozilla.org/mozilla-central/source/content/base/public/Element.h#1480
Updated•10 years ago
|
Attachment #8466538 -
Flags: sec-approval?
Comment 19•10 years ago
|
||
The patch is wrong, per comment 17. Not that I expect my answers to actually change for the fixed patch.
Flags: needinfo?(continuation)
Comment 20•10 years ago
|
||
Oops :) Well, I would keep the patch there too, and cover then also SetAttributeNode.
Comment 21•10 years ago
|
||
Attachment #8466538 -
Attachment is obsolete: true
Attachment #8467229 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8467229 -
Flags: review?(bugs) → review+
Comment 22•10 years ago
|
||
Comment on attachment 8467229 [details] [diff] [review] Avoid static casting from nsIDOMAttr to Attr. [Security approval request comment] See comment 16.
Attachment #8467229 -
Flags: sec-approval?
Comment 23•10 years ago
|
||
try run: https://tbpl.mozilla.org/?tree=Try&rev=c2d7bf52d31d
Comment 24•10 years ago
|
||
Comment on attachment 8467229 [details] [diff] [review] Avoid static casting from nsIDOMAttr to Attr. We should take this on Beta and Aurora and, assuming it applies, ESR31, at the very least.
Attachment #8467229 -
Flags: sec-approval? → sec-approval+
Updated•10 years ago
|
status-firefox31:
--- → ?
status-firefox33:
--- → affected
status-firefox34:
--- → affected
status-firefox-esr31:
--- → ?
tracking-firefox33:
--- → +
tracking-firefox34:
--- → +
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7e3fe987385
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment 27•10 years ago
|
||
Comment on attachment 8467229 [details] [diff] [review] Avoid static casting from nsIDOMAttr to Attr. Approval Request Comment [Feature/regressing bug #]: maybe bug 851470, maybe just some weird new malware problem [User impact if declined]: scary crashes [Describe test coverage new/current, TBPL]: this code is tested on TBPL [Risks and why]: low, it just makes some casts more dynamic [String/UUID change made/needed]: none
Attachment #8467229 -
Flags: approval-mozilla-beta?
Attachment #8467229 -
Flags: approval-mozilla-aurora?
Comment 28•10 years ago
|
||
Comment on attachment 8467229 [details] [diff] [review] Avoid static casting from nsIDOMAttr to Attr. Approved for Aurora and Beta.
Attachment #8467229 -
Flags: approval-mozilla-beta?
Attachment #8467229 -
Flags: approval-mozilla-beta+
Attachment #8467229 -
Flags: approval-mozilla-aurora?
Attachment #8467229 -
Flags: approval-mozilla-aurora+
Comment 29•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/429ea7cf7758 https://hg.mozilla.org/releases/mozilla-beta/rev/551f71d3138f
Comment 30•10 years ago
|
||
For now, I'm inclined to not land this on ESR24 or ESR31, as we have no evidence this is happening in any great quantity on any branch other than 32. Since it is malware, you'd think it would show up on release-31 if it was a problem there.
Updated•10 years ago
|
Assignee | ||
Comment 31•10 years ago
|
||
Now we're crashing slightly earlier: beta: nsGenericHTMLElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**) aurora: nsXULElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**) nightly: nsCOMPtr<nsIAttribute>::nsCOMPtr<nsIAttribute>(nsQueryInterface) | mozilla::dom::Attr::FromDOMAttr(nsIDOMAttr*) The inlining varies but it's always a crash executing address 0x650068. It seems the parameter coming from the malware is not even nsISupports-compatible. Instead of a pointer to QueryInterface they have this bogus address that looks more like text. I assume we still want to keep the fix though?
Comment 32•10 years ago
|
||
dmajor, links to crash-stats?
Comment 33•10 years ago
|
||
We could hard-code a check for 0x650068 though obviously that is fragile. ;)
Comment 34•10 years ago
|
||
(In reply to David Major [:dmajor] from comment #31) > beta: nsGenericHTMLElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**) That one seems to be far worse than what the bug here was in terms of volume. :(
Comment 35•10 years ago
|
||
Hmm, looks like we have some other 3rd-party software crash on us now as well with the same address, all since 32.0b5 shipped (all those might have that other adware installed, though): https://crash-stats.mozilla.com/report/list?signature=datamngr.dll%400x200068#tab-reports https://crash-stats.mozilla.com/report/list?signature=smdmf.dll%400x200068#tab-reports https://crash-stats.mozilla.com/report/list?signature=bavum.dll%400xa0068#tab-reports Or at least near to this address, e.g. this has an offset: https://crash-stats.mozilla.com/report/list?signature=smdmf.dll%400x20006a#tab-reports Something looks really funky with this.
Comment 36•10 years ago
|
||
Are these still beta-only? That does lend some credence to bsmedberg's theory that maybe some UUID needed to get bumped, because it could have incidentally been bumped in Aurora onwards.
Comment 37•10 years ago
|
||
nsXULElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**) is on Aurora (33) too; #3 in volume.
Comment 38•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #36) > Are these still beta-only? I think they are 32+, so beta, aurora and nightly. The 3rd-party apps I mentioned in comment #35 probably do not actually run with anything more unstable than beta by design, but Tracy already mentioned the Aurora case, and https://crash-stats.mozilla.com/report/list?signature=nsCOMPtr%3CnsIAttribute%3E%3A%3AnsCOMPtr%3CnsIAttribute%3E%28nsQueryInterface%29%20%7C%20mozilla%3A%3Adom%3A%3AAttr%3A%3AFromDOMAttr%28nsIDOMAttr%2A%29 is around on Nightly with the same address any symptoms.
Comment 39•10 years ago
|
||
Andrew, we discussed this crash and its followups in today's stability meeting with bsmedberg. Given that the volume increased a ton and the security impact is questionable, can we please back this out at least on beta (and probably aurora)?
Flags: needinfo?(continuation)
Comment 40•10 years ago
|
||
Ryan, could you back this out for me from Aurora and Beta? Thanks!
Flags: needinfo?(continuation) → needinfo?(ryanvm)
Comment 41•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/89133e1972a5 https://hg.mozilla.org/releases/mozilla-beta/rev/b8d426a326f5
Assignee | ||
Comment 42•10 years ago
|
||
(In reply to Robert Kaiser (:kairo@mozilla.com, slow reaction due to vacation backlog) from comment #35) > Hmm, looks like we have some other 3rd-party software crash on us now as > well with the same address, all since 32.0b5 shipped (all those might have > that other adware installed, though): Those other binaries are unrelated. They just happened to be loaded near the 0x650068 address. It's still the *BAApp adware on the stack in those reports. I'll try revving some IIDs over in bug 1051858 to see whether that prevents these calls.
Comment 43•10 years ago
|
||
Andrew (and Ryan), thanks for getting the backouts there. Another question, though: Given that we now seem to think that the IID revs in bug 1051858 will be the correct fix for the crashes on trunk, should the patch of this bug stay in there (i.e. is its behavior better than before it) or should it be backed out there as well?
Flags: needinfo?(continuation)
Comment 44•10 years ago
|
||
Yeah, I think it makes sense to just back out and do the UUID stuff. I'll do that.
Flags: needinfo?(continuation)
Updated•10 years ago
|
Crash Signature: [@ nsINode::OwnerDoc()] → [@ nsINode::OwnerDoc()]
[@ nsCOMPtr<nsIAttribute>::nsCOMPtr<nsIAttribute>(nsQueryInterface) | mozilla::dom::Attr::FromDOMAttr(nsIDOMAttr*)]
[@ nsGenericHTMLElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**)]
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•10 years ago
|
Attachment #8467229 -
Flags: approval-mozilla-beta+
Attachment #8467229 -
Flags: approval-mozilla-aurora+
Comment 47•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #45) > https://hg.mozilla.org/integration/mozilla-inbound/rev/821f18e5c94c Merge of backout: https://hg.mozilla.org/mozilla-central/rev/821f18e5c94c
Updated•10 years ago
|
Attachment #8467229 -
Attachment is obsolete: true
Updated•10 years ago
|
Assignee: continuation → dmajor
Whiteboard: [fixed by bug 1051858]
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 48•10 years ago
|
||
Verifying on Nightly based on no crash reports for these signatures since 2014081303.
Comment 49•10 years ago
|
||
No crashes in Aurora since the 2014081415 build, and no crashes for the 32.0b8 build. I'm not sure that's enough to mark this verified. KaiRo what do you think?
Flags: needinfo?(kairo)
Comment 50•10 years ago
|
||
(In reply to Liz Henry :lizzard from comment #49) > No crashes in Aurora since the 2014081415 build, and no crashes for the > 32.0b8 build. I'm not sure that's enough to mark this verified. KaiRo what > do you think? b8 has no users yet, its numbers don't really count. b6 and b7 should, though. Also, did you check for all signatures connected here? I'm still a bit surprised that the original one doesn't really come back after the backout.
Flags: needinfo?(kairo)
Comment 51•10 years ago
|
||
Kairo - I want to determine how to mark the status of 32 and 33 and whether an uplift is required for 33. Have you seen any resurgence of this crash on beta or aurora?
Flags: needinfo?(kairo)
Comment 52•10 years ago
|
||
Bug 1051858 has already landed on 33.
Comment 53•10 years ago
|
||
Thanks Andrew. Marking 32 as unaffected based on the tracking flags in bug 1051858.
Flags: needinfo?(kairo)
Comment 54•10 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] from comment #51) > Kairo - I want to determine how to mark the status of 32 and 33 and whether > an uplift is required for 33. Have you seen any resurgence of this crash on > beta or aurora? FWIW, no resurgence of this one even on beta, which surprised us all a little but isn't bad overall. ;-)
Comment 55•10 years ago
|
||
KaiRo: there are 12 crashes on 32.0b9 for nsINode::OwnerDoc() and they look like the other BAApp crashes. So they haven't completely disappeared. The other 2 signatures aren't showing up on Beta 7, 8, or 9.
Comment 56•10 years ago
|
||
(In reply to Liz Henry :lizzard from comment #55) > KaiRo: there are 12 crashes on 32.0b9 for nsINode::OwnerDoc() and they look > like the other BAApp crashes. So they haven't completely disappeared. OK, didn't search that deeply, thanks for looking. So, not that much surprise but maybe they fixed something as well. Good.
Updated•10 years ago
|
Group: core-security
Comment 57•10 years ago
|
||
KaiRo, Liz: What's our confidence that this has been fixed on Fx33? I noticed that both of you have looked at this previously, and we ought to know now if this has been addressed.
Flags: needinfo?(lhenry)
Flags: needinfo?(kairo)
Comment 58•10 years ago
|
||
(In reply to Matt Wobensmith from comment #57) > KaiRo, Liz: What's our confidence that this has been fixed on Fx33? I > noticed that both of you have looked at this previously, and we ought to > know now if this has been addressed. There are crashes with the nsINode::OwnerDoc() signature still around but not with BAApp, so this specific thing is fixed. The occurrences of the signature I have checked have a collection of different stack traces and overall account for very low volume. I can't say if any of the remaining ones have any security relevance, but if so, they need to be filed in a different bug anyhow.
Flags: needinfo?(lhenry)
Flags: needinfo?(kairo)
Comment 59•10 years ago
|
||
Thanks KaiRo, that counts as a "verify" for this bug and Fx33, as far as I'm concerned. Much appreciated.
Status: RESOLVED → VERIFIED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•