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)

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox51 --- wontfix
firefox71 --- fixed

People

(Reporter: mstange, Assigned: edgar)

References

Details

(Keywords: sec-other, Whiteboard: [adv-main71+r])

Attachments

(3 files)

Attached file testcase
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?
Flags: needinfo? → needinfo?(bzbarsky)
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)
The last one sounds great, thank you.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Keywords: sec-other
Attached patch patchSplinter Review
Attachment #8786084 - Flags: review?(bzbarsky)
Attachment #8786084 - Flags: review?(bzbarsky) → review+
Group: core-security → dom-core-security
Whiteboard: [keep hidden while bug 1287316 is hidden]
Priority: -- → P2
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
Unhiding, as bug 1287316 is unhidden. Markus, can the patch here land?
Group: dom-core-security
Flags: needinfo?(mstange)
Whiteboard: [keep hidden while bug 1287316 is hidden]
Component: DOM → DOM: Core & HTML

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)

(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: mstange → echen
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Regressions: 1578671
Whiteboard: [adv-main71+r]
Flags: needinfo?(mstange.moz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: