Skip to content

Commit

Permalink
[Backport] Security bug 1485266
Browse files Browse the repository at this point in the history 
Manual cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/4902775:
Drag and drop: prevent cross-origin same-tab drags that span navigations

In IsValidDragTarget, the old RenderViewHostID comparison was not
necessary to distinguish between same- and different-tab drags because,
contrary to the previous comment, that case is covered by the
`drag_start_` check. This check was only serving to permit some drags
which were same-tab, but not same-RVH, which should be disallowed.

A complete rundown of the business logic and the reason for the
business logic is here:
https://bugs.chromium.org/p/chromium/issues/detail?id=1266953#c22

A regression test is added which is confirmed to fail without this fix,
but only on Chrome OS because that's the only Aura platform where the
DND interactive UI tests are not already disabled (Windows and Linux
were disabled).

Bug: 1485266
Change-Id: Ifdd6eec14df42372b0afc8ccba779a948cbaaaa7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4902775
Commit-Queue: Evan Stade <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Reviewed-by: Charlie Reis <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1204930}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/523704
Reviewed-by: Michal Klocek <[email protected]>
  • Loading branch information
@mibrunin
Evan Stade authored and mibrunin committed Dec 18, 2023
1 parent f3f1c75 commit 400d873
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 50 deletions.
44 changes: 13 additions & 31 deletions chromium/content/browser/web_contents/web_contents_view_aura.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -763,13 +763,10 @@ void WebContentsViewAura::PrepareDropData(
// Do not add FileContents if this is a tainted-cross-origin same-page image
// (https://crbug.com/1264873).
bool access_allowed =
// Drag started outside blink.
!drag_start_ ||
// Drag began in blink, but image access is allowed.
drag_start_->image_accessible_from_frame ||
// Drag began in blink, but in a different WebContents.
GetRenderViewHostID(web_contents_->GetRenderViewHost()) !=
drag_start_->view_id;
// Drag began in this top-level WebContents, and image access is allowed
// (not cross-origin).
drag_start_->image_accessible_from_frame;
data.GetFilenames(&drop_data->filenames);
if (access_allowed && drop_data->filenames.empty()) {
base::FilePath filename;
Expand Down Expand Up @@ -885,42 +882,28 @@ bool WebContentsViewAura::IsValidDragTarget(
// drags between cross-origin frames within the same page. Otherwise, a
// malicious attacker could abuse drag interactions to leak information
// across origins without explicit user intent.
// `drag_start_` is null when the drag started outside of the browser or from
// a different top-level WebContents.
if (!drag_start_)
return true;

// For site isolation, it is desirable to avoid having the renderer
// perform the check unless it already has access to the starting
// document's origin. If the SiteInstanceGroups match, then the process
// allocation policy decided that it is OK for the source and target
// frames to live in the same renderer process. Furthermore, it means that
// either the source and target frame are part of the same `blink::Page` or
// that there is an opener relationship and would cross tab boundaries. Allow
// this drag to the renderer. Blink will perform an additional check against
// frames to live in the same renderer process. Furthermore, having matching
// SiteInstanceGroups means that either (1) the source and target frame are
// part of the same blink::Page, or (2) that they are in the same Browsing
// Context Group and the drag would cross tab boundaries (the latter of which
// can't happen here since `drag_start_` is null). Allow this drag to the
// renderer. Blink will perform an additional check against
// `blink::DragController::drag_initiator_` to decide whether or not to
// allow the drag operation. This can be done in the renderer, as the
// browser-side checks only have local tree fragment (potentially with
// multiple origins) granularity at best, but a drag operation eventually
// targets one single frame in that local tree fragment.
bool same_site_instance_group = target_rwh->GetSiteInstanceGroup()->GetId() ==
drag_start_->site_instance_group_id;
if (same_site_instance_group)
return true;

// Otherwise, if the SiteInstanceGroups do not match, enforce explicit
// user intent by ensuring this drag operation is crossing page boundaries.
// `drag_start_->view_id` is set to the main `RenderFrameHost`'s
// `RenderViewHost`'s ID when a drag starts, so if the two IDs match here,
// the drag is within the same page and disallowed.
//
// Drags between an embedder and an inner `WebContents` will disallowed by
// the above view ID check because `WebContentsViewAura` is always created
// for the outermost view. Inner `WebContents` will have a
// `WebContentsViewChildFrame` so when dragging between an inner
// `WebContents` and its embedder the view IDs will be the same.
bool cross_tab_drag =
GetRenderViewHostID(web_contents_->GetRenderViewHost()) !=
drag_start_->view_id;
return cross_tab_drag;
return target_rwh->GetSiteInstanceGroup()->GetId() ==
drag_start_->site_instance_group_id;
}

////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -1175,7 +1158,6 @@ void WebContentsViewAura::StartDragging(

drag_start_ =
DragStart(source_rwh->GetSiteInstanceGroup()->GetId(),
GetRenderViewHostID(web_contents_->GetRenderViewHost()),
drop_data.file_contents_image_accessible);

ui::TouchSelectionController* selection_controller = GetSelectionController();
Expand Down
26 changes: 7 additions & 19 deletions chromium/content/browser/web_contents/web_contents_view_aura.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class CONTENT_EXPORT WebContentsViewAura

// Returns whether |target_rwh| is a valid RenderWidgetHost to be dragging
// over. This enforces that same-page, cross-site drags are not allowed. See
// crbug.com/666858.
// crbug.com/666858, crbug.com/1266953, crbug.com/1485266.
bool IsValidDragTarget(RenderWidgetHostImpl* target_rwh) const;

// Called from CreateView() to create |window_|.
Expand Down Expand Up @@ -342,7 +342,7 @@ class CONTENT_EXPORT WebContentsViewAura
std::unique_ptr<WindowObserver> window_observer_;

// The WebContentsImpl whose contents we display.
raw_ptr<WebContentsImpl> web_contents_;
const raw_ptr<WebContentsImpl> web_contents_;

std::unique_ptr<WebContentsViewDelegate> delegate_;

Expand All @@ -360,33 +360,21 @@ class CONTENT_EXPORT WebContentsViewAura
// avoid sending the drag exited message after leaving the current view.
GlobalRoutingID current_rvh_for_drag_;

// We track the IDs of the source RenderProcessHost and RenderViewHost from
// which the current drag originated. These are used to ensure that drag
// events do not fire over a cross-site frame (with respect to the source
// frame) in the same page (see crbug.com/666858). Specifically, the
// RenderViewHost is used to check the "same page" property, while the
// RenderProcessHost is used to check the "cross-site" property. Note that the
// reason the RenderProcessHost is tracked instead of the RenderWidgetHost is
// so that we still allow drags between non-contiguous same-site frames (such
// frames will have the same process, but different widgets). Note also that
// the RenderViewHost may not be in the same process as the RenderProcessHost,
// since the view corresponds to the page, while the process is specific to
// the frame from which the drag started.
// We also track whether a dragged image is accessible from its frame, so we
// can disallow tainted-cross-origin same-page drag-drop.
// Used to track security-salient details about a drag source. See
// documentation in `IsValidDragTarget()` for `site_instance_group_id`.
// See crbug.com/1264873 for `image_accessible_from_frame`.
struct DragStart {
DragStart(SiteInstanceGroupId site_instance_group_id,
GlobalRoutingID view_id,
bool image_accessible_from_frame)
: site_instance_group_id(site_instance_group_id),
view_id(view_id),
image_accessible_from_frame(image_accessible_from_frame) {}
~DragStart() = default;

SiteInstanceGroupId site_instance_group_id;
GlobalRoutingID view_id;
bool image_accessible_from_frame;
};
// Will hold a value when the current drag started in this page (outermost
// WebContents).
absl::optional<DragStart> drag_start_;

// Responsible for handling gesture-nav and pull-to-refresh UI.
Expand Down

0 comments on commit 400d873

Please sign in to comment.