Commit c64eed06 authored by Ionel Popescu's avatar Ionel Popescu Committed by Commit Bot

Add security mitigations for eye dropper IPC.

As discussed on the security review this CL adds the following mitigations:
- require a transient user activation on the browser side, and consume
it when showing the eye dropper for the renderer (this will prevent a
compromised renderer to repeatedly ask for a color)
- require the eye dropper UI to be visible for a minimum amount of time
before color selection is allowed in order to ensure the user has a
chance to see the UI.

There is also a fix for the popup not correctly updating the user
activation state. This happens because it is using a
EmptyLocalFrameClient and its frame is not related to the
owner element's frame.

Bug: 992297
Change-Id: Ia5d2aead0be153ce4b49048552062de3a6c72e63
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2442132Reviewed-by: default avatarKent Tamura <tkent@chromium.org>
Reviewed-by: default avatarMason Freed <masonfreed@chromium.org>
Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812847}
parent 5b85e230
......@@ -121,6 +121,10 @@ EyeDropperView::EyeDropperView(content::RenderFrameHost* frame,
HideCursor();
pre_dispatch_handler_ = std::make_unique<PreEventDispatchHandler>(this);
widget->Show();
// The ignore selection time should be long enough to allow the user to see
// the UI.
ignore_selection_time_ =
base::TimeTicks::Now() + base::TimeDelta::FromMilliseconds(500);
}
EyeDropperView::~EyeDropperView() {
......@@ -246,6 +250,10 @@ void EyeDropperView::OnColorSelected() {
return;
}
// Prevent the user from selecting a color for a period of time.
if (base::TimeTicks::Now() <= ignore_selection_time_)
return;
// Use the last selected color and notify listener.
listener_->ColorSelected(selected_color_.value());
}
......@@ -8,6 +8,7 @@
#include <memory>
#include "base/optional.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "build/build_config.h"
#include "content/public/browser/eye_dropper.h"
......@@ -78,6 +79,7 @@ class EyeDropperView : public content::EyeDropper,
std::unique_ptr<ViewPositionHandler> view_position_handler_;
std::unique_ptr<ScreenCapturer> screen_capturer_;
base::Optional<SkColor> selected_color_;
base::TimeTicks ignore_selection_time_;
};
#endif // CHROME_BROWSER_UI_VIEWS_EYE_DROPPER_EYE_DROPPER_VIEW_H_
......@@ -5,6 +5,8 @@
#include "content/browser/eye_dropper_chooser_impl.h"
#include "base/callback.h"
#include "content/browser/renderer_host/frame_tree_node.h"
#include "content/browser/renderer_host/render_frame_host_impl.h"
#include "content/public/browser/eye_dropper.h"
#include "content/public/browser/eye_dropper_listener.h"
#include "content/public/browser/web_contents.h"
......@@ -18,6 +20,19 @@ void EyeDropperChooserImpl::Create(
RenderFrameHost* render_frame_host,
mojo::PendingReceiver<blink::mojom::EyeDropperChooser> receiver) {
DCHECK(render_frame_host);
// Renderer process should already check for user activation before sending
// this request. Double check in case of compromised renderer and consume
// the activation.
if (!static_cast<RenderFrameHostImpl*>(render_frame_host)
->frame_tree_node()
->UpdateUserActivationState(
blink::mojom::UserActivationUpdateType::
kConsumeTransientActivation,
blink::mojom::UserActivationNotificationType::kNone)) {
return;
}
new EyeDropperChooserImpl(render_frame_host, std::move(receiver));
}
......
......@@ -643,6 +643,9 @@ WebInputEventResult WebPagePopupImpl::HandleKeyEvent(
suppress_next_keypress_event_ = true;
}
}
LocalFrame::NotifyUserActivation(
popup_client_->OwnerElement().GetDocument().GetFrame(),
mojom::blink::UserActivationNotificationType::kInteraction);
return MainFrame().GetEventHandler().KeyEvent(event);
}
......@@ -689,6 +692,9 @@ WebInputEventResult WebPagePopupImpl::HandleGestureEvent(
Cancel();
return WebInputEventResult::kNotHandled;
}
LocalFrame::NotifyUserActivation(
popup_client_->OwnerElement().GetDocument().GetFrame(),
mojom::blink::UserActivationNotificationType::kInteraction);
CheckScreenPointInOwnerWindowAndCount(
event.PositionInScreen(),
WebFeature::kPopupGestureTapExceedsOwnerWindowBounds);
......@@ -702,6 +708,9 @@ void WebPagePopupImpl::HandleMouseDown(LocalFrame& main_frame,
const WebMouseEvent& event) {
if (IsViewportPointInWindow(event.PositionInWidget().x(),
event.PositionInWidget().y())) {
LocalFrame::NotifyUserActivation(
popup_client_->OwnerElement().GetDocument().GetFrame(),
mojom::blink::UserActivationNotificationType::kInteraction);
CheckScreenPointInOwnerWindowAndCount(
event.PositionInScreen(),
WebFeature::kPopupMouseDownExceedsOwnerWindowBounds);
......
......@@ -276,6 +276,9 @@ void ColorChooserPopupUIController::EyeDropperResponseHandler(bool success,
}
void ColorChooserPopupUIController::OpenEyeDropper() {
if (!LocalFrame::HasTransientUserActivation(frame_))
return;
frame_->GetBrowserInterfaceBroker().GetInterface(
eye_dropper_chooser_.BindNewPipeAndPassReceiver(
frame_->GetTaskRunner(TaskType::kUserInteraction)));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment