Closed Bug 596232 Opened 10 years ago Closed 9 years ago

nsXULTemplateBuilder/nsXULTemplateQueryProcessorXML load new data sources at unsafe time

Categories

(Core :: XUL, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -
blocking1.9.2 --- .14+
status1.9.2 --- .14-fixed
blocking1.9.1 --- .17+
status1.9.1 --- .17-fixed

People

(Reporter: smaug, Assigned: enndeakin)

References

Details

(Whiteboard: [sg:critical?] for 1.9.2 and earlier)

Attachments

(3 files)

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.
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
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Whiteboard: [sg:critical?] for 1.9.2 and earlier
blocking2.0: ? → -
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
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 ;) )
Assignee: nobody → enndeakin
Attached patch add checksSplinter Review
Attachment #479407 - Flags: review?(Olli.Pettay)
Attachment #479407 - Flags: review?(Olli.Pettay) → review+
Attachment #479407 - Flags: approval1.9.2.11?
Comment on attachment 479407 [details] [diff] [review]
add checks

1.9.2.11 is past code-freeze, moving request to next release.
Attachment #479407 - Flags: approval1.9.2.11? → approval1.9.2.12?
Comment on attachment 479407 [details] [diff] [review]
add checks

Approved for 1.9.2.12, a=dveditz for release-drivers
Attachment #479407 - Flags: approval1.9.2.12? → approval1.9.2.12+
So, this just needs to be landed and tested/confirmed.
Keywords: checkin-needed
Can we close this?
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
Attached patch trunk versionSplinter Review
Attachment #485290 - Flags: review?(Olli.Pettay)
Attachment #485290 - Flags: review?(Olli.Pettay) → review+
http://hg.mozilla.org/mozilla-central/rev/3e08f8844f87
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Olli, is there anything for QA to do to verify this fix on 1.9.2?
You could ensure that loading test_tmpl_errors.xul in a debug build doesn't 
cause any assertions.
trunk has a regression 608687 which got attached to the clone bug 553808 rather than here.
Depends on: 608687
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
blocking1.9.1: needed → .17+
blocking1.9.2: needed → .14+
Attachment #479407 - Flags: approval1.9.2.14?
Comment on attachment 479407 [details] [diff] [review]
add checks

a=LegNeato for 1.9.2.14
Attachment #479407 - Flags: approval1.9.2.14?
Attachment #479407 - Flags: approval1.9.2.14+
Attachment #479407 - Flags: approval1.9.2.13+
We'll also need the fix for 1.9.1 as well...
Attachment #505815 - Flags: approval1.9.1.17?
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
Attachment #505815 - Flags: approval1.9.1.17? → approval1.9.1.17+
Group: core-security
You need to log in before you can comment on or make changes to this bug.