596232 - nsXULTemplateBuilder/nsXULTemplateQueryProcessorXML load new data sources at unsafe time

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

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

nsEventDispatcher::Dispatch [content/events/src/nsEventDispatcher.cpp:515]
nsEventDispatcher::DispatchDOMEvent [content/events/src/nsEventDispatcher.cpp:691]
nsDOMEventTargetHelper::DispatchDOMEvent [content/events/src/nsDOMEventTargetHelper.cpp:230]
nsXMLHttpRequest::ChangeState [content/base/src/nsXMLHttpRequest.cpp:3000]
nsXMLHttpRequest::OpenRequest [content/base/src/nsXMLHttpRequest.cpp:1793]
nsXULTemplateQueryProcessorXML::GetDatasource [content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp:221]
nsXULTemplateBuilder::LoadDataSourceUrls [content/xul/templates/src/nsXULTemplateBuilder.cpp:1365]
nsXULTemplateBuilder::LoadDataSources [content/xul/templates/src/nsXULTemplateBuilder.cpp:1263]
nsXULTemplateBuilder::AttributeChanged [content/xul/templates/src/nsXULTemplateBuilder.cpp:1140]
nsXULContentBuilder::AttributeChanged [content/xul/templates/src/nsXULContentBuilder.cpp:1591]
nsNodeUtils::AttributeChanged [content/base/src/nsNodeUtils.cpp:127]
nsGenericElement::SetAttrAndNotify [content/base/src/nsGenericElement.cpp:4688]
nsGenericElement::SetAttr [content/base/src/nsGenericElement.cpp:4622]
nsGenericElement::SetAttribute [content/base/src/nsGenericElement.cpp:2400]
nsXULElement::SetAttribute [content/xul/content/src/nsXULElement.h:561]
nsIDOMElement_SetAttribute [dom_quickstubs.cpp:4918]

This bug is basically a clone of bug 553808.

Comment 1

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

Not sure if web content can access this code on trunk, but on branches
it should be able to.

The following methods may have security problems
nsXULTemplateBuilder::AttributeChanged
nsXULTemplateBuilder::ContentRemoved
nsXULTemplateBuilder::NodeWillBeDestroyed

Comment 2

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

Bug 553808 was assigned to you Neil. Might be safer to fix it here.

Sorry that I haven't fixed that bug, but the original tbox log hasif y long gone
and there were no new reports about it for awhile.

Anyway, Neil, you know this code a lot better than I do.
But just say if you don't have time. I could try to look at this too.
(But you'd have to review the patch carefully ;) )

Comment 3

[Neil Deakin]

Attachment: add checks

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 479407 [details] [diff] [review]
add checks

1.9.2.11 is past code-freeze, moving request to next release.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 479407 [details] [diff] [review]
add checks

Approved for 1.9.2.12, a=dveditz for release-drivers

Comment 6

[Damon Sicore (:damons)]

So, this just needs to be landed and tested/confirmed.

Comment 7

[Neil Deakin]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c7a429d62c81

Comment 8

[Damon Sicore (:damons)]

Can we close this?

Comment 9

[Daniel Veditz [:dveditz]]

Do we want to fix this problem on trunk as well? It may no longer be a security worry, but presumably we--or an addon--could stumble across this as a stability problem.

Does this patch work for trunk? Do we need a different patch for 1.9.1?

Marking status1.9.2 as .12-fixed per comment 7

Comment 10

[Neil Deakin]

Attachment: trunk version

Comment 11

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/3e08f8844f87

Comment 12

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

Olli, is there anything for QA to do to verify this fix on 1.9.2?

Comment 13

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

You could ensure that loading test_tmpl_errors.xul in a debug build doesn't 
cause any assertions.

Comment 14

[Daniel Veditz [:dveditz]]

trunk has a regression 608687 which got attached to the clone bug 553808 rather than here.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 479407 [details] [diff] [review]
add checks

>+      nsCOMPtr<nsIDocument> doc = mRoot ? mRoot->GetDocument() : nsnull;

Patch applies fine to 1.9.1 branch but fails to compile. The above results in

../../../../dist/include/xpcom/nsCOMPtr.h: In constructor ‘nsCOMPtr<T>::nsCOMPtr(T*) [with T = nsIDocument]’:
/Users/daniel/dev/ffcentral/moz191/content/xul/templates/src/nsXULTemplateBuilder.h:159:   instantiated from here
../../../../dist/include/xpcom/nsCOMPtr.h:554: error: invalid use of undefined type ‘struct nsIDocument’
../../../../dist/include/content/nsNodeInfoManager.h:50: error: forward declaration of ‘struct nsIDocument’
../../../../dist/include/xpcom/nsCOMPtr.h:555: error: invalid static_cast from type ‘nsIDocument*’ to type ‘nsISupports*’
../../../../dist/include/xpcom/nsCOMPtr.h: In destructor ‘nsCOMPtr<T>::~nsCOMPtr() [with T = nsIDocument]’:
/Users/daniel/dev/ffcentral/moz191/content/xul/templates/src/nsXULTemplateBuilder.h:159:   instantiated from here
../../../../dist/include/xpcom/nsCOMPtr.h:508: error: invalid static_cast from type ‘nsIDocument*’ to type ‘nsISupports*’
../../../../dist/include/xpcom/nsCOMPtr.h:510: error: invalid use of undefined type ‘struct nsIDocument’
../../../../dist/include/content/nsNodeInfoManager.h:50: error: forward declaration of ‘struct nsIDocument’
../../../../dist/include/xpcom/nsCOMPtr.h: In copy constructor ‘nsCOMPtr<T>::nsCOMPtr(const nsCOMPtr<T>&) [with T = nsIDocument]’:
/Users/daniel/dev/ffcentral/moz191/content/xul/templates/src/nsXULTemplateBuilder.h:159:   instantiated from here
../../../../dist/include/xpcom/nsCOMPtr.h:545: error: invalid use of undefined type ‘struct nsIDocument’
../../../../dist/include/content/nsNodeInfoManager.h:50: error: forward declaration of ‘struct nsIDocument’
../../../../dist/include/xpcom/nsCOMPtr.h:546: error: invalid static_cast from type ‘nsIDocument* const’ to type ‘nsISupports*’
../../../../dist/include/xpcom/nsCOMPtr.h: In member function ‘void nsCOMPtr<T>::Assert_NoQueryNeeded() [with T = nsIDocument]’:
../../../../dist/include/xpcom/nsCOMPtr.h:556:   instantiated from ‘nsCOMPtr<T>::nsCOMPtr(T*) [with T = nsIDocument]’
/Users/daniel/dev/ffcentral/moz191/content/xul/templates/src/nsXULTemplateBuilder.h:159:   instantiated from here
../../../../dist/include/xpcom/nsCOMPtr.h:520: error: no matching function for call to ‘do_QueryInterface(nsIDocument*&)’
../../../../dist/include/xpcom/nsCOMPtr.h:303: note: candidates are: nsQueryInterface do_QueryInterface(nsISupports*)
../../../../dist/include/xpcom/nsCOMPtr.h:310: note:                 nsQueryInterfaceWithError do_QueryInterface(nsISupports*, nsresult*)
make[7]: *** [nsXULTemplateQueryProcessorStorage.o] Error 1

Comment 16

[Daniel Veditz [:dveditz]]

Gavin backed this out from the 1.9.2 branch due to regression bug 608687
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d647705a2cf1 (relbranch)
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8a23f17697a7

Comment 17

[LegNeato]

Comment on attachment 479407 [details] [diff] [review]
add checks

a=LegNeato for 1.9.2.14

Comment 18

[LegNeato]

We'll also need the fix for 1.9.1 as well...

Comment 19

[Neil Deakin]

Attachment: 1.9.1 with extra include

Comment 20

[LegNeato]

Comment on attachment 505815 [details] [diff] [review]
1.9.1 with extra include

Approved for 1.9.1.17. Please land this as early as possible today so it makes the next release

Comment 21

[Neil Deakin]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6d4894752304

Comment 22

[Daniel Veditz [:dveditz]]

from Jan 18:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bc5bc0290118