High firefox UAF 🔧 Commit mapped

Overview

High
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
Impacthigh
DescriptionAn attacker could have caused a use-after-free when accessibility was enabled, leading to a potentially exploitable crash.
ComponentCore
Bug ClassUAF
Tracker1919087
Fix commit9fafa7208a2e (firefox) +79/-26
CISA KEVNot listed
CreditedTyson Smith
Disclosed2024-10-29

Changed Functions

FunctionChangeNotes
if
accessible/base/EventQueue.cpp
modified
switch
accessible/base/EventQueue.cpp
modified
if
accessible/base/NotificationController.cpp
modified
if
accessible/tests/mochitest/name/markup.js
modified

Files Changed

  • accessible/base/EventQueue.cpp
  • accessible/base/EventQueue.h
  • accessible/base/NotificationController.cpp
  • accessible/tests/mochitest/name/markup.js
diff --git a/accessible/base/EventQueue.cpp b/accessible/base/EventQueue.cpp
index 0b535029e7b..7cfc0fc94b3 100644
--- a/accessible/base/EventQueue.cpp
+++ b/accessible/base/EventQueue.cpp
@@ -51,6 +51,23 @@ bool EventQueue::PushEvent(AccEvent* aEvent) {
   return true;
 }
 
+bool EventQueue::PushNameOrDescriptionChangeToRelations(
+    LocalAccessible* aAccessible, RelationType aType) {
+  MOZ_ASSERT(aType == RelationType::LABEL_FOR || RelationType::DESCRIPTION_FOR);
+
+  bool pushed = false;
+  uint32_t eventType = aType == RelationType::LABEL_FOR
+                           ? nsIAccessibleEvent::EVENT_NAME_CHANGE
+                           : nsIAccessibleEvent::EVENT_DESCRIPTION_CHANGE;
+  Relation rel = aAccessible->RelationByType(aType);
+  while (LocalAccessible* relTarget = rel.LocalNext()) {
+    RefPtr<AccEvent> nameChangeEvent = new AccEvent(eventType, relTarget);
+    pushed |= PushEvent(nameChangeEvent);
+  }
+
+  return pushed;
+}
+
 bool EventQueue::PushNameOrDescriptionChange(AccEvent* aOrigEvent) {
   // Fire name/description change event on parent or related LocalAccessible
   // being labelled/described given that this event hasn't been coalesced, the
@@ -58,14 +75,18 @@ bool EventQueue::PushNameOrDescriptionChange(AccEvent* aOrigEvent) {
   // subtree was changed.
   LocalAccessible* target = aOrigEvent->mAccessible;
   // If the text of a text leaf changed without replacing the leaf, the only
-  // event we get is text inserted on the container. In this case, we might
-  // need to fire a name change event on the target itself.
+  // event we get is text inserted on the container. Or, a reorder event may
+  // change the container's name. In this case, we might need to fire a name
+  // change event on the target itself.
   const bool maybeTargetNameChanged =
       (aOrigEvent->mEventType == nsIAccessibleEvent::EVENT_TEXT_REMOVED ||
-       aOrigEvent->mEventType == nsIAccessibleEvent::EVENT_TEXT_INSERTED) &&
+       aOrigEvent->mEventType == nsIAccessibleEvent::EVENT_TEXT_INSERTED ||
+       aOrigEvent->mEventType == nsIAccessibleEvent::EVENT_REORDER ||
+       aOrigEvent->mEventType == nsIAccessibleEvent::EVENT_INNER_REORDER) &&
       nsTextEquivUtils::HasNameRule(target, eNameFromSubtreeRule);
   const bool doName = target->HasNameDependent() || maybeTargetNameChanged;
   const bool doDesc = target->HasDescriptionDependent();
+
   if (!doName && !doDesc) {
     return false;
   }
@@ -80,12 +101,34 @@ bool EventQueue::PushNameOrDescriptionChange(AccEvent* aOrigEvent) {
     if (doName) {
       if (nameCheckAncestor && (maybeTargetNameChanged || parent != target) &&
           nsTextEquivUtils::HasNameRule(parent, eNameFromSubtreeRule)) {
-        nsAutoString name;
-        ENameValueFlag nameFlag = parent->Name(name);
-        // If name is obtained from subtree, fire name change event.
         // HTML file inputs always get part of their name from the subtree, even
         // if the author provided a name.
-        if (nameFlag == eNameFromSubtree || parent->IsHTMLFileInput()) {
+        bool fireNameChange = parent->IsHTMLFileInput();
+        if (!fireNameChange) {
+          nsAutoString name;
+          ENameValueFlag nameFlag = parent->Name(name);
+          switch (nameFlag) {
+            case eNameOK:
+              // Descendants of subtree may have been removed, making the name
+              // void.
+              fireNameChange = name.IsVoid();
+              break;
+            case eNameFromSubtree:
+              // If name is obtained from subtree, fire name change event.
+              fireNameChange = true;
+              break;
+            case eNameFromTooltip:
+              // If the descendants of this accessible were removed, the name
+              // may be calculated using the tooltip. We can guess that the name
+              // was obtained from the subtree before.
+              fireNameChange = true;
+              break;
+            default:
+              MOZ_ASSERT_UNREACHABLE("All name flags not covered!");
+          }
+        }
+
+        if (fireNameChange) {
           RefPtr<AccEvent> nameChangeEvent =
               new AccEvent(nsIAccessibleEvent::EVENT_NAME_CHANGE, parent);
           pushed |= PushEvent(nameChangeEvent);
@@ -93,21 +136,13 @@ bool EventQueue::PushNameOrDescriptionChange(AccEvent* aOrigEvent) {
         nameCheckAncestor = false;
       }
 
-      Relation rel = parent->RelationByType(RelationType::LABEL_FOR);
-      while (LocalAccessible* relTarget = rel.LocalNext()) {
-        RefPtr<AccEvent> nameChangeEvent =
-            new AccEvent(nsIAccessibleEvent::EVENT_NAME_CHANGE, relTarget);
-        pushed |= PushEvent(nameChangeEvent);
-      }
+      pushed |= PushNameOrDescriptionChangeToRelations(parent,
+                                                       RelationType::LABEL_FOR);
     }
 
     if (doDesc) {
-      Relation rel = parent->RelationByType(RelationType::DESCRIPTION_FOR);
-      while (LocalAccessible* relTarget = rel.LocalNext()) {
-        RefPtr<AccEvent> descChangeEvent = new AccEvent(
-            nsIAccessibleEvent::EVENT_DESCRIPTION_CHANGE, relTarget);
-        pushed |= PushEvent(descChangeEvent);
-      }
+      pushed |= PushNameOrDescriptionChangeToRelations(
+          parent, RelationType::DESCRIPTION_FOR);
     }
 
     if (parent->IsDoc()) {
diff --git a/accessible/base/EventQueue.h b/accessible/base/EventQueue.h
index 87bcf893406..f2878eaf818 100644
--- a/accessible/base/EventQueue.h
+++ b/accessible/base/EventQueue.h
@@ -29,6 +29,9 @@ class EventQueue {
    */
   bool PushEvent(AccEvent* aEvent);
 
+  bool PushNameOrDescriptionChangeToRelations(LocalAccessible* aAccessible,
+                                              RelationType aType);
+
   /**
    * Puts name and/or description change events into the queue, if needed.
    */
diff --git a/accessible/base/NotificationController.cpp b/accessible/base/NotificationController.cpp
index d2c37fb5ad0..cd88ceb9a9e 100644
--- a/accessible/base/NotificationController.cpp
+++ b/accessible/base/NotificationController.cpp
@@ -203,6 +203,20 @@ bool NotificationController::QueueMutationEvent(AccTreeMutationEvent* aEvent) {
     }
   }
 
+  if (aEvent->GetEventType() == nsIAccessibleEvent::EVENT_HIDE ||
+      aEvent->GetEventType() == nsIAccessibleEvent::EVENT_SHOW) {
+    LocalAccessible* target = aEvent->GetAccessible();
+    // We need to do this here while the relation is still intact. During the
+    // tick, where we we call PushNameOrDescriptionChange, it will be too late
+    // since we will already have unparented the label and severed the relation.
+    if (PushNameOrDescriptionChangeToRelations(target,
+                                               RelationType::LABEL_FOR) ||
+        PushNameOrDescriptionChangeToRelations(target,
+                                               RelationType::DESCRIPTION_FOR)) {
+      ScheduleProcessing();
+    }
+  }
+
   // We need to fire a reorder event after all of the events targeted at shown
   // or hidden children of a container.  So either queue a new one, or move an
   // existing one to the end of the queue if the container already has a
@@ -213,12 +227,6 @@ bool NotificationController::QueueMutationEvent(AccTreeMutationEvent* aEvent) {
     reorder = new AccReorderEvent(container);
     container->SetReorderEventTarget(true);
     mMutationMap.PutEvent(reorder);
-
-    // Since this is the first child of container that is changing, the name
-    // and/or description of dependent Accessibles may be changing.
-    if (PushNameOrDescriptionChange(aEvent)) {
-      ScheduleProcessing();
-    }
   } else {
     AccReorderEvent* event = downcast_accEvent(
         mMutationMap.GetEvent(container, EventMap::ReorderEvent));
@@ -651,6 +659,13 @@ void NotificationController::ProcessMutationEvents() {
         return;
       }
 
+      // The mutation in the container can change its name, or an ancestor's
+      // name. A labelled/described by relation would also need to be notified
+      // if this is the case.
+      if (PushNameOrDescriptionChange(event)) {
+        ScheduleProcessing();
+      }
+
       LocalAccessible* target = event->GetAccessible();
       target->Document()->MaybeNotifyOfValueChange(target);
       if (!mDocument) {
diff --git a/accessible/tests/mochitest/name/markup.js b/accessible/tests/mochitest/name/markup.js
index 4ad478482b5..afe70fe9151 100644
--- a/accessible/tests/mochitest/name/markup.js
+++ b/accessible/tests/mochitest/name/markup.js
@@ -342,7 +342,7 @@ function testNameForSubtreeRule(aElm) {
 
   if (gDumpToConsole) {
     dump(
-      "\nProcessed from subtree rule. Wait for reorder event on " +
+      "\nProcessed from subtree rule. Wait for name change event on " +
         prettyName(aElm) +
         "\n"
     );
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/accessible/tests/mochitest/name/markup.js b/accessible/tests/mochitest/name/markup.js
index 4ad478482b5..afe70fe9151 100644
--- a/accessible/tests/mochitest/name/markup.js
+++ b/accessible/tests/mochitest/name/markup.js
@@ -342,7 +342,7 @@ function testNameForSubtreeRule(aElm) {
 
   if (gDumpToConsole) {
     dump(
-      "\nProcessed from subtree rule. Wait for reorder event on " +
+      "\nProcessed from subtree rule. Wait for name change event on " +
         prettyName(aElm) +
         "\n"
     );
Loading diff…