Closed
Bug 1298509
Opened 8 years ago
Closed 5 years ago
nsReferencedElement is not notified for elements that are removed through document.open()
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla71
People
(Reporter: mstange, Assigned: edgar)
References
Details
(Keywords: sec-other, Whiteboard: [adv-main71+r])
Attachments
(3 files)
See the testcase. It should print PASS, not FAIL.
I'm not sure if this is causing security bugs, but I'm filing this as a security bug because the testcase shows how to trigger bug 1287316.
Canvas filters observe the referenced SVG filter elements through CanvasFilterChainObserver, which uses nsReferencedElement to observe the referenced ID. If the document is destroyed through document.open(), we call mIdentifierMap.Clear() with this callstack:
#4 0x00007efe0b0cf077 in nsDocument::DestroyElementMaps() (this=0x7efdf47d8000)
at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:9512
#5 0x00007efe0b0b0ca1 in nsDocument::ResetToURI(nsIURI*, nsILoadGroup*, nsIPrincipal*) (this=0x7efdf47d8000, aURI=0x7efde5358900, aLoadGroup=0x7efdfcb84450, aPrincipal=0x7efde3fddd70)
at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:2175
#6 0x00007efe0c4fb71d in nsHTMLDocument::ResetToURI(nsIURI*, nsILoadGroup*, nsIPrincipal*) (this=0x7efdf47d8000, aURI=0x7efde5358900, aLoadGroup=0x7efdfcb84450, aPrincipal=0x7efde3fddd70)
at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:247
#7 0x00007efe0b0b08c2 in nsDocument::Reset(nsIChannel*, nsILoadGroup*) (this=0x7efdf47d8000, aChannel=0x7efdf47b2888, aLoadGroup=0x7efdfcb84450) at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:2123
#8 0x00007efe0c4fb6a1 in nsHTMLDocument::Reset(nsIChannel*, nsILoadGroup*) (this=0x7efdf47d8000, aChannel=0x7efdf47b2888, aLoadGroup=0x7efdfcb84450) at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:234
#9 0x00007efe0c501846 in nsHTMLDocument::Open(JSContext*, nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) (this=0x7efdf47d8000, cx=0x7efdfb7e5000, aType=..., aReplace=..., rv=...)
at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1665
#10 0x00007efe0c500106 in nsHTMLDocument::Open(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, JSContext*, unsigned char, nsISupports**) (this=0x7efdf47d8000, aContentTypeOrUrl=..., aReplaceOrName=..., aFeatures=..., cx=0x7efdfb7e5000, aOptionalArgCount=1 '\001', aReturn=0x7ffd29081d10)
at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1368
#11 0x00007efe0c5025c6 in nsHTMLDocument::WriteCommon(JSContext*, nsAString_internal const&, bool) (this=0x7efdf47d8000, cx=0x7efdfb7e5000, aText=..., aNewlineTerminate=false)
at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1880
#12 0x00007efe0c5020f6 in nsHTMLDocument::WriteCommon(JSContext*, mozilla::dom::Sequence<nsString> const&, bool, mozilla::ErrorResult&) (this=0x7efdf47d8000, cx=0x7efdfb7e5000, aText=..., aNewlineTerminate=false, rv=...)
at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1814
#13 0x00007efe0c5029b8 in nsHTMLDocument::Write(JSContext*, mozilla::dom::Sequence<nsString> const&, mozilla::ErrorResult&) (this=0x7efdf47d8000, cx=0x7efdfb7e5000, aText=..., rv=...)
at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1936
#14 0x00007efe0bf73c82 in mozilla::dom::HTMLDocumentBinding::write(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) (cx=0x7efdfb7e5000, obj=(JSObject * const) 0x7efdf2bf8100 [object Proxy], self=0x7efdf47d8000
So later, when the SVG filter element is unbound from the document tree, we get to nsDocument::RemoveFromIdTable but mIdentifierMap is already empty so we don't notify the nsReferencedElement. For reference, here's the callstack when the SVG filter element is removed:
#0 0x00007efe0b0b51e0 in nsDocument::RemoveFromIdTable(mozilla::dom::Element*, nsIAtom*) (this=0x7efdf47d8000, aElement=0x7efde3ff9c70, aId=0x7efdfa902640) at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:3033
#1 0x00007efe0af9a0ef in mozilla::dom::Element::RemoveFromIdTable() (this=0x7efde3ff9c70)
at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1042
#2 0x00007efe0af9d2f2 in mozilla::dom::Element::UnbindFromTree(bool, bool) (this=0x7efde3ff9c70, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1780
#3 0x00007efe0af9d8fa in mozilla::dom::Element::UnbindFromTree(bool, bool) (this=0x7efde3ff9b80, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1920
#4 0x00007efe0af9d8fa in mozilla::dom::Element::UnbindFromTree(bool, bool) (this=0x7efdfcbf2060, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1920
#5 0x00007efe0cab9c21 in mozilla::dom::SVGSVGElement::UnbindFromTree(bool, bool) (this=0x7efdfcbf2060, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/svg/SVGSVGElement.cpp:777
#6 0x00007efe0af9d8fa in mozilla::dom::Element::UnbindFromTree(bool, bool) (this=0x7efde448bce0, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1920
#7 0x00007efe0c4eb12b in nsGenericHTMLElement::UnbindFromTree(bool, bool) (this=0x7efde448bce0, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/html/nsGenericHTMLElement.cpp:516
#8 0x00007efe0c3ffc55 in mozilla::dom::HTMLBodyElement::UnbindFromTree(bool, bool) (this=0x7efde448bce0, aDeep=true, aNullParent=false) at /home/mstange/code/mozilla-central/dom/html/HTMLBodyElement.cpp:358
#9 0x00007efe0af9d8fa in mozilla::dom::Element::UnbindFromTree(bool, bool) (this=0x7efde3fdb690, aDeep=true, aNullParent=true) at /home/mstange/code/mozilla-central/dom/base/Element.cpp:1920
#10 0x00007efe0c4eb12b in nsGenericHTMLElement::UnbindFromTree(bool, bool) (this=0x7efde3fdb690, aDeep=true, aNullParent=true) at /home/mstange/code/mozilla-central/dom/html/nsGenericHTMLElement.cpp:516
#11 0x00007efe0c4a6ec5 in mozilla::dom::HTMLSharedElement::UnbindFromTree(bool, bool) (this=0x7efde3fdb690, aDeep=true, aNullParent=true) at /home/mstange/code/mozilla-central/dom/html/HTMLSharedElement.cpp:312
#12 0x00007efe0b0b0e3e in nsDocument::ResetToURI(nsIURI*, nsILoadGroup*, nsIPrincipal*) (this=0x7efdf47d8000, aURI=0x7efde5358900, aLoadGroup=0x7efdfcb84450, aPrincipal=0x7efde3fddd70)
at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:2192
#13 0x00007efe0c4fb71d in nsHTMLDocument::ResetToURI(nsIURI*, nsILoadGroup*, nsIPrincipal*) (this=0x7efdf47d8000, aURI=0x7efde5358900, aLoadGroup=0x7efdfcb84450, aPrincipal=0x7efde3fddd70)
at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:247
#14 0x00007efe0b0b08c2 in nsDocument::Reset(nsIChannel*, nsILoadGroup*) (this=0x7efdf47d8000, aChannel=0x7efdf47b2888, aLoadGroup=0x7efdfcb84450) at /home/mstange/code/mozilla-central/dom/base/nsDocument.cpp:2123
#15 0x00007efe0c4fb6a1 in nsHTMLDocument::Reset(nsIChannel*, nsILoadGroup*) (this=0x7efdf47d8000, aChannel=0x7efdf47b2888, aLoadGroup=0x7efdfcb84450) at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:234
#16 0x00007efe0c501846 in nsHTMLDocument::Open(JSContext*, nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) (this=0x7efdf47d8000, cx=0x7efdfb7e5000, aType=..., aReplace=..., rv=...)
at /home/mstange/code/mozilla-central/dom/html/nsHTMLDocument.cpp:1665
#17 0x00007efe0c500106 in nsHTMLDocument::Open(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, JSContext*, unsigned char, nsISupports**) (this=0x7efdf47d8000, aContentTypeOrUrl=..., aReplaceOrName=..., aFea
The call to nsDocument::DestroyElementMaps() at http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/dom/base/nsDocument.cpp#2201 has this comment:
// Destroy link map now so we don't waste time removing
// links one by one
It was added in bug 304759.
I'm not sure what to do about this. Boris, do you have an opinion?
Flags: needinfo?
Reporter | ||
Updated•8 years ago
|
Flags: needinfo? → needinfo?(bzbarsky)
Comment 1•8 years ago
|
||
Bug 304759 was just about the link map.
The change to clear all the maps was from bug 564863. I think the right answer is probably to either go back to just clearing the link map in ResetToURI or to clear only the non-id parts of mIdentifierMap, or to go through mIdentifierMap before clearing it and notify all the referenced elements that they now reference nothing.
This last is probably simplest. In fact, we should perhaps do it in DestroyElementMaps, so it's always done, for safety.
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 2•8 years ago
|
||
The last one sounds great, thank you.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•8 years ago
|
||
Attachment #8786084 -
Flags: review?(bzbarsky)
Comment 4•8 years ago
|
||
Comment on attachment 8786084 [details] [diff] [review]
patch
r=me
Attachment #8786084 -
Flags: review?(bzbarsky) → review+
Updated•8 years ago
|
Group: core-security → dom-core-security
Whiteboard: [keep hidden while bug 1287316 is hidden]
Updated•7 years ago
|
Priority: -- → P2
Comment 5•6 years ago
|
||
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3
Comment 6•6 years ago
|
||
Unhiding, as bug 1287316 is unhidden. Markus, can the patch here land?
Group: dom-core-security
Flags: needinfo?(mstange)
Updated•6 years ago
|
Whiteboard: [keep hidden while bug 1287316 is hidden]
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Comment 7•5 years ago
|
||
Hsin-yi, this might need a different owner to finish driving this in. I just checked, and this is NOT fixed by bug 1489308, because while we no longer ResetToURI in document.open() we are still doing Document::DisconnectNodeTree which is still calling DestroyElementMaps().
Flags: needinfo?(htsai)
Comment 8•5 years ago
|
||
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #7)
Hsin-yi, this might need a different owner to finish driving this in. I just checked, and this is NOT fixed by bug 1489308, because while we no longer ResetToURI in document.open() we are still doing Document::DisconnectNodeTree which is still calling DestroyElementMaps().
Thank you for bringing it up. I'll take a look and work on a plan for it.
Flags: needinfo?(htsai)
Assignee | ||
Comment 9•5 years ago
|
||
Assignee | ||
Comment 10•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee: mstange → echen
Assignee | ||
Comment 11•5 years ago
|
||
Comment 12•5 years ago
|
||
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/908c9c793e92
Notify identifier map entries if they get removed via DestroyElementMaps; r=bzbarsky
Comment 13•5 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Updated•5 years ago
|
Whiteboard: [adv-main71+r]
Updated•5 years ago
|
Reporter | ||
Updated•2 years ago
|
Flags: needinfo?(mstange.moz)
You need to log in
before you can comment on or make changes to this bug.
Description
•