Validate untrusted VR mojo inputs into browser process

third_party/WebKit/Source/modules/vr/VRDisplay.cpp

Index: third_party/WebKit/Source/modules/vr/VRDisplay.cpp
diff --git a/third_party/WebKit/Source/modules/vr/VRDisplay.cpp b/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
index e161005f8d4698aba5e9f3fceae5de5c7c75b5d8..9d43361890648e551aab3b23d3c5a7cb9e34ddbf 100644
--- a/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
+++ b/third_party/WebKit/Source/modules/vr/VRDisplay.cpp
@@ -372,6 +372,28 @@ ScriptPromise VRDisplay::requestPresent(ScriptState* script_state,
     return promise;
   }
 
+  for (float value : layer_.leftBounds()) {

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

Should move these checks into UpdateLayerBounds(), and return an error or
something so that the caller can reject the promise, as UpdateLayerBounds is
called from multiple places.

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

It is called from multiple places, but this is the only place we are consuming
data from callers - I'd prefer to keep the validation close to where data comes
from javascript.  This is where other validation of layer_ is performed.

Also, we don't call UpdateLayerBounds for a new presentation until BeginPresent,
which is after GVR has started up.

+    if (std::isnan(value)) {
+      ForceExitPresent();
+      DOMException* exception = DOMException::Create(
+          kInvalidStateError, "Layer bounds must not contain NAN values");
+      resolver->Reject(exception);
+      ReportPresentationResult(PresentationResult::kInvalidLayerBounds);
+      return promise;
+    }
+  }
+
+  for (float value : layer_.rightBounds()) {
+    if (std::isnan(value)) {
+      ForceExitPresent();
+      DOMException* exception = DOMException::Create(
+          kInvalidStateError, "Layer bounds must not contain NAN values");
+      resolver->Reject(exception);
+      ReportPresentationResult(PresentationResult::kInvalidLayerBounds);
+      return promise;
+    }
+  }
+

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

Also check the width/height to be >= 0 to avoid unnecessary crashes.

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

I removed the size check on the other side.  We already check for size.width !=
0 and size.height != 0.  The magic of mojo turns the WebSize into a gfx::size,
which will make width and height = 0 if negative.

   if (!pending_present_resolvers_.IsEmpty()) {
     // If we are waiting on the results of a previous requestPresent call don't
     // fire a new request, just cache the resolver and resolve it when the
@@ -697,6 +719,9 @@ void VRDisplay::submitFrame() {
   TRACE_EVENT_END0("gpu", "VRDisplay::SubmitFrame");
 
   did_submit_this_frame_ = true;
+  // Reset our frame id, since anything we'd want to do (resizing/etc) can
+  // no-longer happen to this frame.
+  vr_frame_id_ = -1;
   // If we were deferring a rAF-triggered vsync request, do this now.
   RequestVSync();