Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2950233002:
Validate untrusted VR mojo inputs into browser process (Closed)
DescriptionValidate untrusted VR mojo inputs into browser process
For defense in depth, we cannot trust data coming from the renderer
processes. This change hardens the mojo API surface by validating
parameters before using them.
frame_index on SubmitFrame and UpdateLayerBounds could be used to
read/write undesired memory locations (by passing in negative indices),
but aren't obviously attackable.
Viewports could contain NANs or invalid rectangles, and the desired size
could be too large/small.
BUG=729852
Review-Url: https://codereview.chromium.org/2950233002
Cr-Commit-Position: refs/heads/master@{#486585}
Committed: https://chromium.googlesource.com/chromium/src/+/0cdabaee801fff51aa2d90258c58da52a4e8b118
Patch Set 1 #
Total comments: 5
Patch Set 2 : make validation less strict, and fix a bug found with validation #
Total comments: 8
Patch Set 3 : cr feedback #Patch Set 4 : add test and rebase #
Messages
Total messages: 32 (15 generated)
billorr@chromium.org changed reviewers: + mthiesse@chromium.org
PTAL https://codereview.chromium.org/2950233002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2950233002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:1603: // Size should be between at least 2 in each direction and less than 5000. arbitrary values - what should we use? Or are we ok with any size?
mthiesse@chromium.org changed reviewers: + bajones@chromium.org
+bajones for review/opinions. https://codereview.chromium.org/2950233002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2950233002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:1593: return bounds.x() < bounds.right() && bounds.y() < bounds.bottom() && 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 https://codereview.chromium.org/2950233002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:1603: // Size should be between at least 2 in each direction and less than 5000. On 2017/06/22 00:28:34, billorr wrote: > arbitrary values - what should we use? Or are we ok with any size? 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. https://codereview.chromium.org/2950233002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:1619: mojo::ReportBadMessage("UpdateLayerBounds called with invalid bounds"); 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.
Michael/Brandon, could one of you take a look at the latest iteration? https://codereview.chromium.org/2950233002/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2950233002/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:1593: return bounds.x() < bounds.right() && bounds.y() < bounds.bottom() && On 2017/06/22 05:59:47, mthiesse wrote: > 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 Done.
https://codereview.chromium.org/2950233002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2950233002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:1603: float left = std::max(0.0f, std::min(bounds.x(), 1.0f)); I think you can just use bounds.AdjustToFit(gfx::RectF(0, 0, 1, 1));? https://codereview.chromium.org/2950233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2950233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:375: for (float value : layer_.leftBounds()) { 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. https://codereview.chromium.org/2950233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:396: Also check the width/height to be >= 0 to avoid unnecessary crashes.
Also, are these error cases testable with LayoutTests?
bajones@google.com changed reviewers: + bajones@google.com
Agree with mthiesse@'s comments, and had one question of my own, but otherwise this is looking good. https://codereview.chromium.org/2950233002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2950233002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:1607: return gfx::RectF(left, top, right - left, bottom - top); 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.
On 2017/07/11 14:53:05, mthiesse wrote: > Also, are these error cases testable with LayoutTests? I don't know how to send arbitrary messages from the test. It seems like something that should be doable though.
https://codereview.chromium.org/2950233002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2950233002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:1603: float left = std::max(0.0f, std::min(bounds.x(), 1.0f)); On 2017/07/11 14:52:24, mthiesse wrote: > I think you can just use bounds.AdjustToFit(gfx::RectF(0, 0, 1, 1));? 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. https://codereview.chromium.org/2950233002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:1607: return gfx::RectF(left, top, right - left, bottom - top); On 2017/07/11 17:11:06, bajones1 wrote: > 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. Yes, setWidth/setHeight will make the width/height 0 if the parameter is <0. https://codereview.chromium.org/2950233002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2950233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:375: for (float value : layer_.leftBounds()) { On 2017/07/11 14:52:24, mthiesse wrote: > 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. 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. https://codereview.chromium.org/2950233002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:396: On 2017/07/11 14:52:24, mthiesse wrote: > Also check the width/height to be >= 0 to avoid unnecessary crashes. 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.
On 2017/07/13 00:31:29, billorr wrote: > On 2017/07/11 14:53:05, mthiesse wrote: > > Also, are these error cases testable with LayoutTests? > > I don't know how to send arbitrary messages from the test. It seems like > something that should be doable though. Why would you have to send arbitrary messages? Just call requestPresent with various invalid layer bounds. Should be a trivial test.
Ahh you meant to check the js side validation? I thought you meant the browser side validation. Yes the js side is easy to test. On Jul 13, 2017 7:54 AM, <mthiesse@chromium.org> wrote: On 2017/07/13 00:31:29, billorr wrote: > On 2017/07/11 14:53:05, mthiesse wrote: > > Also, are these error cases testable with LayoutTests? > > I don't know how to send arbitrary messages from the test. It seems like > something that should be doable though. Why would you have to send arbitrary messages? Just call requestPresent with various invalid layer bounds. Should be a trivial test. https://codereview.chromium.org/2950233002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm once tests are added.
The CQ bit was checked by billorr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by billorr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2950233002/#ps60001 (title: "add test and rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
You'll still need an lg from Brandon for webkit owners.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/07/13 23:13:08, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Lgtm
The CQ bit was checked by billorr@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499992806116290,
"parent_rev": "290d0a12119fcf23eec5aafeb2a3c509061f9703", "commit_rev":
"ac0fd2525d56c770908b27d83a97c85dcd8c2b7e"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499992806116290,
"parent_rev": "95b28c48a98d0cdcbdfd47d94e29e1b88636461e", "commit_rev":
"65fa5a0fa6511324c8a3e1985136caf414bd1e64"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499992806116290,
"parent_rev": "4584b248f66bd2b857faf80244a82fba2f71f186", "commit_rev":
"0cdabaee801fff51aa2d90258c58da52a4e8b118"}
Message was sent while issue was closed.
Description was changed from ========== Validate untrusted VR mojo inputs into browser process For defense in depth, we cannot trust data coming from the renderer processes. This change hardens the mojo API surface by validating parameters before using them. frame_index on SubmitFrame and UpdateLayerBounds could be used to read/write undesired memory locations (by passing in negative indices), but aren't obviously attackable. Viewports could contain NANs or invalid rectangles, and the desired size could be too large/small. BUG=729852 ========== to ========== Validate untrusted VR mojo inputs into browser process For defense in depth, we cannot trust data coming from the renderer processes. This change hardens the mojo API surface by validating parameters before using them. frame_index on SubmitFrame and UpdateLayerBounds could be used to read/write undesired memory locations (by passing in negative indices), but aren't obviously attackable. Viewports could contain NANs or invalid rectangles, and the desired size could be too large/small. BUG=729852 Review-Url: https://codereview.chromium.org/2950233002 Cr-Commit-Position: refs/heads/master@{#486585} Committed: https://chromium.googlesource.com/chromium/src/+/0cdabaee801fff51aa2d90258c58... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/0cdabaee801fff51aa2d90258c58... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
