Project Fortify, an experimental security project, has identified the following potential security issue. If you’re a feature owner CC-ed on this bug, please do your best to review these reports without the Chrome Security team.

Overview: A Use-After-Free (UAF) vulnerability exists in the browser process due to synchronous object deletion during MenuController::MenuChildrenChanged. Accessibility events triggered during a SetSelection call can synchronously destroy the MenuController, but the caller unconditionally accesses the freed this pointer afterward. MiraclePtr does not protect against this because the dangling references are an implicit this pointer and a local raw pointer.

Affected files:

• ui/views/controls/menu/menu_controller.cc
• ui/views/controls/menu/menu_item_view.cc
• ui/views/controls/menu/menu_runner_impl.cc

Estimated timestamp from git blame: 2024-01-29

Description

A potential Use-After-Free (UAF) vulnerability in the browser process has been identified in MenuController::MenuChildrenChanged. This function updates the menu state when child items are added or removed (e.g., due to a renderer-timed model mutation).

The vulnerability is triggered because MenuController::MenuChildrenChanged calls SetSelection(item, SELECTION_OPEN_SUBMENU | SELECTION_UPDATE_IMMEDIATELY). Within SetSelection, interactions with the accessibility system (specifically menu_item->GetViewAccessibility().SetPopupFocusOverride()) can trigger synchronous accessibility events if platform accessibility is enabled. On platforms like Linux/Aura, these events can cause the active window to lose activation, leading MenuPreTargetHandlerAura to call MenuController::Cancel(ExitType::kAll). This cancellation synchronously deletes the MenuController via MenuRunnerImpl::OnMenuClosed.

While SetSelection anticipates this risk and guards against its own deletion using a base::WeakPtr (if (!this_ref) return;), the caller MenuChildrenChanged lacks any such guard. Once SetSelection safely returns, MenuChildrenChanged unconditionally dereferences the freed this pointer by calling OpenMenuImpl(item, false).

Furthermore, the function that called MenuChildrenChanged, which is MenuItemView::ChildrenChanged(), also saves the controller as a raw local pointer (MenuController* controller = GetMenuController();) and subsequently calls controller->UpdateSubmenuSelection(submenu_.get());, creating a second UAF site on the same freed object.

MiraclePtr (BackupRefPtr) does not protect against either UAF because the dangling references are not raw_ptr<T> members; they are an implicit this pointer held in a register and a local raw pointer. Consequently, the memory is immediately returned to PartitionAlloc.

Potential Exploitation Path

While we do not have a working Proof of Concept, an attacker could potentially exploit this by:

1. Gaining code execution in a Renderer process.
2. Waiting for the user to open a dynamic menu (e.g., the History -> Recent Tabs submenu).
3. Triggering a model mutation from the renderer (e.g., by executing window.close() on a popup associated with that menu).
4. Timing heap spraying (via IPCs or Blob URLs) to reclaim the freed MenuController memory slot immediately after MenuRunnerImpl::OnMenuClosed executes, but before OpenMenuImpl accesses it.
5. Overwriting the internal std::vector (child_menu_open_direction_) accessed by OpenMenuImpl to achieve arbitrary read/write primitives in the browser process.

Suggested Reproduction Steps

1. Run Chrome with platform accessibility enabled (e.g., by attaching a screen reader or using --force-renderer-accessibility).
2. Open a same-origin popup from a tab using window.open().
3. Open the Chrome App Menu and navigate to the History -> Recent Tabs submenu.
4. From the renderer of the popup, execute window.close().
5. The resulting state change should trigger MenuChildrenChanged while the menu is open, potentially crashing the browser if the timing coincides with the accessibility event dispatch.

Suggested Fix

1. In ui/views/controls/menu/menu_controller.cc, update MenuChildrenChanged to use a WeakPtr to verify this is still valid after calling SetSelection:

  auto this_ref = AsWeakPtr();
  SetSelection(item, SELECTION_OPEN_SUBMENU | SELECTION_UPDATE_IMMEDIATELY);
  if (!this_ref) return;
  if (item->HasSubmenu()) {
    OpenMenuImpl(item, false);
  }

2. In ui/views/controls/menu/menu_item_view.cc, update MenuItemView::ChildrenChanged() to hold a WeakPtr instead of a raw pointer:

  base::WeakPtr<MenuController> controller = GetMenuController()->AsWeakPtr();
  if (controller) {
    UpdateEmptyMenusAndMetrics();
    controller->MenuChildrenChanged(this);
    if (!controller) return;
    if (submenu_) {
      submenu_->DeprecatedLayoutImmediately();
      submenu_->SchedulePaint();
      controller->UpdateSubmenuSelection(submenu_.get());
    }
  }

Evaluated with Chrome root at commit: 096fc8fdbfacf2546485756d03f160a3d04fcc9b

Results so far have been promising, but there can be wrong deductions. If this proves to be a false positive, please close as WAI; data from false positives will be used to improve accuracy over time. And please feel free to reach out to me directly if you have concerns or feedback on the project.