Commit 10c65934 authored by Chris Sharp's avatar Chris Sharp Committed by Commit Bot

Revert "Add security mitigations for eye dropper IPC."

This reverts commit c64eed06.

Reason for revert: Broke virtual/eye-dropper/color-picker-show-eye-dropper.html on WebKit Linux MSAN

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,danakj@chromium.org,tkent@chromium.org,masonfreed@chromium.org,iopopesc@microsoft.com
NOTRY=true

Bug: 992297
Change-Id: If16db478fb59c4caa6f4fd90190adb72ce38e68a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2448054
Commit-Queue: Chris Sharp <csharp@chromium.org>
Reviewed-by: default avatarChris Sharp <csharp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813723}
parent 73c3cc8b
......@@ -121,10 +121,6 @@ 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() {
......@@ -250,10 +246,6 @@ 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,7 +8,6 @@
#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"
......@@ -79,7 +78,6 @@ 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,8 +5,6 @@
#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"
......@@ -20,19 +18,6 @@ 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,9 +632,6 @@ WebInputEventResult WebPagePopupImpl::HandleKeyEvent(
suppress_next_keypress_event_ = true;
}
}
LocalFrame::NotifyUserActivation(
popup_client_->OwnerElement().GetDocument().GetFrame(),
mojom::blink::UserActivationNotificationType::kInteraction);
return MainFrame().GetEventHandler().KeyEvent(event);
}
......@@ -681,9 +678,6 @@ WebInputEventResult WebPagePopupImpl::HandleGestureEvent(
Cancel();
return WebInputEventResult::kNotHandled;
}
LocalFrame::NotifyUserActivation(
popup_client_->OwnerElement().GetDocument().GetFrame(),
mojom::blink::UserActivationNotificationType::kInteraction);
CheckScreenPointInOwnerWindowAndCount(
event.PositionInScreen(),
WebFeature::kPopupGestureTapExceedsOwnerWindowBounds);
......@@ -697,9 +691,6 @@ 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,9 +276,6 @@ 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