From 9b94ebcc1f9c9d62ec56b80fbd0968dbb97bd15d Mon Sep 17 00:00:00 2001 From: Liviu Tinta Date: Tue, 30 Mar 2021 13:44:40 +0000 Subject: [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 Reviewed-by: Robert Flack Cr-Commit-Position: refs/heads/master@{#867587} Reviewed-by: Allan Sandfeld Jensen --- .../blink/renderer/core/scroll/scroll_animator_mac.mm | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) 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 { -- cgit v1.2.3