New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 759225 link

Starred by 1 user
Status: Fixed
Owner:
dtapu...@chromium.org
Closed: Jan 2018
Cc:
█ aelias@chromium.org
dtapu...@chromium.org
tdres...@chromium.org
lanwei@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug-Security



Sign in to add a comment

CHECK failure in SyntheticGestureTargetBase::DispatchInputEventToPlatform()

Project Member Reported by trchen@chromium.org, Aug 25 2017 
This is a spin-off bug from  issue 610021 /631015/756613.

The CHECK failure happens during a family of telemetry tests. These tests internally inject pinch events in the center of <body> element. When the page zooms in, the center point may no longer be inside of the view bounds, thus triggering the CHECK.

I uploaded a CL to fix those tests (reviewing, expected to land soon) so they no longer inject invalid points, but I think this needs further investigation.

The test inject input events via chrome.gpuBenchmarking.pinchBy, which is a native method implemented by GpuBenchmarking::PinchBy(). This means although under normal circumstances the invalid values can only come from internal code, the values come from the renderer process, which is the other side of security boundary. A compromised renderer process shouldn't be able to crash the browser process.

The involved call stack looks like this:

chrome.gpuBenchmarking.pinchBy
GpuBenchmarking::PinchBy
RenderWidget::QueueSyntheticGesture
(IPC)InputHostMsg_QueueSyntheticGesture
RenderWidgetHostImpl::OnQueueSyntheticGesture
RenderWidgetHostImpl::QueueSyntheticGesture
SyntheticGestureController::QueueSyntheticGesture
SyntheticGestureController::StartGesture
SyntheticGestureController::StartTimer
(Timer)
SyntheticGestureController::DispatchNextEvent
SyntheticGesture::ForwardInputEvents
SyntheticTouchscreenPinchGesture::ForwardInputEvents
SyntheticTouchscreenPinchGesture::ForwardTouchInputEvents
SyntheticTouchDriver::DispatchEvent
SyntheticGestureTargetBase::DispatchInputEventToPlatform

I think we should make the contract clear whether the synthetic event subsystem should accept invalid inputs. If yes, it should handle the failure gracefully. If not, then RenderWidgetHostImpl should sanitize inputs, or even kill the misbehaving renderer process.

Comment 1 by tdres...@chromium.org, Aug 28 2017

Cc: dtapu...@chromium.org
Labels: Hotlist-Input-Dev
cc'ing dtapuska@.

With Lan out, is there a clear owner for this?

Synthetic input is only enabled in contexts where we're trusting the renderer process, so I'm not sure how big an issue this is. I agree that clarifying the contract makes sense though.

Comment 2 by dtapu...@chromium.org, Aug 28 2017

Labels: -Pri-2 Pri-3
Owner: dtapu...@chromium.org
Status: Assigned (was: Untriaged)
I agree we should gracefully handle bad input. The tests should be corrected though because once we fix the crash they are likely to be flaky if they cause different behavior inconsistently.

Comment 3 by ta...@google.com, Aug 28 2017

Labels: Security_Severity-Low Security_Impact-Head OS-All

Comment 4 by trchen@chromium.org, Aug 28 2017 

Just double checked. Yes RenderWidgetHostImpl::OnQueueSyntheticGesture handles the message only if --enable-gpu-benchmarking is specified. So not a security to end users after all.

Comment 5 by dtapu...@chromium.org, Jan 3 2018

Status: Fixed (was: Assigned)
Project Member

Comment 6 by sheriffbot@chromium.org, Jan 4 2018

Labels: Restrict-View-SecurityNotify
Project Member

Comment 7 by sheriffbot@chromium.org, Apr 12 2018

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
►

Sign in to add a comment