1819160 - Crash in [@ mozilla::a11y::Accessible::IsLocal] on poison values

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/1c8b7a96-5e1d-408e-88e7-2224d0230226

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0  libxul.so  mozilla::a11y::Accessible::IsLocal const  accessible/basetypes/Accessible.h:568
0  libxul.so  mozilla::a11y::AccessibleWrap::SetVirtualViewID  accessible/android/AccessibleWrap.cpp:585
1  libxul.so  mozilla::a11y::SessionAccessibility::RegisterAccessible  accessible/android/SessionAccessibility.cpp:1064
2  libxul.so  mozilla::a11y::AccessibleWrap::AccessibleWrap  accessible/android/AccessibleWrap.cpp:49
3  libxul.so  mozilla::a11y::HyperTextAccessible::HyperTextAccessible  accessible/generic/HyperTextAccessible.cpp:173
4  libxul.so  mozilla::a11y::HTMLLinkAccessible::HTMLLinkAccessible  accessible/html/HTMLLinkAccessible.cpp:27
5  libxul.so  $_1::operator const  accessible/base/HTMLMarkupMap.h:8
5  libxul.so  $_1::__invoke  accessible/base/HTMLMarkupMap.h:8
6  libxul.so  nsAccessibilityService::CreateAccessible  accessible/base/nsAccessibilityService.cpp:1169
7  libxul.so  mozilla::a11y::TreeWalker::Next  accessible/base/TreeWalker.cpp:188

Bug 1791152 really reduced the volume of these crashes, but I still see a decent volume of them, on jemalloc poison values like 0xe5e5e5e5e5e5e7ad. I don't know how actionable this is. These crashes look like they are all on Android.

Comment 1

[BugBot [:suhaib / :marco]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 AArch64 and ARM crashes on nightly

:Jamie, could you consider increasing the severity of this top-crash bug?

For more information, please visit auto_nag documentation.

Comment 2

[Eitan Isaacson [:eeejay]]

I'm still looking into this, and attempting to come up with a solution.

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1819160 - Hold a reference to registering session in DocAccessibleParent. r?Jamie

This should fully eliminate the case where remote accessibles fail to
unregister because of the outer PresShell's lifecycle.

Comment 4

[Eitan Isaacson [:eeejay]]

Comment on attachment 9322826 [details]
Bug 1819160 - Hold a reference to registering session in DocAccessibleParent. r?Jamie

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This is a pretty hard crash to reproduce so I am not worried about that.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?:
• If not all supported branches, which bug introduced the flaw?: Bug 1786584
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Not risky, but because this is hard to reproduce it is worth seeing if the crash is eliminated on Nightly first.
• How likely is this patch to cause regressions; how much testing does it need?:
• Is Android affected?: Yes

Comment 5

[Tom Ritter [:tjr]]

Comment on attachment 9322826 [details]
Bug 1819160 - Hold a reference to registering session in DocAccessibleParent. r?Jamie

Approved to land and uplift

Comment 6

[Eitan Isaacson [:eeejay]]

I'm assuming this will be queued by a member of the security team, right?

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Eitan Isaacson [:eeejay] from comment #6)

I'm assuming this will be queued by a member of the security team, right?

You can go ahead and land it, now that it has sec-approval.

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1819160 - Hold a reference to registering session in DocAccessibleParent. r?Jamie

This should fully eliminate the case where remote accessibles fail to
unregister because of the outer PresShell's lifecycle.

Original Revision: https://phabricator.services.mozilla.com/D172473

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Hold a reference to registering session in DocAccessibleParent. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/04cb1323164e7415e8ce383b553de77a52490a82
https://hg.mozilla.org/mozilla-central/rev/04cb1323164e

Comment 10

[[:fabrice] Fabrice Desré]

Nit: all the ANDROID #ifdef in that patch should be MOZ_WIDGET_ANDROID

Comment 11

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:eeejay, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[Dianna Smith [:diannaS]]

:eeejay We have a couple of betas left in the 112 cycle, is the plan to uplift this to 112 or is it riding the 113 train?

Comment 13

[James Teh [:Jamie]]

It perhaps doesn't make sense to uplift this given that there are still crashes here, so we're not sure if this is the right fix. Eitan, do you want to reopen this?

Comment 14

[James Teh [:Jamie]]

Reopening per discussion with RyanVM.

Comment 15

[BugBot [:suhaib / :marco]]

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit auto_nag documentation.

Comment 16

[Dianna Smith [:diannaS]]

Marking this as fix optional since RC week is next week and there is no upliftable change yet. I would however take this in a respin/dot release if a fix is available.

Comment 17

[Eitan Isaacson [:eeejay]]

Since this is not a new regression in 112, i tend to think this could just ride the trains. Because this is very hard to reproduce I don't see this as exploitable or a real issue.

Comment 18

[Daniel Veditz [:dveditz]]

(In reply to Eitan Isaacson [:eeejay] from comment #17)

Since this is not a new regression in 112, i tend to think this could just ride the trains.

What would ride the trains? In comment 14 this was reopened which, without explanation, implies the fix didn't work. And indeed, I see crashes with this signature referencing the poison value on 113 beta and 114 nightly builds.

Comment 19

[Eitan Isaacson [:eeejay]]

Hi Daniel.

A solution to this has proven elusive. Is this indeed a sec-high bug? Seems like the crash volume is low, and it has been very hard to reproduce so I don't know how exploitable this is.

Comment 20

[Daniel Veditz [:dveditz]]

Yes, these are the symptoms of a sec-high bug. The volume of crashes is unrelated to what an attacker could do if they discover how to trigger it intentionally. Many times exploitable crashes don't occur at all in crash-stats if the triggering conditions are very specific. Attackers generally aren't looking at crash-stats to find exploitable bugs because they run into the same problem we do trying to work backwards to the triggering conditions. More likely they will work forward from fuzzing where they have a starting testcase (advantage to the attacker: we have no idea which of the many exploitable-looking crashes they might rediscover, and they only need one).

Is there a way to get more information? Maybe a bunch of added telemetry could check and track "impossible" conditions throughout the code to see where along the way we get into this state? Or MOZ_RELEASE_ASSERT()s that would crash earlier but more definitively when we've gotten into this bad state? MOZ_DIAGNOSTIC_ASSERTs would be less disruptive but I don't think we see enough crashes in beta to get the info we need. Or could you work with the fuzzing team and expose hooks or testing APIs that would help them detect these crashes with a reproducible test case? They might not even be testing a11y on Android, in which case running any at all might shake this out.

Or it may be time to cut losses and ask your manager to mark this "stalled" for now. There are plenty of other bugs that need work. "stalled" is appropriate when we're stumped and don't have any reasonable way forward to get more information that might help. Please talk about this with your team to see what's appropriate here.

Comment 21

[James Teh [:Jamie]]

Unfortunately, we can't use WeakPtr because it requires that you acquire, dereference and destroy the reference on the same thread. Even though we'd acquire and release on the main thread, we would sometimes dereference in the UI thread.

Eitan, even though we now hold a reference to the SessionAccessibility from DocAccessibleParent, are we absolutely certain that a DOM Document will always still have a reference to its PresShell in the local case? I'm not convinced.

Our understanding of the lifecycle of certain DOM things is a bit blurry and there is a lot more complexity. On the other hand, I think we understand the lifecycle of a11y objects fairly well and we also have a lot more control over that. Thus, I'm wondering whether we should maintain a global static map from document to SessionAccessibility; i.e. nsTHashMap<Accessible*, RefPtr<SessionAccessibility>>. When a document shuts down (DocAccessible::Shutdown/DocAccessibleParent::Destroy), remove the entry for that doc. This way, we don't have to worry about whether a DocAccessibleParent is detached, whether a DocAccessible's PresShell is still alive, etc. This ensures we always correctly register and unregister Accessibles; there's never a risk that we fail to unregister because we (hopefully) clearly understand the lifecycle of a11y documents.

Alternatively (or in addition), we could change the id map to hold a reference to the document as well as the target Accessible; e.g. nsTHashMap<uint32_t, std::pair<RefPtr<DocAccessibleParent>, Accessible*>. Because we have no base ref-countable document type, we'd need two maps, one for DocAccessible and one for DocAccessibleParent. We'd search both maps when looking for an id.

With the document reference, raw pointers to the Accessibles should be fine, since we know that if the document is alive, it should be correctly notifying us of registration/unregistration. However, if we wanted to be super paranoid, we could store ids instead; i.e. nsTHashMap<uint32_t, std::pair<RefPtr<DocAccessibleParent>, uint64_t>. We'd then use nsAccUtils::GetAccessibleByID. There's a risk we could return the wrong Accessible, but that's far better than a UAF. It does mean more hash lookups though.

Eitan, what do you think?

Comment 22

[James Teh [:Jamie]]

As an alternative to the global map of documents, I guess we could just add a RefPtr<SessionAccessibility> to DocAccessible as well, the same as we've done for DocAccessibleParent. It's a bit ugly - I don't love having platform specific stuff there - but I guess a global map isn't great either (except that the ugliness would be inside the Android code rather than cross-platform).

Comment 23

[James Teh [:Jamie]]

Also, can the IsDetachedDoc thing happen any more now that DocAccessibleParent holds a reference to SessionAccessibility? I don't think it can?

Comment 24

[Andrew McCreight [:mccr8]]

There is a ThreadSafeWeakPtr class in mfbt/, though I haven't used it myself.

Comment 25

[Eitan Isaacson [:eeejay]]

Two thoughts:

1. We can probably choose between a thread safe and not thread safe refcounting smart pointer type during preprocessing so we only use the thread safe one in android and don't burden other platforms with that.
2. I like the idea of nsTHashMap<uint32_t, std::pair<RefPtr<DocAccessibleParent>, Accessible*> as a change for the id map. We do something similar in mac's TextMarker where we pass accessible raw pointers as opaque data to/from the OS. This can be a very minimal change, we can land that and see how it goes. This means reverting the previous patch where we hold a reference for SessionAccessibility in DocAccessibleParent, or dealing with the cycle correctly. I would just remove that temporarily and see what happens in nightly.. So I think I would go ahead with that and defer any further investigation into RemoteAccessible refcounting.

Comment 26

[James Teh [:Jamie]]

(In reply to Andrew McCreight [:mccr8] from comment #24)

There is a ThreadSafeWeakPtr class in mfbt/, though I haven't used it myself.

Unfortunately, that requires that the object support strong references, which is a large chunk of work we're trying to avoid unless we have no choice.

Comment 27

[James Teh [:Jamie]]

(In reply to Eitan Isaacson [:eeejay] from comment #25)

We do something similar in mac's TextMarker where we pass accessible raw pointers as opaque data to/from the OS.

Looking briefly, I think Mac text markers use ids for the Accessible, not raw pointers? That makes sense because there would be no way to verify that the Accessible still exists. The document is a raw pointer, but that's verified by walking the document tree.

In this case, the target Accessible would be a raw pointer and we're relying on our (hopefully solid) understanding of the lifecycle to be certain that it isn't destroyed underneath us. An id would be safer still, but it means an extra hash lookup for every Android query.

Comment 28

[Eitan Isaacson [:eeejay]]

It would be a similar lookup process (using a doc/accessible 64bit id pair) , except the 32 bit integer will be another level of redirection. I'll let the patch speak.

Comment 29

[James Teh [:Jamie]]

Ah okay. So that'd be nsTHashMap<uint32_t, std::pair<RefPtr<DocAccessibleParent>, uint64_t>>; i.e. uint64_t for an id, not Accessible* for a raw target pointer.

Comment 30

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1819160 - Map Android ids to doc/accessible id pairs. r?Jamie

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Map Android ids to doc/accessible id pairs. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/2d8fa86b279358b19ca794787248a9600c86ef5a
https://hg.mozilla.org/mozilla-central/rev/2d8fa86b2793

Comment 32

[Donal Meehan [:dmeehan]]

:eeejay could you add a beta uplift request on this when ready?

Comment 33

[Donal Meehan [:dmeehan]]

:tjr can you chime in, this landed in central for 116.
Looks like we need to uplift this to beta for 115, unless there was a reason not to and that was discussed outside bugzilla?

Comment 34

[Tom Ritter [:tjr]]

No, I'm not aware of any reason we shouldn't uplift this.

Comment 35

[James Teh [:Jamie]]

We can uplift this if security believes it's super important. However, it is not a low risk patch; it fundamentally changes the way a11y id mapping works on Android. Given the time left on beta this cycle, I'm not convinced it's worth the risk.

Comment 36

[Tom Ritter [:tjr]]

Hm, fair enough. Can we aim to ship it in the mid-cycle Fenix dot release?

Comment 37

[James Teh [:Jamie]]

I guess any problems should have shown up on nightly or beta by then, so that seems reasonable.

Comment 38

[James Teh [:Jamie]]

Ryan, did you intentionally mark this as unaffected for ESR 115? I don't see why it would be unaffected given that the bug exists in 115. Just wanted to double check before adjusting the flag myself.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

We don't ship Android releases from ESR. So technically it's affected, but in practice it isn't.