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

Reland "Add security mitigations for eye dropper IPC."

This is a reland of c64eed06

The difference from the original change is that this CL updates the
color-picker-show-eye-dropper.html test to provide user activation.

Original change's description:
> 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/+/2442132
> Reviewed-by: Kent Tamura <tkent@chromium.org>
> Reviewed-by: Mason Freed <masonfreed@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Commit-Queue: Mason Freed <masonfreed@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#812847}

TBR=avi@chromium.org,tkent@chromium.org,masonfreed@chromium.org

Bug: 992297
Change-Id: Icecebf941b277790e12a12d06bca5b20da404ff1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2450731Reviewed-by: default avatarIonel Popescu <iopopesc@microsoft.com>
Reviewed-by: default avatarKent Tamura <tkent@chromium.org>
Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Reviewed-by: default avatarMason Freed <masonfreed@chromium.org>
Commit-Queue: Ionel Popescu <iopopesc@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#814008}
parent b4f6fb91
......@@ -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));
}
......
......@@ -632,6 +632,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);
}
......@@ -678,6 +681,9 @@ WebInputEventResult WebPagePopupImpl::HandleGestureEvent(
Cancel();
return WebInputEventResult::kNotHandled;
}
LocalFrame::NotifyUserActivation(
popup_client_->OwnerElement().GetDocument().GetFrame(),
mojom::blink::UserActivationNotificationType::kInteraction);
CheckScreenPointInOwnerWindowAndCount(
event.PositionInScreen(),
WebFeature::kPopupGestureTapExceedsOwnerWindowBounds);
......@@ -691,6 +697,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)));
......
......@@ -24,7 +24,10 @@ function openPickerSuccessfulCallback() {
const popupDocument = popupWindow.document;
waitUntilEyeDropperShown(() => testRunner.notifyDone());
const eyeDropper = popupDocument.querySelector('eye-dropper');
eyeDropper.click();
const eyeDropperRect = eyeDropper.getBoundingClientRect();
eventSender.mouseMoveTo(eyeDropperRect.left + (eyeDropperRect.width / 2), eyeDropperRect.top + (eyeDropperRect.height / 2));
eventSender.mouseDown();
eventSender.mouseUp();
}
</script>
</body>
......
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