1038243 - crash in nsINode::OwnerDoc()

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[juan becerra [:juanb]]

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 *)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Oh, there is also TowerTiltBAApp.dll

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

In other words, is there any crash where some_odd_dll isn't doing the SetAttributeNode call?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 5

[(Away)]

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?

Comment 6

[(Away)]

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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Well, we should either do that or blacklist these DLLs, right?

Comment 9

[(Away)]

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

[(no longer active)]

(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

[David Bolter [:davidb] (NeedInfo me for attention)]

Some of the crashes are rated as highly exploitable by our dump analysis in socorro.

This bug definitely needs an owner.

Comment 12

[Daniel Veditz [:dveditz]]

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.

Comment 13

[Andrew McCreight [:mccr8]]

How is this supposed to work, Olli?  I see the cast in nsDOMAttributeMap::SetNamedItem{NS}, but you can't QI to Attr, right

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

You can QI to nsIAttribute, and cast that to Attr.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: QI to nsIAttribute in SetNamedItem.

try: https://tbpl.mozilla.org/?tree=Try&rev=2c1fd7b1cd8c

Comment 16

[Andrew McCreight [:mccr8]]

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.

Comment 17

[(Away)]

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

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Andrew, why did you clear sec-approval?

Comment 19

[Andrew McCreight [:mccr8]]

The patch is wrong, per comment 17.  Not that I expect my answers to actually change for the fixed patch.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Oops :)
Well, I would keep the patch there too, and cover then also SetAttributeNode.

Comment 21

[Andrew McCreight [:mccr8]]

Attachment: Avoid static casting from nsIDOMAttr to Attr.

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 8467229 [details] [diff] [review]
Avoid static casting from nsIDOMAttr to Attr.

[Security approval request comment]
See comment 16.

Comment 23

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=c2d7bf52d31d

Comment 24

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 25

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e3fe987385

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f7e3fe987385

Comment 27

[Andrew McCreight [:mccr8]]

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

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8467229 [details] [diff] [review]
Avoid static casting from nsIDOMAttr to Attr.

Approved for Aurora and Beta.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/429ea7cf7758
https://hg.mozilla.org/releases/mozilla-beta/rev/551f71d3138f

Comment 30

[Andrew McCreight [:mccr8]]

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.

Comment 31

[(Away)]

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

[Olli Pettay [:smaug][[e-mail]]]

dmajor, links to crash-stats?

Comment 33

[Andrew McCreight [:mccr8]]

We could hard-code a check for 0x650068 though obviously that is fragile. ;)

Comment 34

[Robert Kaiser]

(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

[Robert Kaiser]

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

[Andrew McCreight [:mccr8]]

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

[Tracy Walker [:tracy]]

nsXULElement::SetAttributeNode(nsIDOMAttr*, nsIDOMAttr**) is on Aurora (33) too; #3 in volume.

Comment 38

[Robert Kaiser]

(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

[Robert Kaiser]

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)?

Comment 40

[Andrew McCreight [:mccr8]]

Ryan, could you back this out for me from Aurora and Beta?  Thanks!

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89133e1972a5
https://hg.mozilla.org/releases/mozilla-beta/rev/b8d426a326f5

Comment 42

[(Away)]

(In reply to Robert Kaiser (:[e-mail], 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

[Robert Kaiser]

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?

Comment 44

[Andrew McCreight [:mccr8]]

Yeah, I think it makes sense to just back out and do the UUID stuff.  I'll do that.

Comment 45

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/821f18e5c94c

Comment 47

[Ed Morley [:emorley]]

(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

Comment 48

[Liz Henry (:lizzard) (relman/hg->git project)]

Verifying on Nightly based on no crash reports for these signatures since 2014081303.

Comment 49

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 50

[Robert Kaiser]

(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.

Comment 51

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 52

[Andrew McCreight [:mccr8]]

Bug 1051858 has already landed on 33.

Comment 53

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks Andrew. Marking 32 as unaffected based on the tracking flags in bug 1051858.

Comment 54

[Robert Kaiser]

(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

[Liz Henry (:lizzard) (relman/hg->git project)]

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

[Robert Kaiser]

(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.

Comment 57

[Matt Wobensmith [:mwobensmith][:matt:]]

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.

Comment 58

[Robert Kaiser]

(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.

Comment 59

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks KaiRo, that counts as a "verify" for this bug and Fx33, as far as I'm concerned. Much appreciated.