Requires fuzzPriv extension to reproduce: https://github.com/MozillaSecurity/funfuzz/tree/master/dom/extension ==18157==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000048 (pc 0x7f4340360a5f bp 0x7fffc39de9f0 sp 0x7fffc39de9b0 T0) #0 0x7f4340360a5e in Get /home/worker/workspace/build/src/obj-firefox/dist/include/PLDHashTable.h:228:26 #1 0x7f4340360a5e in PLDHashTable::Add(void const*, mozilla::fallible_t const&) /home/worker/workspace/build/src/xpcom/glue/PLDHashTable.cpp:535 #2 0x7f4347dcf4ee in PutEntry /home/worker/workspace/build/src/obj-firefox/dist/include/nsTHashtable.h:161:36 #3 0x7f4347dcf4ee in Put /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:150 #4 0x7f4347dcf4ee in Put /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:142 #5 0x7f4347dcf4ee in mozilla::a11y::DocManager::CreateDocOrRootAccessible(nsIDocument*) /home/worker/workspace/build/src/accessible/base/DocManager.cpp:503 #6 0x7f4347dcf458 in GetDocAccessible /home/worker/workspace/build/src/accessible/base/DocManager.cpp:67:10 #7 0x7f4347dcf458 in mozilla::a11y::DocManager::CreateDocOrRootAccessible(nsIDocument*) /home/worker/workspace/build/src/accessible/base/DocManager.cpp:489 #8 0x7f4347df2bdd in GetDocAccessible /home/worker/workspace/build/src/accessible/base/DocManager.cpp:67:10 #9 0x7f4347df2bdd in mozilla::a11y::SelectionManager::NotifySelectionChanged(nsIDOMDocument*, nsISelection*, short) /home/worker/workspace/build/src/accessible/base/SelectionManager.cpp:175 #10 0x7f43470b5719 in mozilla::dom::Selection::NotifySelectionListeners() /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:6247:5 #11 0x7f43470c935b in NotifySelectionListeners /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:2429:12 #12 0x7f43470c935b in mozilla::dom::Selection::RemoveAllRanges(mozilla::ErrorResult&) /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:4934 #13 0x7f43470b5b3d in mozilla::dom::Selection::RemoveAllRanges() /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:4915:3 ... see log.txt

The control can go away before a11y is shutdown. We then don't remove the relevant selection listeners, and encounter a use after free.

Comment on attachment 8882344 [details] [diff] [review] Don't use control's frame as reference in SelectionManager. The control can go away before a11y is shutdown. We then don't remove the relevant selection listeners, and encounter a use after free.

(In reply to Eitan Isaacson [:eeejay] from comment #2) > Created attachment 8882344 [details] [diff] [review] > Don't use control's frame as reference in SelectionManager. > > The control can go away before a11y is shutdown. We then don't remove > the relevant selection listeners, and encounter a use after free. You switch from a weak frame to weak selection pointers, the crash happens when selection is changed and when the doc manager seems dead? I'm not sure if I follow the change after the long vacations. Could you provide more details please?

Yeah, the doc manager goes away, but we fail to remove listeners. 1. a page is loaded. 2. a11y starts. a. SelectionManager::SetControlSelectionListener is called on the focused element (in this test case, the autofocused button). i. The element's frame is stored weakly. ii. We get the frame's associated nsFrameSelection (it is not a 1:1 association. The nsFrameSelection can be owned by an ancestor, or even the top PresShell). iii. The SelectionManager instance is set up as a listener for the nsFrameSelection's selections. 4. The page is unloaded and the frame is destroyed. 5. a11y is shutdown. a. SelectionManager::Shutdown is called. i. SelectionManager::ClearControlSelectionListener is called ii. The WeakFrame is now "null" because the backed frame is gone, so ClearControlSelectionListener returns without removing the listeners. 6. A selection change occurs (eg. changing focus or clicking anywhere in the location bar). a. SelectionManager::NotifySelectionChanged is called on a dead object. b. DocManager::CreateDocOrRootAccessible is called on a dead object. c. Get is called on a freed hash table. d. CRASH

Comment on attachment 8882344 [details] [diff] [review] Don't use control's frame as reference in SelectionManager. Review of attachment 8882344 [details] [diff] [review]: ----------------------------------------------------------------- thanks for the explanation, now it makes sense with me ::: accessible/base/SelectionManager.h @@ +124,4 @@ > int32_t mCaretOffset; > HyperTextAccessible* mAccWithCaret; > + nsWeakPtr mCurrCtrlNormalSel; > + nsWeakPtr mCurrCtrlSpellSel; We probably want to keep those as strong references since weak references often go at performance cost and either way they shouldn't be destroyed since we add ourselves as a selection listener, I'll ping Eshan on this.

Ehsan, would you take a look at the patch and advise whether it's preferable to use weak or strong references.

I think the correct solution for this bug is to use strong references, since you really want to clear the selection listeners when it's time as far as I understand. You'd need to make this object participate in cycle collection for those new strong reference members, by adding the appropriate macros. You can use the TypeInState class <https://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/editor/libeditor/TypeInState.h#44> which is very similar as an example.

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #8) > I think the correct solution for this bug is to use strong references, since > you really want to clear the selection listeners when it's time as far as I > understand. Is there any guarantee that nsISelection object is never destroyed while somebody keep itself added by AddSelectionListener? If this is true, then would raw pointers to nsISelection object be acceptable solution?

(In reply to alexander :surkov from comment #9) > (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from > comment #8) > > I think the correct solution for this bug is to use strong references, since > > you really want to clear the selection listeners when it's time as far as I > > understand. > > Is there any guarantee that nsISelection object is never destroyed while > somebody keep itself added by AddSelectionListener? Currently yes, because <https://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/dom/base/Selection.cpp#3630> holds a strong pointer to them. > If this is true, then > would raw pointers to nsISelection object be acceptable solution? No, because the code I linked to above may change in the future to not hold strong pointers and if that happens then such raw pointers will be prone to use after free issues.

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #10) > > If this is true, then > > would raw pointers to nsISelection object be acceptable solution? > > No, because the code I linked to above may change in the future to not hold > strong pointers and if that happens then such raw pointers will be prone to > use after free issues. Fair enough. Cycle collections is not a trivial thing and not error-proof though, which also worries me. Can't we have any kind of static assertions for this case to make sure nsISelection object holds strong pointers?

(In reply to alexander :surkov from comment #11) > (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from > comment #10) > > > > If this is true, then > > > would raw pointers to nsISelection object be acceptable solution? > > > > No, because the code I linked to above may change in the future to not hold > > strong pointers and if that happens then such raw pointers will be prone to > > use after free issues. > > Fair enough. Cycle collections is not a trivial thing and not error-proof > though, which also worries me. It is quite easy to add and I gave an example already, not sure what the complication is or why you think it is error prone. If you have any questions please feel free to ask! > Can't we have any kind of static assertions for this case to make sure > nsISelection object holds strong pointers? I don't see how that would work. The lifetime of this object isn't known at compile time, so no static assertions can be made about its lifetime.

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #12) > (In reply to alexander :surkov from comment #11) > > Fair enough. Cycle collections is not a trivial thing and not error-proof > > though, which also worries me. > > It is quite easy to add and I gave an example already, not sure what the > complication is or why you think it is error prone. cycle collection is indeed simple in simple cases, it gets more complicated in complex cases, where inheritance, cross references between classes including strong pointers, weak pointers, raw pointers are involved. In this particular case, agreed, it may be that simple as your example, since the derived classes don't have cycle collection stuff. > > Can't we have any kind of static assertions for this case to make sure > > nsISelection object holds strong pointers? > > I don't see how that would work. The lifetime of this object isn't known at > compile time, so no static assertions can be made about its lifetime. You could ensure that nsISelectionListener objects are stored as nsCOMPtr types, for example, class Selection { public: typedef nsCOMPtr<nsISelectionListener> listeners_type; private: FallibleTArray<listeners_type> mSelectionListeners; }; // SelectionManager.cpp static_assert(std::is_same<Selection::listeners_type, nsCOMPtr<nsISelectionListener>>::value, "Bla"); That should assure you that the contract between these classes stays unchanged. I bet this is probably not a nicest way ever, but good c++'ers might keep more elegant ideas in a pocket. cycle collection goes at perf cost, no? the more classes participates in cycle collection, the longer chains of cycle collected objects to deal with?

(In reply to alexander :surkov from comment #13) > (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from > comment #12) > > (In reply to alexander :surkov from comment #11) > > > > Fair enough. Cycle collections is not a trivial thing and not error-proof > > > though, which also worries me. > > > > It is quite easy to add and I gave an example already, not sure what the > > complication is or why you think it is error prone. > > cycle collection is indeed simple in simple cases, it gets more complicated > in complex cases, where inheritance, cross references between classes > including strong pointers, weak pointers, raw pointers are involved. In this > particular case, agreed, it may be that simple as your example, since the > derived classes don't have cycle collection stuff. Sure, but the difficulty of doing it doesn't make it the wrong solution. :-) > > > Can't we have any kind of static assertions for this case to make sure > > > nsISelection object holds strong pointers? > > > > I don't see how that would work. The lifetime of this object isn't known at > > compile time, so no static assertions can be made about its lifetime. > > You could ensure that nsISelectionListener objects are stored as nsCOMPtr > types, for example, > > class Selection { > public: > typedef nsCOMPtr<nsISelectionListener> listeners_type; > private: > FallibleTArray<listeners_type> mSelectionListeners; > }; This is indeed how it works: <https://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/base/Selection.h#445> > // SelectionManager.cpp > > static_assert(std::is_same<Selection::listeners_type, > nsCOMPtr<nsISelectionListener>>::value, "Bla"); > > That should assure you that the contract between these classes stays > unchanged. I bet this is probably not a nicest way ever, but good c++'ers > might keep more elegant ideas in a pocket. This means super tight coupling of all types in the type system which makes code hard to maintain, so it's a pretty bad solution IMO. It also makes the updating of all of the consumer sites the problem of whoever that wants to change Selection.h and the like, which raises the maintenance cost of that code quite significantly. In this case the other solution is for us to add a notification of some sort to clear the weak pointer manually so that you can use a raw pointer to implement it manually. But that may be too much work here, I'm not sure... > cycle collection goes at perf cost, no? the more classes participates in > cycle collection, the longer chains of cycle collected objects to deal with? I don't think the cycle collection cost increases linearly with the number of classes participating in cycle collection but I'm not quite sure on that.

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #14) > > cycle collection is indeed simple in simple cases, it gets more complicated > > in complex cases, where inheritance, cross references between classes > > including strong pointers, weak pointers, raw pointers are involved. In this > > particular case, agreed, it may be that simple as your example, since the > > derived classes don't have cycle collection stuff. > > Sure, but the difficulty of doing it doesn't make it the wrong solution. :-) not arguing on this, but it makes me wonder though whether other solutions exists :) > > // SelectionManager.cpp > > > > static_assert(std::is_same<Selection::listeners_type, > > nsCOMPtr<nsISelectionListener>>::value, "Bla"); > > > > That should assure you that the contract between these classes stays > > unchanged. I bet this is probably not a nicest way ever, but good c++'ers > > might keep more elegant ideas in a pocket. > > This means super tight coupling of all types in the type system which makes > code hard to maintain, so it's a pretty bad solution IMO. It also makes the > updating of all of the consumer sites the problem of whoever that wants to > change Selection.h and the like, which raises the maintenance cost of that > code quite significantly. totally agreed if it's taken as a general pattern, however Selection code probably hasn't changed for last 10 years, and if one day somebody would want to change it, then that guy is likely has to revisit anyway all code paths, where selection is used. So in this particular case I wouldn't expect a high maintenance fee. Having said, I don't try to sell this approach, it feels ugly anyway, I just gave it as an example of alternatives approaches for our conversation. > In this case the other solution is for us to add a notification of some sort > to clear the weak pointer manually so that you can use a raw pointer to > implement it manually. But that may be too much work here, I'm not sure... This is interesting idea indeed, seems out of scope of this bug though. Do you want me to file bug on it to get some attention to it? > > cycle collection goes at perf cost, no? the more classes participates in > > cycle collection, the longer chains of cycle collected objects to deal with? > > I don't think the cycle collection cost increases linearly with the number > of classes participating in cycle collection but I'm not quite sure on that. ok, anyway we probably don't have other safe solution for now. Eitan, how do you feel about updating the patch for cycle collection stuff as Ehsan advises?

(In reply to alexander :surkov from comment #15) > > In this case the other solution is for us to add a notification of some sort > > to clear the weak pointer manually so that you can use a raw pointer to > > implement it manually. But that may be too much work here, I'm not sure... > > This is interesting idea indeed, seems out of scope of this bug though. Do > you want me to file bug on it to get some attention to it? No, my preferred solution here is comment 8. :-) I honestly feel like I don't have anything more to add to the discussion here. Using a weak pointer here is fine too I guess, it just has a bit more overhead, it's really up to you. I've now spent too much time discussing this bug. :-)

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #16) > No, my preferred solution here is comment 8. :-) I honestly feel like I > don't have anything more to add to the discussion here. Using a weak > pointer here is fine too I guess, it just has a bit more overhead, it's > really up to you. I've now spent too much time discussing this bug. :-) Thanks! It is indeed a long discussion but it was nice conversation overall. I'm flexible on the approach either. So Eitan, it's your call then, which way to choose.

I'm out of my depth with implementing this correctly with cycle collection. It needs to be thread-safe because nsAccessibilityService inherits both from this and DocManager. I'm sure there is some way to do it, we can open a bug for that if it's important.

This version just sets the weak references to null after removing the SelectionManager as listener. This is needed because sometimes we are registered twice as a listener if the control and doc selection are identical. If we don't set that to null, we risk removing both listeners.

Comment on attachment 8890526 [details] [diff] [review] Don't use control's frame as reference in SelectionManager. [Security approval request comment] How easily could an exploit be constructed based on the patch? I don't think it would be easy, for 3 reasons: 1. The patch does not make it obvious that this is a fix to a UAF. 2. This is a race condition that can't be reproduced on-demand. 3. This requires more than simple content access, the attacker would need to turn browser accessibility on and off. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? I don't believe so. The patch innocently changes the way we hold a reference to an object that is only tangentially related to the UAF. Which older supported branches are affected by this flaw? This was introduced in version 53. If not all supported branches, which bug introduced the flaw? bug 527003 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? It would not be hard to backport. How likely is this patch to cause regressions; how much testing does it need? This patch benefits from mochitests we already have.

[Tracking Requested - why for this release]:

Comment on attachment 8890526 [details] [diff] [review] Don't use control's frame as reference in SelectionManager. sec-approval+ for trunk. I've spoken to Ritu in Release Management. We'll want a Beta patch made and nominated ASAP as well so it can make the last beta.

https://hg.mozilla.org/integration/mozilla-inbound/rev/28b09080d79ac444361f5d9f2e3c851ba9ec97f8 This grafts cleanly to Beta. Just need an approval request on the patch :)

Comment on attachment 8890526 [details] [diff] [review] Don't use control's frame as reference in SelectionManager. Al and I talked about this one. Unless the dev feels this is a risky patch to take in Beta, let's uplift and include this in b13 tomorrow. Julien FYI

Looks like this got approved! Unsetting NI.

https://hg.mozilla.org/releases/mozilla-beta/rev/7fc15cbb8026 We should probably try to land this test at some point after Fx55 is released.