Closed
Bug 1228882
Opened 9 years ago
Closed 9 years ago
"ASSERTION: element already removed from map" or heap-buffer-overflow with <bdi>, dir=auto
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: jruderman, Assigned: ehsan.akhgari)
References
Details
(5 keywords, Whiteboard: [adv-main46+][adv-esr45.1+])
Attachments
(7 files, 1 obsolete file)
136 bytes,
text/html
|
Details | |
5.83 KB,
text/plain
|
Details | |
72.23 KB,
text/plain
|
Details | |
5.17 KB,
text/plain
|
Details | |
3.80 KB,
patch
|
peterv
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr38+
Sylvestre
:
approval-mozilla-esr45+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
3.32 KB,
patch
|
Details | Diff | Splinter Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
Nonfatal assertion or scary crash, depending on how long the testcase was open:
1. Load
2. Quit
###!!! ASSERTION: element already removed from map: 'mElements.Contains(aElement)', file dom/base/DirectionalityUtils.cpp, line 469
1. Load
2. Wait for GC
3. Quit
heap-buffer-overflow
#2 mozilla::nsTextNodeDirectionalityMap::RemoveElementFromMap
#3 mozilla::ResetDir
...
#12 nsCycleCollector::ShutdownCollect
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Comment 2•9 years ago
|
||
rax = 0xe5e5e5e5e5e5e5e5, but crash address is 0x0000000d
Reporter | ||
Comment 3•9 years ago
|
||
Reporter | ||
Comment 4•9 years ago
|
||
> rax = 0xe5e5e5e5e5e5e5e5, but crash address is 0x0000000d
So it was mostly luck that I found this bug despite having bug 836925 in my ignore lists
Updated•9 years ago
|
Keywords: csectype-uaf,
sec-critical
Comment 6•9 years ago
|
||
FWIW, I tried the patch in bug 836925 but that didn't help.
It might be the same issue as bug 900997?
Flags: needinfo?(mats) → needinfo?(smontagu)
Assignee | ||
Comment 7•9 years ago
|
||
The issue here is very subtle. When we're setting the dir to "auto" on the span, we get to this code: <https://dxr.mozilla.org/mozilla-central/source/dom/base/DirectionalityUtils.cpp#509> under this stack:
#0 mozilla::nsTextNodeDirectionalityMap::ResetNodeDirection (aEntry=0x7f110adf3140, aData=0x7f110adea4c0) at /home/ehsan/moz/src/dom/base/DirectionalityUtils.cpp:519
#1 0x00007f11271a56ba in nsCheapSet<nsPtrHashKey<mozilla::dom::Element> >::EnumerateEntries (this=0x7f110adf3140,
aEnumFunc=0x7f112719da79 <mozilla::nsTextNodeDirectionalityMap::ResetNodeDirection(nsPtrHashKey<mozilla::dom::Element>*, void*)>, aUserArg=0x7f110adea4c0)
at /home/ehsan/moz/src/xpcom/ds/nsCheapSets.h:79
#2 0x00007f112719dc33 in mozilla::nsTextNodeDirectionalityMap::ResetAutoDirection (this=0x7f110adf3140, aTextNode=0x7f110adea4c0)
at /home/ehsan/moz/src/dom/base/DirectionalityUtils.cpp:538
#3 0x00007f112719de77 in mozilla::nsTextNodeDirectionalityMap::ResetTextNodeDirection (aTextNode=0x7f110adea4c0) at /home/ehsan/moz/src/dom/base/DirectionalityUtils.cpp:577
#4 0x00007f112716d0ed in mozilla::WalkDescendantsResetAutoDirection (aElement=0x7f110adea430) at /home/ehsan/moz/src/dom/base/DirectionalityUtils.cpp:684
#5 0x00007f112716d8f9 in mozilla::OnSetDirAttr (aElement=0x7f110adea430, aNewValue=0x7ffde504a840, hadValidDir=false, hadDirAuto=false, aNotify=true)
at /home/ehsan/moz/src/dom/base/DirectionalityUtils.cpp:923
#6 0x00007f11271769ac in mozilla::dom::Element::SetAttrAndNotify (this=0x7f110adea430, aNamespaceID=0, aName=0x7f1114df09d0, aPrefix=0x0, aOldValue=..., aParsedValue=...,
aModType=2 '\002', aFireMutation=false, aNotify=true, aCallAfterSetAttr=true) at /home/ehsan/moz/src/dom/base/Element.cpp:2420
Here, rootNode is the <bdi> element which doesn't have a dir attribute yet. But HasDirAuto() returns true for it, since it's a <bdi> element! <https://dxr.mozilla.org/mozilla-central/source/dom/base/Element.h#353> Therefore, we start going into the weeds, like this. We call WalkDescendantsSetDirectionFromText(), and it first finds the "f" text node, but it won't use it because that's aChangedNode, so it keeps looking, and then it finds the "g" text node and incorrectly determines that "g" determines the directionality of our <bdi> node. Later when we set the dir="auto" attribute on the <bdi> element itself, we correctly decide that "f" determines the directionality of the <bdi> and later when we remove the span, we correctly clear the directionality map entry for "f" but not "g" (since that is in an inconsistent state) and we lose. IOW, this bug can only be triggered by this *exact* test case. ;-)
The fix is very simple. We need to run the downward propagation algorithm only if we've encountered a real dir=auto element, not a <bdi> without one.
Assignee: nobody → ehsan
Flags: needinfo?(smontagu)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8704388 -
Flags: review?(bugs)
Comment 9•9 years ago
|
||
Comment on attachment 8704388 [details] [diff] [review]
Only run the downward propagation algorithm for elements with a real dir=auto attribute
I'm not really familiar with this stuff.
Like, I have no idea when "downward propagation algorithm" should run and when not. Based on Bug 613149, peterv might be better reviewer for this code.
Feels _very _error prone to have a method called HasDirAuto() which doesn't mean what we want here.
The code has
1491 // Set if the node has dir=auto.
1492 NodeHasDirAuto
That clearly hints that attribute value is auto.
But based on the code <bdi> sets the flag too. So the comment is wrong at least, but I'd
say also the flag name and its setter/getter.
Did you go through other HasDirAuto() usage?
So, I would change HasDirAuto() implementation to
bool HasDirAuto() { return HasFixedDir() && HasValidDir(); }
and then rename NodeHasDirAuto to something else. NodeBehavesAsDirAuto or some such.
But ok, that becomes rather large change, not anything for branches.
Attachment #8704388 -
Flags: review?(bugs) → review?(peterv)
Updated•9 years ago
|
status-firefox43:
--- → ?
status-firefox44:
--- → ?
status-firefox46:
--- → affected
status-firefox-esr38:
--- → ?
tracking-firefox45:
--- → +
tracking-firefox46:
--- → +
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8704388 [details] [diff] [review]
Only run the downward propagation algorithm for elements with a real dir=auto attribute
There are some test failures on try...
Attachment #8704388 -
Flags: review?(peterv)
Comment 11•9 years ago
|
||
Ehsan is this something we should be trying to get onto 44? Is it affected? Are you still working on the patch? Or is the problem in the patch? Thanks.
We should track this for 44+ since it's sec-critical, but it looks like it may end up wontfix for 44.
tracking-firefox43:
--- → +
tracking-firefox44:
--- → +
Flags: needinfo?(rkothari)
Flags: needinfo?(ehsan)
I will gtb Beta9 tomm and enter RC mode next week. If a stable fix is ready and nominated for uplift soon, we can consider taking the fix in Beta44.
Flags: needinfo?(rkothari)
Assignee | ||
Comment 13•9 years ago
|
||
I'm still struggling to come up with a good fix, but it's been years since I looked at this stuff. Also I still think that my analysis on the edge casey-ness of this is correct, so I'm not in a rush to get this fixed in 44.
Flags: needinfo?(ehsan)
Comment 14•9 years ago
|
||
I'll just mark this wontfix in 44 for now. Thanks for investigating this issue, Ehsan.
Comment 15•9 years ago
|
||
Tracking for 47.
status-firefox47:
--- → affected
tracking-firefox47:
--- → +
Comment 16•9 years ago
|
||
This is likely heading for a wontfix for 45 and 46 as well.
Are we ok with shipping several releases in a row with this sec-critical issue, even if it is an edge case?
Assignee | ||
Comment 17•9 years ago
|
||
I'll try to prioritize this... (Note that I'm traveling next week and there is a good chance I won't get to this one this week.)
Updated•9 years ago
|
Assignee | ||
Comment 18•9 years ago
|
||
After further investigation, I think most of comment 7 is correct, but the conclusion I originally drew from it is not.
The real problem here is that WalkDescendantsSetDirectionFromText() thinks that the "f" text node is being changed, which is the beginning of things getting into an inconsistent state. We use this function from three places: the call site in question, TextNodeChangedDirection() and ResetDirectionSetByTextNode(). In the latter two cases, the contents of the text node are changing in a way that make it impossible for it to affect the direction of dir=auto ancestors. However, in the call site in question, all we're doing is re-resolving the auto direction of any possible ancestors, but we know that the text node hasn't necessarily changed in a way to make it no longer responsible for any dir=auto ancestors. Therefore, in this call site we should be passing null for aChangedNode. Once that happens, following comment 7, the "f" text node will be picked, which is the correct text node to choose here.
Assignee | ||
Comment 19•9 years ago
|
||
Attachment #8728062 -
Flags: review?(peterv)
Assignee | ||
Updated•9 years ago
|
Attachment #8704388 -
Attachment is obsolete: true
Updated•9 years ago
|
Comment 20•9 years ago
|
||
I had a question about oldTextNode at https://dxr.mozilla.org/mozilla-central/source/dom/base/DirectionalityUtils.cpp#506, which is then passed to WalkDescendantsSetDirectionFromText as aChangedNode. We cast it to Element*, but from your comments here it sounds like it's really a text node. WalkDescendantsSetDirectionFromText has this check: |child->NodeType() == nsIDOMNode::TEXT_NODE && child != aChangedNode|, which if aChangedNode is an element is an odd condition. Do you know what's really going on there?
Flags: needinfo?(ehsan)
Assignee | ||
Comment 21•9 years ago
|
||
This argument is only useful when it points to a text node (hence the check in WalkDescendantsSetDirectionFromText). The cast to Element there is wrong, and we're getting lucky since the pointer math turns out to the correct case. Also calling this "text node" along the way is not really helpful. I submitted a patch to improve things in bug 1257208 to keep the cleanup separate from this bug.
Flags: needinfo?(ehsan)
Updated•9 years ago
|
Attachment #8728062 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8728062 [details] [diff] [review]
Don't assume that the textnode has changed when checking whether any textnode descendants determine the directionality of a dir=auto ancestor
[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not very easily.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. The commit message basically describes the code change in English.
Which older supported branches are affected by this flaw? All.
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? No, and very easy (if the patch doesn't apply verbatim.)
How likely is this patch to cause regressions; how much testing does it need? It passed our existing tests. Still I think some baking time would be needed since the code in question is pretty complex.
Attachment #8728062 -
Flags: sec-approval?
Comment 23•9 years ago
|
||
Comment on attachment 8728062 [details] [diff] [review]
Don't assume that the textnode has changed when checking whether any textnode descendants determine the directionality of a dir=auto ancestor
sec-approval+. We'll want Aurora and Beta patches made and nominated. I suspect we'll want this on ESR38 and ESR45 also.
Attachment #8728062 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8728062 [details] [diff] [review]
Don't assume that the textnode has changed when checking whether any textnode descendants determine the directionality of a dir=auto ancestor
Please refer to comment 22, I won't repeat the same info here.
Attachment #8728062 -
Flags: approval-mozilla-esr45?
Attachment #8728062 -
Flags: approval-mozilla-esr38?
Attachment #8728062 -
Flags: approval-mozilla-beta?
Attachment #8728062 -
Flags: approval-mozilla-aurora?
Comment 25•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
tracking-firefox-esr38:
--- → ?
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•9 years ago
|
Group: dom-core-security → core-security-release
Ehsan, you suggested some bake time in comment 22. Is a week in Nightly enough for this patch before uplifting to all affected branches? More?
Flags: needinfo?(ehsan)
Assignee | ||
Comment 27•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #26)
> Ehsan, you suggested some bake time in comment 22. Is a week in Nightly
> enough for this patch before uplifting to all affected branches? More?
Honestly I don't know how to judge how much time is the right amount, so sure. I haven't seen any regressions from this yet.
Flags: needinfo?(ehsan)
Comment 28•9 years ago
|
||
Landing now in beta (for beta 5) means we have some time to fix possible regressions in beta. So I am inclined to uplift now.
Comment 29•9 years ago
|
||
Comment on attachment 8728062 [details] [diff] [review]
Don't assume that the textnode has changed when checking whether any textnode descendants determine the directionality of a dir=auto ancestor
Crash/assertion fix, let's land this for beta 5.
Attachment #8728062 -
Flags: approval-mozilla-beta?
Attachment #8728062 -
Flags: approval-mozilla-beta+
Attachment #8728062 -
Flags: approval-mozilla-aurora?
Attachment #8728062 -
Flags: approval-mozilla-aurora+
Comment 30•9 years ago
|
||
Comment 31•9 years ago
|
||
Comment 32•9 years ago
|
||
Comment on attachment 8728062 [details] [diff] [review]
Don't assume that the textnode has changed when checking whether any textnode descendants determine the directionality of a dir=auto ancestor
sec critical, taking it
Should be in 45.1.0 & 38.8.0
Attachment #8728062 -
Flags: approval-mozilla-esr45?
Attachment #8728062 -
Flags: approval-mozilla-esr45+
Attachment #8728062 -
Flags: approval-mozilla-esr38?
Attachment #8728062 -
Flags: approval-mozilla-esr38+
Updated•9 years ago
|
This doesn't apply cleanly to esr38.
Flags: needinfo?(ehsan)
Assignee | ||
Comment 35•9 years ago
|
||
This is the ESR38 patch. Unfortunately with this patch, the following tests fail:
1:15.91 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsan/moz/mozilla-esr38/layout/reftests/bidi/dirAuto/dir_auto-set-contained-dir-L.html | image comparison (==), max difference: 255, number of differing pixels: 4505
1:15.91 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsan/moz/mozilla-esr38/layout/reftests/bidi/dirAuto/dir_auto-set-contained-dir-R.html | image comparison (==), max difference: 255, number of differing pixels: 4473
1:15.91 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsan/moz/mozilla-esr38/layout/reftests/bidi/dirAuto/dir_auto-set-contained-invalid-dir-L.html | image comparison (==), max difference: 255, number of differing pixels: 4505
1:15.91 REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsan/moz/mozilla-esr38/layout/reftests/bidi/dirAuto/dir_auto-set-contained-invalid-dir-R.html | image comparison (==), max difference: 255, number of differing pixels: 4473
Unfortunately I don't have cycles to look into why any time soon. Mats, can you please help here? Much appreciated!
Flags: needinfo?(ehsan)
Attachment #8741526 -
Flags: feedback?(mats)
Comment 36•9 years ago
|
||
I have no clue how this feature is supposed to work, sorry.
Assignee | ||
Comment 37•9 years ago
|
||
Sylvestre, how long is ESR38 going to be supported for? I'm really short on free time these days and would like to avoid spending time on the ESR38 port of this patch if at all possible.
Flags: needinfo?(sledru)
Comment 38•9 years ago
|
||
Ehsan,
Next week's ESR38 (ESR38.8) is the last release. Technically, given its rating, we'd normally fix this in ESR38 given our normal criteria.
Whiteboard: [adv-main46+][adv-esr45.1+]
Comment 39•9 years ago
|
||
I am not happy about doing it but I think we should just wontfix it...
Let's see if we are lucky...
Flags: needinfo?(sledru)
Updated•9 years ago
|
Attachment #8741526 -
Flags: feedback?(mats)
Updated•8 years ago
|
Group: core-security-release
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Assignee | ||
Comment 40•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 41•5 years ago
|
||
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de5ff2a8024f
Add a crashtest based on the test case for the bug
Comment 42•5 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•