1649253 - Crash in [@ mozilla::a11y::IDSet::ReleaseID]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[[ex-Mozilla] Agi Sferro | :agi]

I can't make out if this is exploitable or not so I'm starting out as it is out of caution. Feel free to open up if it isn't.

This bug is for crash report bp-c3a5b146-ff79-452b-84bb-79b7e0200629.

Top 10 frames of crashing thread:

0 libxul.so mozilla::a11y::IDSet::ReleaseID accessible/base/IDSet.h:89
1 libxul.so mozilla::a11y::DocProxyAccessibleWrap::Shutdown accessible/android/ProxyAccessibleWrap.h:111
2 libxul.so mozilla::a11y::ProxyDestroyed accessible/android/Platform.cpp:50
3 libxul.so mozilla::a11y::DocAccessibleParent::Destroy accessible/ipc/DocAccessibleParent.cpp:743
4 libxul.so mozilla::a11y::DocAccessibleParent::Destroy accessible/ipc/DocAccessibleParent.cpp:716
5 libxul.so mozilla::dom::BrowserParent::DestroyInternal dom/ipc/BrowserParent.cpp:601
6 libxul.so mozilla::dom::BrowserParent::Destroy dom/ipc/BrowserParent.cpp:632
7 libxul.so nsFrameLoader::DestroyDocShell dom/base/nsFrameLoader.cpp:1967
8 libxul.so nsFrameLoaderDestroyRunnable::Run dom/base/nsFrameLoader.cpp:1918
9 libxul.so mozilla::dom::Document::MaybeInitializeFinalizeFrameLoaders dom/base/Document.cpp:8505

Comment 1

[Eitan Isaacson [:eeejay]]

I have a suspicion that ParentDoc() returns null when this proxy is first constructed[1], so it gets an ID of -1, and on destruction it has a parent doc[2], so we attempt to release this non-zero ID and crash.

1. https://searchfox.org/mozilla-central/source/accessible/android/ProxyAccessibleWrap.h#96
2. https://searchfox.org/mozilla-central/source/accessible/android/ProxyAccessibleWrap.h#108

Jamie, does that make sense? Can this be a regression from anything fission related? Looks like this started showing up on a 6/15 build.

Comment 2

[James Teh [:Jamie]]

This can absolutely happen with Fission, and the crash report in comment 0 certainly shows Fission as being enabled. (I didn't know that was possible on Android, but there you go.) AddChildDoc can be called late in two possible places:
https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/dom/ipc/BrowserBridgeParent.cpp#235
https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/accessible/ipc/DocAccessibleParent.cpp#150

Comment 3

[James Teh [:Jamie]]

Note that for an OOP iframe document, even if it has a null parent (because we don't have the parent yet), DocAccessibleParent::IsTopLevelInContentProcess() will return true, but DocAccessibleParent::IsTopLevel() will return false. That might be useful if we need a specific code path to handle this.

Comment 4

[James Teh [:Jamie]]

Eitan, as we discussed, I was going to check for an OOP iframe and assign an id in that case. However, it seems we also want to add that id to the parent document:
https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/accessible/android/ProxyAccessibleWrap.h#98
Of course, we can't do that because we don't have a parent yet.

I see two solutions here:

1. Add an Android specific ifdef to DocAccessibleParent::AddChildDoc which adds the id to the new parent, just before this line:
   https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/accessible/ipc/DocAccessibleParent.cpp#608
   We'd also need to decide whether to acquire the id in the DocProxyAccessibleWrap constructor or acquire it in this new place in DocAccessibleParent.
2. Tweak things a bit so that we don't add a child document's id to its parent document's id map.
   This won't work right now because RootAccessibleWrap::FindAccessibleById expects the id to be accepted by one of the documents. However, we already have to walk the child documents:
   https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/accessible/android/RootAccessibleWrap.cpp#68
   I think we could just check whether the child document id matches at that point. This seems to be what we do for local (non-proxy) Accessibles:
   https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/accessible/android/DocAccessibleWrap.cpp#52
   However, there may be some other reason you did this that I'm missing.

Any thoughts/preferences?

Comment 5

[Eitan Isaacson [:eeejay]]

The second option looks good. Lets not ifdef DocAccesibleParent further! I think we didn't match child doc ids in the remote case just because we didn't have to, since it was easy enough to add to parent in construction. Obviously now with the construction order in disarray, that solution would be fine.

Comment 6

[James Teh [:Jamie]]

Attachment: Bug 1649253: Correctly assign an Android a11y id to OOP iframe documents.

The DocProxyAccessibleWrap for an OOP iframe document is always created before we know its parent.
Previously, this resulted in the document getting an id of -1, which is only valid for the root document.
It also broke subsequent assumptions once the parent was set later.

Because we don't have the parent in this case, we can't add the document's id to its parent's hash table.
To deal with this, we no longer add any document to its parent's hash table.
Instead, when finding an accessible by id, we just check the id of each child document before checking that child document's hash table.

Comment 7

[James Teh [:Jamie]]

Comment on attachment 9160826 [details]
Bug 1649253: Correctly assign an Android a11y id to OOP iframe documents.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it's easy to reproduce this crash, but a) you need Fission enabled (for which there hasn't been a call for dogfooding on Android); and b) the crash address is always the same because the id passed in is always the same (id -1, SIGSEGV /SEGV_MAPERR, 0x20). I actually don't think this needs to be a secure bug, but I don't know enough about this IDSet code and am thus being overly cautious.
• 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?: I think this can only impact Nightly, since the fission.autostart pref is only allowed on Nightly.
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: The automated tests pass and I'd expect them to fail quickly if this were broken. That said, there's still a risk of regressions, albeit very low.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 9160826 [details]
Bug 1649253: Correctly assign an Android a11y id to OOP iframe documents.

not necessary, but sec-approval+

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77019df5373b
Correctly assign an Android a11y id to OOP iframe documents. r=eeejay

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/77019df5373b

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Jamie, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[James Teh [:Jamie]]

This only impacts Fission and Fission is Nightly-only, so this doesn't need an uplift.