Validate untrusted VR mojo inputs into browser process

chrome/browser/android/vr_shell/vr_shell_gl.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/vr_shell/vr_shell_gl.h"
 
 #include <chrono>
 #include <limits>
 #include <utility>
 
@@ skipping 360 matching lines @@
                             const gpu::MailboxHolder& mailbox) {
   TRACE_EVENT0("gpu", "VrShellGl::SubmitWebVRFrame");
 
   // submit_client_ could be null when we exit presentation, if there were
   // pending SubmitFrame messages queued.  VRDisplayClient::OnExitPresent
   // will clean up state in blink, so it doesn't wait for
   // OnSubmitFrameTransferred or OnSubmitFrameRendered.
   if (!submit_client_.get())
     return;
 
+  if (frame_index < 0 ||
+      !webvr_frame_oustanding_[frame_index % kPoseRingBufferSize]) {
+    mojo::ReportBadMessage("SubmitFrame called with an invalid frame_index");
+    binding_.Close();
+    return;
+  }
+
   webvr_time_js_submit_[frame_index % kPoseRingBufferSize] =
       base::TimeTicks::Now();
 
   // Swapping twice on a Surface without calling updateTexImage in
   // between can lose frames, so don't draw+swap if we already have
   // a pending frame we haven't consumed yet.
   bool swapped = false;
   if (pending_frames_.empty()) {
     swapped = mailbox_bridge_->CopyMailboxToSurfaceAndSwap(mailbox);
     if (swapped) {
@@ skipping 57 matching lines @@
     browser_->ToggleCardboardGamepad(true);
   }
 }
 
 void VrShellGl::InitializeRenderer() {
   gvr_api_->InitializeGl();
   gfx::Transform head_pose;
   device::GvrDelegate::GetGvrPoseWithNeckModel(gvr_api_.get(), &head_pose);
   webvr_head_pose_.assign(kPoseRingBufferSize, head_pose);
   webvr_time_pose_.assign(kPoseRingBufferSize, base::TimeTicks());
+  webvr_frame_oustanding_.assign(kPoseRingBufferSize, false);
   webvr_time_js_submit_.assign(kPoseRingBufferSize, base::TimeTicks());
 
   std::vector<gvr::BufferSpec> specs;
   // For kFramePrimaryBuffer (primary VrShell and WebVR content)
   specs.push_back(gvr_api_->CreateBufferSpec());
   gvr::Sizei render_size_primary = specs[kFramePrimaryBuffer].GetSize();
   render_size_primary_ = {render_size_primary.width,
                           render_size_primary.height};
   render_size_vrshell_ = render_size_primary_;
 
@@ skipping 592 matching lines @@
 
   // When using async reprojection, we need to know which pose was
   // used in the WebVR app for drawing this frame and supply it when
   // submitting. Technically we don't need a pose if not reprojecting,
   // but keeping it uninitialized seems likely to cause problems down
   // the road. Copying it is cheaper than fetching a new one.
   if (ShouldDrawWebVr()) {
     static_assert(!((kPoseRingBufferSize - 1) & kPoseRingBufferSize),
                   "kPoseRingBufferSize must be a power of 2");
     head_pose = webvr_head_pose_[frame_index % kPoseRingBufferSize];
+    webvr_frame_oustanding_[frame_index % kPoseRingBufferSize] = false;
   } else {
     device::GvrDelegate::GetGvrPoseWithNeckModel(gvr_api_.get(), &head_pose);
   }
 
   // Update the render position of all UI elements (including desktop).
   scene_->OnBeginFrame(current_time);
 
   {
     // TODO(crbug.com/704690): Acquire controller state in a way that's timely
     // for both the gamepad API and UI input handling.
@@ skipping 488 matching lines @@
   vsync_timebase_ += base::TimeDelta::FromMicroseconds(timebase_nanos / 1000);
   vsync_interval_ = base::TimeDelta::FromSecondsD(interval_seconds);
   vsync_task_.Reset(base::Bind(&VrShellGl::OnVSync, base::Unretained(this)));
   OnVSync();
 }
 
 void VrShellGl::ForceExitVr() {
   browser_->ForceExitVr();
 }
 
+namespace {
+bool ValidateRect(const gfx::RectF& bounds) {
+  // Bounds should be between 0 and 1, with positive width/height
+  constexpr gfx::SizeF max_size(1, 1);
+  constexpr gfx::SizeF min_size(0, 0);
+
+  return bounds.x() < bounds.right() && bounds.y() < bounds.bottom() &&

[mthiesse, 2017/06/22 05:59:47]

You can hugely simplify this. I think the set of checks you need is:
bounds.width() >= 0 && bounds.height() >= 0 && bounds.x() >= 0 && bounds.y() >=
0 && bounds.right() <= 1 && bounds.bottom() <= 1

[billorr, 2017/07/10 18:59:23]

Done.

+         bounds.x() <= max_size.width() && bounds.x() >= min_size.width() &&
+         bounds.right() <= max_size.width() &&
+         bounds.right() >= min_size.width() &&
+         bounds.y() <= max_size.height() && bounds.y() >= min_size.height() &&
+         bounds.bottom() <= max_size.height() &&
+         bounds.bottom() >= min_size.height();
+}
+
+bool ValidateSize(const gfx::Size& size) {
+  // Size should be between at least 2 in each direction and less than 5000.

[billorr, 2017/06/22 00:28:34]

arbitrary values - what should we use?  Or are we ok with any size?

[mthiesse, 2017/06/22 05:59:46]

Not sure. The only problem I can see is intentionally triggering out of memory
situations? Which could happen anyways at any point really. I'm not sure how
Chrome deals with that problem in general.

These values are probably okay... but we should get Brandon/Klaus to review.

+  constexpr gfx::Size max_size(5000, 5000);
+  constexpr gfx::Size min_size(2, 2);
+
+  return size.width() <= max_size.width() &&
+         size.height() <= max_size.height() &&
+         size.width() >= min_size.width() && size.height() >= min_size.height();
+}
+}  // namespace
+
 void VrShellGl::UpdateLayerBounds(int16_t frame_index,
                                   const gfx::RectF& left_bounds,
                                   const gfx::RectF& right_bounds,
                                   const gfx::Size& source_size) {
+  if (!ValidateRect(left_bounds) || !ValidateRect(right_bounds) ||
+      !ValidateSize(source_size)) {
+    mojo::ReportBadMessage("UpdateLayerBounds called with invalid bounds");

[mthiesse, 2017/06/22 05:59:46]

So I don't think we want to ReportBadMessage in these cases. That crashes the
renderer, which feels too extreme. If we're doing this, the renderer should
perform the same validation, and report a JS exception without crashing. The
validation here would be to validate in the case of a compromised renderer
bypassing the checks, so crashing a compromised or buggy renderer sounds
reasonable.

It would just suck if a rounding error in floating point math made the layer
bound 1.000000001 and we crashed. Maybe we should just clamp to valid values
rather than reporting errors, as clamping seems pretty harmless here.

+    binding_.Close();
+    return;
+  }
+
+  if (frame_index >= 0 &&
+      !webvr_frame_oustanding_[frame_index % kPoseRingBufferSize]) {
+    mojo::ReportBadMessage("UpdateLayerBounds called with invalid frame_index");
+    binding_.Close();
+    return;
+  }
+
   if (frame_index < 0) {
     webvr_left_viewport_->SetSourceUv(UVFromGfxRect(left_bounds));
     webvr_right_viewport_->SetSourceUv(UVFromGfxRect(right_bounds));
     CreateOrResizeWebVRSurface(source_size);
   } else {
     pending_bounds_.emplace(
         frame_index, WebVrBounds(left_bounds, right_bounds, source_size));
   }
 }
 
@@ skipping 20 matching lines @@
   TRACE_EVENT1("input", "VrShellGl::SendVSync", "frame", frame_index);
 
   int64_t prediction_nanos = GetPredictedFrameTimeNanos();
 
   gfx::Transform head_mat;
   device::mojom::VRPosePtr pose =
       device::GvrDelegate::GetVRPosePtrWithNeckModel(gvr_api_.get(), &head_mat,
                                                      prediction_nanos);
 
   webvr_head_pose_[frame_index % kPoseRingBufferSize] = head_mat;
+  webvr_frame_oustanding_[frame_index % kPoseRingBufferSize] = true;
   webvr_time_pose_[frame_index % kPoseRingBufferSize] = base::TimeTicks::Now();
 
   std::move(callback).Run(
       std::move(pose), time, frame_index,
       device::mojom::VRPresentationProvider::VSyncStatus::SUCCESS);
 }
 
 void VrShellGl::CreateVRDisplayInfo(
     const base::Callback<void(device::mojom::VRDisplayInfoPtr)>& callback,
     uint32_t device_id) {
   // This assumes that the initial webvr_surface_size_ was set to the
   // appropriate recommended render resolution as the default size during
   // InitializeGl. Revisit if the initialization order changes.
   device::mojom::VRDisplayInfoPtr info =
       device::GvrDelegate::CreateVRDisplayInfo(gvr_api_.get(),
                                                webvr_surface_size_, device_id);
   browser_->RunVRDisplayInfoCallback(callback, &info);
 }
 
 }  // namespace vr_shell