[Backport] CVE-2021-21204: Use after free in Blink.

Manual backport of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/2787572:
Fix Mac crash due to use after free of BlinkScrollbarPartAnimation

What is happening is that the BlinkScrollbarPartAnimation instance
passed to BlinkScrollbarPartAnimationTimer is released while
the BlinkScrollbarPartAnimationTimer::TimerFired method runs as
part of BlinkScrollbarPartAnimation::setCurrentProgress call,
during the execution of ScrollbarPainter::setKnobAlpha which ends
up calling BlinkScrollbarPainterDelegate::setUpAlphaAnimation
through a chain of observers.
BlinkScrollbarPainterDelegate::setUpAlphaAnimation releases the
BlinkScrollbarPartAnimation instance which gets deallocated.
BlinkScrollbarPartAnimation::setCurrentProgress continues execution
after ScrollbarPainter::setKnobAlpha returns, but the _scrollbar
pointer is overwritten with garbage and when SetNeedsPaintInvalidation
is called the crash happens.

I believe that BlinkScrollbarPartAnimationTimer::TimerFired should
retain the animation_ while it runs and release animation_ before
it exits. By retaining Objective C runtime won't free animation_
while BlinkScrollbarPartAnimationTimer is running and the crash
should be avoided.

Bug: 1183276, 1189926
Change-Id: Ibd5092a1dbae53bc21940c43883536624d1b03f3
Commit-Queue: Robert Flack <flackr@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867587}
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

1 files changed, 16 insertions, 1 deletions

diff --git a/chromium/third_party/blink/renderer/core/scroll/scroll_animator_mac.mm b/chromium/third_party/blink/renderer/core/scroll/scroll_animator_mac.mm
index 51d9ba90713..532dbcf5a3e 100644
--- a/chromium/third_party/blink/renderer/core/scroll/scroll_animator_mac.mm
+++ b/chromium/third_party/blink/renderer/core/scroll/scroll_animator_mac.mm
@@ -327,6 +327,11 @@ class BlinkScrollbarPartAnimationTimer {
     double fraction = delta / duration_;
     fraction = clampTo(fraction, 0.0, 1.0);
     double progress = timing_function_->Evaluate(fraction);
+    // In some scenarios, animation_ gets released during the call to
+    // setCurrentProgress. Because BlinkScrollbarPartAnimationTimer is a
+    // member variable of BlinkScrollbarPartAnimation animation_ the timer
+    // gets freed at the same time with animation_. In that case, it will
+    // not be safe to call any other code after animation_ setCurrentProgress.
     [animation_ setCurrentProgress:progress];
   }
 
@@ -401,6 +406,10 @@ class BlinkScrollbarPartAnimationTimer {
 
 - (void)setCurrentProgress:(NSAnimationProgress)progress {
   DCHECK(_scrollbar);
+  // In some scenarios, BlinkScrollbarPartAnimation is released in the middle
+  // of this method by _scrollbarPainter. This is why we have to retain the self
+  // pointer when we run this method.
+  [self retain];
 
   CGFloat currentValue;
   if (_startValue > _endValue)
@@ -427,7 +436,13 @@ class BlinkScrollbarPartAnimationTimer {
       break;
   }
 
-  _scrollbar->SetNeedsPaintInvalidation(invalidParts);
+  // Before BlinkScrollbarPartAnimation is released by _scrollbarPainter,
+  // invalidate is called and _scrollbar is set to nullptr. Check to see
+  // if _scrollbar is non-null before calling SetNeedsPaintInvalidation.
+  if (_scrollbar)
+    _scrollbar->SetNeedsPaintInvalidation(invalidParts);
+
+  [self release];
 }
 
 - (void)invalidate {