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 0fe0fef80b484ca58101375ff79e6b778aced06c..8aa2220422e47af8a63e876fefcc18ff1f3d1837 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);
   }
@@ -1575,10 +1584,50 @@ 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));
@@ -1619,6 +1668,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(