crash in nsINode::OwnerDoc()

VERIFIED FIXED in Firefox 33

Status

()

--
critical
VERIFIED FIXED
5 years ago
8 days ago

People

(Reporter: jbecerra, Assigned: dmajor)

Tracking

({crash, sec-high, sec-vector})

32 Branch
mozilla34
x86
Windows NT
crash, sec-high, sec-vector
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 wontfix, firefox32+ unaffected, firefox33+ verified, firefox34+ verified, firefox-esr24 wontfix, firefox-esr31 wontfix)

Details

(Whiteboard: [fixed by bug 1051858], crash signature)

Attachments

(2 obsolete attachments)

(Reporter)

Description

5 years ago
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 *)
Group: core-security
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)
Oh, there is also TowerTiltBAApp.dll
In other words, is there any crash where some_odd_dll isn't doing the SetAttributeNode call?
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)
(Assignee)

Comment 5

5 years ago
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)
(Assignee)

Comment 6

5 years ago
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.
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)
Well, we should either do that or blacklist these DLLs, right?
tracking-firefox32: --- → ?
tracking-firefox33: --- → ?
(Assignee)

Comment 9

5 years ago
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

5 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.
Some of the crashes are rated as highly exploitable by our dump analysis in socorro.

This bug definitely needs an owner.
tracking-firefox32: ? → +
tracking-firefox33: ? → ---
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
Assignee: nobody → continuation
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)
You can QI to nsIAttribute, and cast that to Attr.
Flags: needinfo?(bugs)
Attachment #8466538 - Flags: review?(bugs) → review+
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

5 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
Attachment #8466538 - Flags: sec-approval?
Andrew, why did you clear sec-approval?
Flags: needinfo?(continuation)
The patch is wrong, per comment 17.  Not that I expect my answers to actually change for the fixed patch.
Flags: needinfo?(continuation)
Oops :)
Well, I would keep the patch there too, and cover then also SetAttributeNode.
Attachment #8466538 - Attachment is obsolete: true
Attachment #8467229 - Flags: review?(bugs)
Attachment #8467229 - Flags: review?(bugs) → review+
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 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+
status-firefox31: --- → ?
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox-esr31: --- → ?
tracking-firefox33: --- → +
tracking-firefox34: --- → +
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7e3fe987385
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox34: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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 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+
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.
status-firefox31: ? → wontfix
status-firefox-esr24: --- → wontfix
status-firefox-esr31: ? → wontfix
(Assignee)

Comment 31

5 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?
dmajor, links to crash-stats?
Depends on: 1051858
We could hard-code a check for 0x650068 though obviously that is fragile. ;)

Comment 34

5 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

5 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.
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.
nsXULElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**) is on Aurora (33) too; #3 in volume.

Comment 38

5 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

5 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)
Ryan, could you back this out for me from Aurora and Beta?  Thanks!
Flags: needinfo?(continuation) → needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-aurora/rev/89133e1972a5
https://hg.mozilla.org/releases/mozilla-beta/rev/b8d426a326f5
status-firefox32: fixed → affected
status-firefox33: fixed → affected
Flags: needinfo?(ryanvm)
(Assignee)

Comment 42

5 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

5 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)
Yeah, I think it makes sense to just back out and do the UUID stuff.  I'll do that.
Flags: needinfo?(continuation)
Duplicate of this bug: 1051863
Crash Signature: [@ nsINode::OwnerDoc()] → [@ nsINode::OwnerDoc()] [@ nsCOMPtr<nsIAttribute>::nsCOMPtr<nsIAttribute>(nsQueryInterface) | mozilla::dom::Attr::FromDOMAttr(nsIDOMAttr*)] [@ nsGenericHTMLElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**)]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8467229 - Flags: approval-mozilla-beta+
Attachment #8467229 - Flags: approval-mozilla-aurora+
Attachment #8467229 - Attachment is obsolete: true
Assignee: continuation → dmajor
status-firefox33: affected → fixed
status-firefox34: affected → fixed
Whiteboard: [fixed by bug 1051858]
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Verifying on Nightly based on no crash reports for these signatures since 2014081303.
status-firefox34: fixed → verified
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

5 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)
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)
Bug 1051858 has already landed on 33.
Thanks Andrew. Marking 32 as unaffected based on the tracking flags in bug 1051858.
status-firefox32: affected → unaffected
Flags: needinfo?(kairo)

Comment 54

5 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. ;-)
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

5 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.
Group: core-security
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

5 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)
Thanks KaiRo, that counts as a "verify" for this bug and Fx33, as far as I'm concerned. Much appreciated.
Status: RESOLVED → VERIFIED
status-firefox33: fixed → verified
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.