Firing a slider event can detach the current AXObject.

In response to an accessibility action to change the value of a slider, we fire a DOM event. However, firing that DOM event can result in arbitrary user code being called, which can mean detaching the current AXObject, leading to a crash/UAF. Fix this by checking after firing a DOM event. Bug: 1079445 Change-Id: Ic16b9a5312a14e57bc56a9c8124ffe64d1b69f65 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2211930 Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org> Reviewed-by: Meredith Lane <meredithl@chromium.org> Cr-Commit-Position: refs/heads/master@{#772443}

Firing a slider event can detach the current AXObject.
In response to an accessibility action to change the value of a slider, we fire a DOM event. However, firing that DOM event can result in arbitrary user code being called, which can mean detaching the current AXObject, leading to a crash/UAF. Fix this by checking after firing a DOM event. Bug: 1079445 Change-Id: Ic16b9a5312a14e57bc56a9c8124ffe64d1b69f65 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2211930 Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org> Reviewed-by: Meredith Lane <meredithl@chromium.org> Cr-Commit-Position: refs/heads/master@{#772443}
c2b2115c · Dominic Mazzoni · Commit Bot · 853f91d6 · c2b2115c · c2b2115c
Commit c2b2115c authored May 27, 2020 by Dominic Mazzoni Committed by Commit Bot May 27, 2020
3 changed files
--- a/third_party/blink/renderer/modules/accessibility/ax_node_object.cc
+++ b/third_party/blink/renderer/modules/accessibility/ax_node_object.cc
@@ -145,6 +145,12 @@ void AXNodeObject::AlterSliderOrSpinButtonValue(bool increase) {
  value += increase ? step : -step;

  OnNativeSetValueAction(String::Number(value));
+
+  // Dispatching an event could result in changes to the document, like
+  // this AXObject becoming detached.
+  if (IsDetached())
+    return;
+
  AXObjectCache().PostNotification(GetNode(),
                                   ax::mojom::blink::Event::kValueChanged);
 }

--- a/third_party/blink/renderer/modules/accessibility/ax_slider.cc
+++ b/third_party/blink/renderer/modules/accessibility/ax_slider.cc
@@ -114,6 +114,11 @@ bool AXSlider::OnNativeSetValueAction(const String& value) {
  // Fire change event manually, as LayoutSlider::setValueForPosition does.
  input->DispatchFormControlChangeEvent();

+  // Dispatching an event could result in changes to the document, like
+  // this AXObject becoming detached.
+  if (IsDetached())
+    return false;
+
  // Ensure the AX node is updated.
  AXObjectCache().MarkAXObjectDirty(this, false);


--- a/third_party/blink/web_tests/accessibility/slider-crash.html
+++ b/third_party/blink/web_tests/accessibility/slider-crash.html
+<!DOCTYPE HTML>
+<script src="../resources/gc.js"></script>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+
+<input min=0 max=100 value=50 onchange=update(this) type=range id=slider>
+
+<script>
+function update(slider) {
+  slider.parentElement.removeChild(slider);
+}
+test(function(t) {
+  accessibilityController.accessibleElementById('slider').decrement();
+}, "Decrementing a slider that's destroyed doesn't crash.");
+</script>