"ASSERTION: element already removed from map" or heap-buffer-overflow with <bdi>, dir=auto

RESOLVED FIXED in Firefox 46

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: jruderman, Assigned: Ehsan)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla48
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox43+ wontfix, firefox44+ wontfix, firefox45+ wontfix, firefox46+ fixed, firefox47+ fixed, firefox48+ fixed, firefox-esr3846+ wontfix, firefox-esr4546+ fixed)

Details

(Whiteboard: [adv-main46+][adv-esr45.1+])

Attachments

(6 attachments, 1 obsolete attachment)

Reporter

Description

4 years ago
Posted file testcase
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

4 years ago
Posted file stack for assertion
Reporter

Comment 2

4 years ago
rax = 0xe5e5e5e5e5e5e5e5, but crash address is 0x0000000d
Reporter

Comment 3

4 years ago
Reporter

Comment 4

4 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
To Mats for a look...
Flags: needinfo?(mats)
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

3 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)
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)
Assignee

Comment 10

3 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)
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.
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

3 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)
I'll just mark this wontfix in 44 for now. Thanks for investigating this issue, Ehsan.
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

3 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.)
Assignee

Comment 18

3 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

Updated

3 years ago
Attachment #8704388 - Attachment is obsolete: true
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

3 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)
Attachment #8728062 - Flags: review?(peterv) → review+
Assignee

Comment 22

3 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 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

3 years ago
Flags: in-testsuite?
Assignee

Comment 24

3 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?
https://hg.mozilla.org/mozilla-central/rev/a4e010070e05
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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

3 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)
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 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 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+
This doesn't apply cleanly to esr38.
Flags: needinfo?(ehsan)
Assignee

Comment 35

3 years ago
Posted patch ESR38 patchSplinter Review
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)
I have no clue how this feature is supposed to work, sorry.
Assignee

Comment 37

3 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)
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+]
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)
Attachment #8741526 - Flags: feedback?(mats)
Group: core-security-release
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.