Validate untrusted VR mojo inputs into browser process

chrome/browser/android/vr_shell/vr_shell_gl.cc

Index: chrome/browser/android/vr_shell/vr_shell_gl.cc
diff --git a/chrome/browser/android/vr_shell/vr_shell_gl.cc b/chrome/browser/android/vr_shell/vr_shell_gl.cc
index aa9613251d39a6d36704f37d039ecf75d0de052f..008f0cbd6652580e515a59371e3af808601a82ef 100644
--- a/chrome/browser/android/vr_shell/vr_shell_gl.cc
+++ b/chrome/browser/android/vr_shell/vr_shell_gl.cc
@@ -378,6 +378,13 @@ void VrShellGl::SubmitFrame(int16_t frame_index,
   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();
 
@@ -455,6 +462,7 @@ void VrShellGl::InitializeRenderer() {
   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;
@@ -1067,6 +1075,7 @@ void VrShellGl::DrawFrame(int16_t frame_index) {
     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);
   }
@@ -1577,14 +1586,54 @@ void VrShellGl::ForceExitVr() {
   browser_->ForceExitVr();
 }
 
+namespace {
+bool ValidateRect(const gfx::RectF& bounds) {
+  // Bounds should be between 0 and 1, with positive width/height.
+  // We simply clamp to [0,1], but still validate that the bounds are not NAN.
+  return !std::isnan(bounds.width()) && !std::isnan(bounds.height()) &&
+         !std::isnan(bounds.x()) && !std::isnan(bounds.y());
+}
+
+bool ValidateSize(const gfx::Size& size) {
+  // Size should be at least 0 in each direction.
+  return size.width() >= 0 && size.height() >= 0;
+}
+
+gfx::RectF ClampRect(const gfx::RectF& bounds) {
+  float left = std::max(0.0f, std::min(bounds.x(), 1.0f));

[mthiesse, 2017/07/11 14:52:24]

I think you can just use bounds.AdjustToFit(gfx::RectF(0, 0, 1, 1));?

[billorr, 2017/07/13 00:31:40]

That is close enough to the same behavior that I'll use it - same behavior for
well-behaved sites.  If a caller passes in something that doesn't intersect with
(0,0,1,1), the current code would make this a zero-sized rect, and AdjustToFit
will return a rect intersecting (0,0,1,1), with width/height a minimum of of the
current size or 1.  For example, (-2, -2, 0.5, 0.5).AdjustToFit(0,0,1,1) will
return (0,0,0.5,0.5), which doesn't even intersect with our initial rect.

+  float right = std::max(left, std::min(bounds.right(), 1.0f));
+  float top = std::max(0.0f, std::min(bounds.y(), 1.0f));
+  float bottom = std::max(top, std::min(bounds.bottom(), 1.0f));
+  return gfx::RectF(left, top, right - left, bottom - top);

[bajones1, 2017/07/11 17:11:06]

Sanity check: 
Does gfx::RectF enforces that right > left and bottom > top? I think so, but if
not we probably want to do that here.

[billorr, 2017/07/13 00:31:41]

Yes, setWidth/setHeight will make the width/height 0 if the parameter is <0.

+}
+
+}  // namespace
+
 void VrShellGl::UpdateLayerBounds(int16_t frame_index,
                                   const gfx::RectF& left_bounds,
                                   const gfx::RectF& right_bounds,
                                   const gfx::Size& source_size) {
+  if (!ValidateSize(source_size) || !ValidateRect(left_bounds) ||
+      !ValidateRect(right_bounds)) {
+    mojo::ReportBadMessage("UpdateLayerBounds called with invalid bounds");
+    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));
+    webvr_left_viewport_->SetSourceUv(UVFromGfxRect(ClampRect(left_bounds)));
+    webvr_right_viewport_->SetSourceUv(UVFromGfxRect(ClampRect(right_bounds)));
     CreateOrResizeWebVRSurface(source_size);
+
+    // clear all pending bounds
+    pending_bounds_ = std::queue<std::pair<uint8_t, WebVrBounds>>();
   } else {
     pending_bounds_.emplace(
         frame_index, WebVrBounds(left_bounds, right_bounds, source_size));
@@ -1621,6 +1670,7 @@ void VrShellGl::SendVSync(base::TimeDelta time, GetVSyncCallback callback) {
                                                      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(