[Backport] CVE-2024-11116: Inappropriate implementation in Paint

Manual backport of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/5894513:
Reapply "[PH] Disable cross origin paint holding if there was no user activation."

This reverts commit 011f2568aab9a77614d10ad913a18a825ea7d6bb.

Original patch description:
This patch disables paint holding if this is a cross origin navigation
and there was no user activation. This is a safety measure to prevent
sites from continually displaying mismatched URL and content.

With regular user behavior (clicks, etc), the behavior should be
unchanged since this counts as user activation.

Difference from original CL:
The difference is that we post a task to timeout paintholding, which allows embedding to differ and happen further down in the stack.

Details:
The surface eviction happens in this stack
content::DelegatedFrameHost::ResetFallbackToFirstNavigationSurface()
content::RenderWidgetHostImpl::ClearDisplayedGraphics()
content::RenderWidgetHostImpl::ForceFirstFrameAfterNavigationTimeout()
content::RenderFrameHostManager::CommitPendingIfNecessary()
content::RenderFrameHostManager::DidNavigateFrame()
content::Navigator::DidNavigate()

There is a bifurcation in ResetFallbackToFirstNavigationSurface to
decide whether to evict the delegated frame. This decision is based
on whether we have an first local surface id after navigation. On
non-Mac system, this local surface id is set in a stack similar to
the one below:

content::DelegatedFrameHost::EmbedSurface()
content::RenderWidgetHostViewAura::SynchronizeVisualProperties()
content::RenderWidgetHostViewAura::ShowWithVisibility()
content::RenderFrameHostManager::CommitPendingIfNecessary()
content::RenderFrameHostManager::DidNavigateFrame()
content::Navigator::DidNavigate()

Importantly, this happens _before_ we reset fallback, so in typical
cases we avoid eviction of the frame and simply reset its surface.

On Mac, the stack that sets the frame is below:
content::DelegatedFrameHost::EmbedSurface()
content::BrowserCompositorMac::DidNavigate()
content::RenderWidgetHostImpl::DidNavigate()
content::RenderFrameHostImpl::DidCommitNavigation()
...
content::mojom::NavigationClient_CommitNavigation_ForwardToCallback

This call happens _after_ we reset the fallback, so in typical cases
we evict the frame before embedding a new one. This is a cause for
a lot of test failures (and ultimately the reason for the revert).

Because the reset fallback path never happened synchronously with
DidNavigate, it isn't clear at this time whether this poses a problem
in non-test cases. Out of abundance of caution, I propose posting a
(non-delayed) task to remove paint holding. In practice this means
potentially having paintholding in place while the UI thread is busy.
This, however, is still a mitigation for the initial bug, albeit one
that does not have strict guarantees.

[email protected]
Bug: 40942531

Change-Id: Id45d1e2267147da2a6f4351cb95d3d8002d8f7ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5894513
Reviewed-by: Charlie Reis <[email protected]>
Commit-Queue: Vladimir Levin <[email protected]>
Reviewed-by: Nate Fischer <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1363640}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/604272
Reviewed-by: Michal Klocek <[email protected]>
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/607613
Reviewed-by: Anu Aliyas <[email protected]>

Author: vmpstr

chromium/content/browser/renderer_host/delegated_frame_host.cc

@@ -503,7 +503,10 @@ void DelegatedFrameHost::ContinueDelegatedFrameEviction(
   // preventing the FrameTree from being traversed. This could happen during
   // navigation involving BFCache. This should not occur with
   // features::kEvictSubtree.
-  DCHECK(!surface_ids.empty() ||
+  // We do allow the surface ids to be empty if we
+  // don't have a local surface id, since that means we don't have memory
+  // allocated in viz.
+  DCHECK(!surface_ids.empty() || !local_surface_id_.is_valid() ||
          !base::FeatureList::IsEnabled(features::kEvictSubtree));
   if (!surface_ids.empty()) {
     DCHECK(host_frame_sink_manager_);

chromium/content/browser/renderer_host/navigator.cc

@@ -32,6 +32,7 @@
 #include "content/browser/web_package/prefetched_signed_exchange_cache.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/browser/webui/web_ui_impl.h"
+#include "content/common/features.h"
 #include "content/common/navigation_params_utils.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
@@ -516,7 +517,38 @@ void Navigator::DidNavigate(
   // Store this information before DidNavigateFrame() potentially swaps RFHs.
   url::Origin old_frame_origin = old_frame_host->GetLastCommittedOrigin();
 
-  // Only allow paint holding for same-origin navigations.
+  // RenderFrameHostImpl::DidNavigate will update the url, and may cause the
+  // node to consider itself no longer on the initial empty document. Record
+  // whether we're leaving the initial empty document before that.
+  bool was_on_initial_empty_document =
+      frame_tree_node->is_on_initial_empty_document();
+
+  // Allow main frame paint holding in the following cases:
+  //  - We don't have an animated transition. See crbug.com/360844863.
+  //  - At least one of the following conditions is true:
+  //    - This is a navigation from the initial document. This part helps with
+  //      tests. See crbug.com/367623929.
+  //    - This is a same origin navigation (or we're not limiting cross-origin
+  //      paint holding)
+  //    - There is a user activation. This means that the user interacted with
+  //      the page. Commonly used attacks are done without user activation --
+  //      which will not enable paint holding. However, if the user interacts
+  //      with the page, we treat it as a valid case for paint holding.
+  //    - The client allows non-activated cross origin paintholding, which is
+  //      currently the case with webview.
+  //
+  // See https://issues.chromium.org/40942531 for reasons we limit paint
+  // holding.
+  ContentBrowserClient* client = GetContentClient()->browser();
+  const bool allow_main_frame_paint_holding =
+      (was_on_initial_empty_document ||
+       old_frame_origin.IsSameOriginWith(params.origin) ||
+       old_frame_host->HasStickyUserActivation() ||
+       client->AllowNonActivatedCrossOriginPaintHolding() ||
+       !base::FeatureList::IsEnabled(
+           kLimitCrossOriginNonActivatedPaintHolding));
+
+  // Only allow subframe paint holding for same origin.
   const bool allow_subframe_paint_holding =
       old_frame_origin.IsSameOriginWith(params.origin);
 
@@ -525,13 +557,16 @@ void Navigator::DidNavigate(
   // the frame we're navigating from, which might trigger those subframes to
   // run unload handlers.  Those unload handlers should still see the old
   // frame's origin.  See https://crbug.com/825283.
+  const bool allow_paint_holding = frame_tree_node->IsMainFrame()
+                                       ? allow_main_frame_paint_holding
+                                       : allow_subframe_paint_holding;
+
   frame_tree_node->render_manager()->DidNavigateFrame(
       render_frame_host, navigation_request->common_params().has_user_gesture,
       was_within_same_document,
       navigation_request->browsing_context_group_swap()
           .ShouldClearProxiesOnCommit(),
-      navigation_request->commit_params().frame_policy,
-      allow_subframe_paint_holding);
+      navigation_request->commit_params().frame_policy, allow_paint_holding);
 
   // The main frame, same site, and cross-site navigation checks for user
   // activation mirror the checks in DocumentLoader::CommitNavigation() (note:
@@ -598,12 +633,6 @@ void Navigator::DidNavigate(
     render_frame_host->GetPage().SetContentsMimeType(params.contents_mime_type);
   }
 
-  // RenderFrameHostImpl::DidNavigate will update the url, and may cause the
-  // node to consider itself no longer on the initial empty document. Record
-  // whether we're leaving the initial empty document before that.
-  bool was_on_initial_empty_document =
-      frame_tree_node->is_on_initial_empty_document();
-
   render_frame_host->DidNavigate(params, navigation_request.get(),
                                  was_within_same_document);
 

chromium/content/browser/renderer_host/render_frame_host_manager.cc

@@ -732,10 +732,10 @@ void RenderFrameHostManager::DidNavigateFrame(
     bool is_same_document_navigation,
     bool clear_proxies_on_commit,
     const blink::FramePolicy& frame_policy,
-    bool allow_subframe_paint_holding) {
+    bool allow_paint_holding) {
   CommitPendingIfNecessary(render_frame_host, was_caused_by_user_gesture,
                            is_same_document_navigation, clear_proxies_on_commit,
-                           allow_subframe_paint_holding);
+                           allow_paint_holding);
 
   // Make sure any dynamic changes to this frame's sandbox flags and permissions
   // policy that were made prior to navigation take effect.  This should only
@@ -772,7 +772,7 @@ void RenderFrameHostManager::CommitPendingIfNecessary(
     bool was_caused_by_user_gesture,
     bool is_same_document_navigation,
     bool clear_proxies_on_commit,
-    bool allow_subframe_paint_holding) {
+    bool allow_paint_holding) {
   if (!speculative_render_frame_host_) {
     // There's no speculative RenderFrameHost so it must be that the current
     // RenderFrameHost completed a navigation.
@@ -787,7 +787,7 @@ void RenderFrameHostManager::CommitPendingIfNecessary(
     // A cross-RenderFrameHost navigation completed, so show the new renderer.
     CommitPending(std::move(speculative_render_frame_host_),
                   std::move(stored_page_to_restore_), clear_proxies_on_commit,
-                  allow_subframe_paint_holding);
+                  allow_paint_holding);
 
     if (GetNavigationQueueingFeatureLevel() >=
         NavigationQueueingFeatureLevel::kAvoidRedundantCancellations) {
@@ -844,9 +844,26 @@ void RenderFrameHostManager::CommitPendingIfNecessary(
     // output on prerender activation.
     if (render_frame_host_->lifecycle_state() !=
         LifecycleStateImpl::kPrerendering) {
-      static_cast<RenderWidgetHostImpl*>(
-          render_frame_host_->GetView()->GetRenderWidgetHost())
-          ->StartNewContentRenderingTimeout();
+      auto* rwhi = static_cast<RenderWidgetHostImpl*>(
+          render_frame_host_->GetView()->GetRenderWidgetHost());
+
+      rwhi->StartNewContentRenderingTimeout();
+      // Force the timer to expire immediately if we don't allow main frame
+      // paint holding.
+      if (frame_tree_node_->IsMainFrame() && !allow_paint_holding) {
+        // We post task here, since this evicts a surface but the embedding of a
+        // new surface would be done in the same stack as this call. The
+        // ordering of whether the new surface has or has not yet been embedded
+        // differs for different platforms, and we always want the new surface
+        // to be embedded before we evict. Hence, we post a task. In practice
+        // this still disables paint holding unless this task is delayed for a
+        // long time.
+        GetUIThreadTaskRunner({})->PostTask(
+            FROM_HERE,
+            base::BindOnce(
+                &RenderWidgetHostImpl::ForceFirstFrameAfterNavigationTimeout,
+                rwhi->GetWeakPtr()));
+      }
     }
   }
 
@@ -1471,7 +1488,7 @@ void RenderFrameHostManager::PerformEarlyRenderFrameHostSwapIfNeeded(
   CommitPending(
       std::move(speculative_render_frame_host_), nullptr,
       request->browsing_context_group_swap().ShouldClearProxiesOnCommit(),
-      /* allow_subframe_paint_holding */ false);
+      /* allow_paint_holding */ false);
   request->SetAssociatedRFHType(
       NavigationRequest::AssociatedRenderFrameHostType::CURRENT);
 
@@ -4346,7 +4363,7 @@ void RenderFrameHostManager::CommitPending(
     std::unique_ptr<RenderFrameHostImpl> pending_rfh,
     std::unique_ptr<StoredPage> pending_stored_page,
     bool clear_proxies_on_commit,
-    bool allow_subframe_paint_holding) {
+    bool allow_paint_holding) {
   TRACE_EVENT1("navigation", "RenderFrameHostManager::CommitPending",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
   CHECK(pending_rfh);
@@ -4599,9 +4616,10 @@ void RenderFrameHostManager::CommitPending(
   // valid surface id, because it already has that surface embedded through
   // `RenderFrameHostImpl::WillLeaveBackForwardCache` and the timeout that
   // would be set here will clear that frame (incorrectly).
-  if (is_main_frame && old_view && old_view != new_view) {
-    // We should take the fallback if we're not coming from BFCache or if we
-    // don't have a valid surface id to display.
+  if (is_main_frame && allow_paint_holding && old_view && old_view != new_view) {
+    // If allowed, we should take the fallback in any of the following cases:
+    //  - We're not coming from BFCache
+    //  - We don't have a valid surface id to display.
     auto* render_widget_host_view_base =
         static_cast<RenderWidgetHostViewBase*>(render_frame_host_->GetView());
     should_take_fallback_content =
@@ -4736,7 +4754,7 @@ void RenderFrameHostManager::CommitPending(
   if (proxy_to_parent_or_outer_delegate) {
     proxy_to_parent_or_outer_delegate->SetChildRWHView(
         static_cast<RenderWidgetHostViewChildFrame*>(new_view),
-        old_size ? &*old_size : nullptr, allow_subframe_paint_holding);
+        old_size ? &*old_size : nullptr, allow_paint_holding);
   }
 
   if (render_frame_host_->is_local_root()) {
@@ -5145,7 +5163,7 @@ void RenderFrameHostManager::CreateNewFrameForInnerDelegateAttachIfNecessary() {
 
   CommitPending(std::move(speculative_render_frame_host_), nullptr,
                 false /* clear_proxies_on_commit */,
-                /* allow_subframe_paint_holding */ false);
+                /* allow_paint_holding */ false);
   NotifyPrepareForInnerDelegateAttachComplete(true /* success */);
 }
 

chromium/content/browser/renderer_host/render_frame_host_manager.h

@@ -323,7 +323,7 @@ class CONTENT_EXPORT RenderFrameHostManager {
                         bool is_same_document_navigation,
                         bool clear_proxies_on_commit,
                         const blink::FramePolicy& frame_policy,
-                        bool allow_subframe_paint_holding);
+                        bool allow_paint_holding);
 
   // Called when this frame's opener is changed to the frame specified by
   // |opener_frame_token| in |source_site_instance_group|'s process.  This
@@ -972,19 +972,18 @@ class CONTENT_EXPORT RenderFrameHostManager {
   // |clear_proxies_on_commit| Indicates if the proxies and opener must be
   // removed during the commit. This can happen following some BrowsingInstance
   // swaps, such as those for COOP.
-  // |allow_subframe_paint_holding| Indicates that paint holding is allowed if
-  // this is a subframe navigation.
+  // |allow_paint_holding| Indicates whether paint holding is allowed.
   void CommitPending(std::unique_ptr<RenderFrameHostImpl> pending_rfh,
                      std::unique_ptr<StoredPage> pending_stored_page,
                      bool clear_proxies_on_commit,
-                     bool allow_subframe_paint_holding);
+                     bool allow_paint_holding);
 
   // Helper to call CommitPending() in all necessary cases.
   void CommitPendingIfNecessary(RenderFrameHostImpl* render_frame_host,
                                 bool was_caused_by_user_gesture,
                                 bool is_same_document_navigation,
                                 bool clear_proxies_on_commit,
-                                bool allow_subframe_paint_holding);
+                                bool allow_paint_holding);
 
   // Runs the unload handler in the old RenderFrameHost, after the new
   // RenderFrameHost has committed.  |old_render_frame_host| will either be

chromium/content/common/features.cc

@@ -59,6 +59,11 @@ BASE_FEATURE(kWindowOpenFileSelectFix,
              "WindowOpenFileSelectFix",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
+// Flag guard for fix for crbug.com/40942531.
+BASE_FEATURE(kLimitCrossOriginNonActivatedPaintHolding,
+             "LimitCrossOriginNonActivatedPaintHolding",
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 // Please keep features in alphabetical order.
 
 }  // namespace content

chromium/content/common/features.h

@@ -72,6 +72,8 @@ CONTENT_EXPORT BASE_DECLARE_FEATURE(kSpeculativeServiceWorkerStartup);
 // Flag guard for fix for crbug.com/1414936.
 CONTENT_EXPORT BASE_DECLARE_FEATURE(kWindowOpenFileSelectFix);
 
+CONTENT_EXPORT BASE_DECLARE_FEATURE(kLimitCrossOriginNonActivatedPaintHolding);
+
 // Please keep features in alphabetical order.
 
 }  // namespace content

chromium/content/public/browser/content_browser_client.cc

@@ -1593,4 +1593,8 @@ bool ContentBrowserClient::
   return true;
 }
 
+bool ContentBrowserClient::AllowNonActivatedCrossOriginPaintHolding() {
+  return false;
+}
+
 }  // namespace content

chromium/content/public/browser/content_browser_client.h

@@ -2619,6 +2619,10 @@ class CONTENT_EXPORT ContentBrowserClient {
   // "Cache-control: no-store" header in BFCache.
   virtual bool ShouldAllowBackForwardCacheForCacheControlNoStorePage(
       content::BrowserContext* browser_context);
+
+  // Indicates whether this client allows paint holding in cross-origin
+  // navigations even if there was no user activation.
+  virtual bool AllowNonActivatedCrossOriginPaintHolding();
 };
 
 }  // namespace content