Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2811413002:
Add check for renderer client id. (Closed)
DescriptionCheck renderer client id for offscreen canvas.
Add a check before creating an OffscreenCanvasSurfaceImpl or
MojoCompositorFrameSink for offscreen canvas. The renderer should only
be using FrameSinkIds within it's own namespace.
Without this check, a malicious renderer could learn about SurfaceIds
from other clients by guessing the FrameSinkId. After creating an
OffscreenCanvasSurfaceImpl any new SurfaceIds for the FrameSinkId get
forwarded back to the renderer.
BUG=664547
TBR=piman@chromium.org
Review-Url: https://codereview.chromium.org/2811413002
Cr-Commit-Position: refs/heads/master@{#465300}
Committed: https://chromium.googlesource.com/chromium/src/+/8c09280da3c8a6c4c7e064ddbf7851498abb577d
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 6
Patch Set 3 : Don't CHECK, return early. #Patch Set 4 : Fix comment. #
Messages
Total messages: 28 (20 generated)
Description was changed from ========== Add check for renderer client id. WIP. BUG= ========== to ========== Add check for renderer client id. WIP. BUG= ==========
Description was changed from ========== Add check for renderer client id. WIP. BUG= ========== to ========== Check renderer client id for offscreen canvas. Add a check before creating an OffscreenCanvasSurfaceImpl or MojoCompositorFrameSink for offscreen canvas. The renderer should only be using FrameSinkIds within it's own namespace. Without this check, a malicious renderer could learn about SurfaceIds from other clients by guessing the FrameSinkId. BUG=664547 ==========
Description was changed from ========== Check renderer client id for offscreen canvas. Add a check before creating an OffscreenCanvasSurfaceImpl or MojoCompositorFrameSink for offscreen canvas. The renderer should only be using FrameSinkIds within it's own namespace. Without this check, a malicious renderer could learn about SurfaceIds from other clients by guessing the FrameSinkId. BUG=664547 ========== to ========== Check renderer client id for offscreen canvas. Add a check before creating an OffscreenCanvasSurfaceImpl or MojoCompositorFrameSink for offscreen canvas. The renderer should only be using FrameSinkIds within it's own namespace. Without this check, a malicious renderer could learn about SurfaceIds from other clients by guessing the FrameSinkId. After creating an OffscreenCanvasSurfaceImpl any new SurfaceIds for the FrameSinkId get forwarded back to the renderer. BUG=664547 ==========
kylechar@chromium.org changed reviewers: + piman@chromium.org, tsepez@chromium.org
piman/tsepez: This seems like a potential security problem (at least would be if offscreen canvas was enabled). Let me know what you think of this as a quick fix. I'll make it crash the renderer instead as a follow up.
https://codereview.chromium.org/2811413002/diff/20001/content/browser/rendere... File content/browser/renderer_host/offscreen_canvas_provider_impl.cc (right): https://codereview.chromium.org/2811413002/diff/20001/content/browser/rendere... content/browser/renderer_host/offscreen_canvas_provider_impl.cc:31: CHECK_EQ(frame_sink_id.client_id(), renderer_client_id_); Can we just return early / do nothing (and DLOG etc.)? It will mess up the renderer, but it's the one asking for trouble :) https://codereview.chromium.org/2811413002/diff/20001/content/browser/rendere... content/browser/renderer_host/offscreen_canvas_provider_impl.cc:42: CHECK_EQ(frame_sink_id.client_id(), renderer_client_id_); ditto. https://codereview.chromium.org/2811413002/diff/20001/content/browser/rendere... File content/browser/renderer_host/offscreen_canvas_provider_impl.h (right): https://codereview.chromium.org/2811413002/diff/20001/content/browser/rendere... content/browser/renderer_host/offscreen_canvas_provider_impl.h:18: OffscreenCanvasProviderImpl(uint32_t renderer_client_id); nit: explicit
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
FrameSinkIds aren't really meant to be private. A client still can't embed an arbitrary surface because the LocalSurfaceId is unguessable.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2811413002/diff/20001/content/browser/rendere... File content/browser/renderer_host/offscreen_canvas_provider_impl.cc (right): https://codereview.chromium.org/2811413002/diff/20001/content/browser/rendere... content/browser/renderer_host/offscreen_canvas_provider_impl.cc:31: CHECK_EQ(frame_sink_id.client_id(), renderer_client_id_); On 2017/04/12 22:53:37, piman - OOO back 4-27 wrote: > Can we just return early / do nothing (and DLOG etc.)? > It will mess up the renderer, but it's the one asking for trouble :) Done. https://codereview.chromium.org/2811413002/diff/20001/content/browser/rendere... content/browser/renderer_host/offscreen_canvas_provider_impl.cc:42: CHECK_EQ(frame_sink_id.client_id(), renderer_client_id_); On 2017/04/12 22:53:37, piman - OOO back 4-27 wrote: > ditto. Done. https://codereview.chromium.org/2811413002/diff/20001/content/browser/rendere... File content/browser/renderer_host/offscreen_canvas_provider_impl.h (right): https://codereview.chromium.org/2811413002/diff/20001/content/browser/rendere... content/browser/renderer_host/offscreen_canvas_provider_impl.h:18: OffscreenCanvasProviderImpl(uint32_t renderer_client_id); On 2017/04/12 22:53:37, piman - OOO back 4-27 wrote: > nit: explicit Done.
LGTM per the unguessable IDs.
Description was changed from ========== Check renderer client id for offscreen canvas. Add a check before creating an OffscreenCanvasSurfaceImpl or MojoCompositorFrameSink for offscreen canvas. The renderer should only be using FrameSinkIds within it's own namespace. Without this check, a malicious renderer could learn about SurfaceIds from other clients by guessing the FrameSinkId. After creating an OffscreenCanvasSurfaceImpl any new SurfaceIds for the FrameSinkId get forwarded back to the renderer. BUG=664547 ========== to ========== Check renderer client id for offscreen canvas. Add a check before creating an OffscreenCanvasSurfaceImpl or MojoCompositorFrameSink for offscreen canvas. The renderer should only be using FrameSinkIds within it's own namespace. Without this check, a malicious renderer could learn about SurfaceIds from other clients by guessing the FrameSinkId. After creating an OffscreenCanvasSurfaceImpl any new SurfaceIds for the FrameSinkId get forwarded back to the renderer. BUG=664547 ==========
The CQ bit was checked by kylechar@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...
Description was changed from ========== Check renderer client id for offscreen canvas. Add a check before creating an OffscreenCanvasSurfaceImpl or MojoCompositorFrameSink for offscreen canvas. The renderer should only be using FrameSinkIds within it's own namespace. Without this check, a malicious renderer could learn about SurfaceIds from other clients by guessing the FrameSinkId. After creating an OffscreenCanvasSurfaceImpl any new SurfaceIds for the FrameSinkId get forwarded back to the renderer. BUG=664547 ========== to ========== Check renderer client id for offscreen canvas. Add a check before creating an OffscreenCanvasSurfaceImpl or MojoCompositorFrameSink for offscreen canvas. The renderer should only be using FrameSinkIds within it's own namespace. Without this check, a malicious renderer could learn about SurfaceIds from other clients by guessing the FrameSinkId. After creating an OffscreenCanvasSurfaceImpl any new SurfaceIds for the FrameSinkId get forwarded back to the renderer. BUG=664547 TBR=piman@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kylechar@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: This issue passed the CQ dry run.
TBRing piman since he's OOO for a while.
The CQ bit was checked by kylechar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2811413002/#ps80001 (title: "Fix comment.")
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": 80001, "attempt_start_ts": 1492539433072390,
"parent_rev": "3e82c5b9d88086ead8bbbd122eef8c1b5d45f5e3", "commit_rev":
"8c09280da3c8a6c4c7e064ddbf7851498abb577d"}
Message was sent while issue was closed.
Description was changed from ========== Check renderer client id for offscreen canvas. Add a check before creating an OffscreenCanvasSurfaceImpl or MojoCompositorFrameSink for offscreen canvas. The renderer should only be using FrameSinkIds within it's own namespace. Without this check, a malicious renderer could learn about SurfaceIds from other clients by guessing the FrameSinkId. After creating an OffscreenCanvasSurfaceImpl any new SurfaceIds for the FrameSinkId get forwarded back to the renderer. BUG=664547 TBR=piman@chromium.org ========== to ========== Check renderer client id for offscreen canvas. Add a check before creating an OffscreenCanvasSurfaceImpl or MojoCompositorFrameSink for offscreen canvas. The renderer should only be using FrameSinkIds within it's own namespace. Without this check, a malicious renderer could learn about SurfaceIds from other clients by guessing the FrameSinkId. After creating an OffscreenCanvasSurfaceImpl any new SurfaceIds for the FrameSinkId get forwarded back to the renderer. BUG=664547 TBR=piman@chromium.org Review-Url: https://codereview.chromium.org/2811413002 Cr-Commit-Position: refs/heads/master@{#465300} Committed: https://chromium.googlesource.com/chromium/src/+/8c09280da3c8a6c4c7e064ddbf78... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8c09280da3c8a6c4c7e064ddbf78... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
