Security: Calling "mojo::WrapSharedMemoryHandle" is insufficient to produce read-only descriptors for IPC |
|||||||||||||||||||||||
Issue descriptionVULNERABILITY DETAILS The "mojo::WrapSharedMemoryHandle" function is used to produce a "base::SharedBufferHandle" wrapping a given "base::SharedMemoryHandle". The created buffer handle can be sent over Mojo IPC to remote endpoints, including across process boundaries. In some cases, shared memory descriptors need to be made read-only before being transferred to a different process, in order to ensure that only the owning process can modify the underlying memory. While the "mojo::WrapSharedMemoryHandle" function does provide a flag indicating whether the created buffer handle should be made read-only, this only serves as a hint for the code paths wrapping and unwrapping the handle (specifying how the memory should be mapped), and does not affect the actual properties of the underlying memory region. In particular, calling "mojo::WrapSharedMemoryHandle" with the "read_only" flag set will not result in "SharedMemory::GetReadOnlyHandle" being called on the underlying handle. Consequently, calling "mojo::WrapSharedMemoryHandle" with the "read_only" flag set is insufficient for producing read-only descriptors for IPC, as the receiving endpoint can remap the received descriptor as writable and modify the shared region's contents. There are a few services which appear to use the aforementioned pattern in order to create "read-only" buffers that can be shared with clients. For example, the "device::mojom::GamepadMonitor" interface exposed by the "device" service (running in the browser process and exposed to the renderer), allows clients to acquire a "base::SharedBufferHandle" containing the current state of the connected Gamepads. The service attempts to make this memory read-only before passing it to the client, by calling "mojo::WrapSharedMemoryHandle" (https://cs.chromium.org/chromium/src/device/gamepad/gamepad_provider.cc?type=cs&l=95). However, since this does not result in "SharedMemory::GetReadOnlyHandle" being called, the underlying descriptor can be mapped as writable in the receiving process. Other instances of such the same pattern exist in the PDF Compositor client, the Shared Bitmap Allocation Notifier, the Video Encode Accelerator, etc. I think the issue can be addressed by modifying "mojo::WrapSharedMemoryHandle" to ensure that the underlying memory handle is indeed read-only when a read-only wrapping is requested (or alternately calling to GetReadOnlyHandle), thus preventing accidental misuses of the above API. VERSION Chromium 64.0.3282.0 64-bit Revision dd12859a9c856c6919cedf6c35d13b8b22af94e1-refs/heads/master@{#520743} OS Linux 4.4.0-97-generic REPRODUCTION CASE I'm attaching a patch that adds code to the renderer process which binds to the "device::mojom::GamepadMonitor" interface and maps the Gamepad memory region as writable. The patch corrupts the memory region to indicate the presence of an active gamepad with a large number of buttons. This will trigger an OOB access by GamepadHasUserGesture (see https://cs.chromium.org/chromium/src/device/gamepad/gamepad_user_gesture.cc?l=27). However, in many cases if another memory region is mapped directly after the gamepad's memory, this will not cause a crash, but will simply set the gamepad as active. For example, on my machine the "Cookies" memory mapping is consistently placed after the gamepad's memory: 7fc8cfccd000-7fc8cfcce000 rw-s 00000000 00:18 93 /run/shm/.org.chromium.Chromium.XjyPGf (deleted) 7fc8cfcce000-7fc8cfcdb000 r--s 00000000 fc:02 12481589 /usr/.../chromium/Default/Cookies This bug is subject to a 90 day disclosure deadline. If 90 days elapse without a broadly available patch, then the bug report will automatically become visible to the public.
,
Dec 7 2017
This sounds similar to Issue 789959 . Any thoughts on a good owner for this one?
,
Dec 7 2017
rockot: Are you a good owner for this? +digit for FYI, regarding the discussion about how it's important that renderers only receive handles after they've been dropped to read-only
,
Dec 7 2017
In principle this should affect Fuchsia too, right?
,
Dec 7 2017
,
Dec 8 2017
Per the documentation on WrapSharedMemoryHandle, the |read_only| flag DOES NOT force the memory handle to actually be read-only. It is essentially a reflection of SharedMemoryCreateOptions::share_read_only. I realize this is confusing and I think it needs to be addressed, but I consider it an API bug rather than an implementation bug. If the argument were named "supports_read_only_sharing" maybe that would be more correct. If there are identified instances of the API being used incorrectly (i.e. assuming |true| makes the wrapped handle only mappable as read-only), those should be fixed.
,
Dec 8 2017
I will update the API documentation accordingly and audit (and if necessary, fix) existing uses.
,
Dec 8 2017
One minor correction: the argument is not really analogous to SharedMemoryCreateOptions::share_read_only. Rather it informs the system that the handle being passed already is (or isn't) only read-only-mappable. Still obviously confusing and not documented clearly enough. I wonder if it would be worth tracking read-only-ness on base::SharedMemoryHandle itself, assuming base::SharedMemory is always the one creating or vending such handles. That would eliminate the need for this argument.
,
Dec 8 2017
I've filed a separate bug (issue 793446) to cover specific issues in the media stack. All other issues, including API clarity, should be addressed by https://chromium-review.googlesource.com/c/chromium/src/+/818282 once landed.
,
Dec 8 2017
> I wonder if it would be worth tracking read-only-ness on base::SharedMemoryHandle itself, assuming base::SharedMemory is always the one creating or vending such handles. That would eliminate the need for this argument. Yes, I do strongly think so. digit@ is adding this on Android because it's needed to make the semantics of ashmem work. If we could remove the prot argument to WrapSharedMemoryHandle (or at least make it impossible to misuse by CHECKing it against the SharedMemoryHandle's associated protection), that'd be best.
,
Dec 8 2017
+1 to tracking this on SharedMemoryHandle itself. (In general, I think we should be doing more to bundle the 'interesting' bits of info about a shared memory handle with it in both //base and mojo, i.e. see issue 788243 , where we were forced to pass an extra bit of information about size because it wasn't accessible from the handle itself)
,
Dec 23 2017
rockot: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers? If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one? If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 6 2018
rockot: Uh oh! This issue still open and hasn't been updated in the last 28 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers? If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one? If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 18 2018
,
Jan 18 2018
FYI: Related to this is bug 797470 , the issue of Clone(READ_ONLY) crashing the process when called on a mojo handle that is RW. This may be encompassed by bug 793446 (specific issues in the media stack), but I can't open it to check. ;-)
,
Jan 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/673ce95d481ea9368c4d4d43ac756ba1d6d9e608 commit 673ce95d481ea9368c4d4d43ac756ba1d6d9e608 Author: Ken Rockot <rockot@chromium.org> Date: Thu Jan 18 20:59:24 2018 Correct mojo::WrapSharedMemoryHandle usage Fixes some incorrect uses of mojo::WrapSharedMemoryHandle which were assuming that the call actually has any control over the memory protection applied to a handle when mapped. Where fixing usage is infeasible for this CL, TODOs are added to annotate follow-up work. Also updates the API and documentation to (hopefully) improve clarity and avoid similar mistakes from being made in the future. BUG= 792900 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I0578aaa9ca3bfcb01aaf2451315d1ede95458477 Reviewed-on: https://chromium-review.googlesource.com/818282 Reviewed-by: Wei Li <weili@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Yuzhu Shen <yzshen@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Commit-Queue: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#530268} [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/discardable_memory/client/client_discardable_shared_memory_manager.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/discardable_memory/service/discardable_shared_memory_manager.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/printing/browser/print_composite_client.h [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/printing/renderer/DEPS [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/printing/renderer/print_render_frame_helper.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/printing/renderer/print_render_frame_helper.h [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/printing/renderer/print_render_frame_helper_linux.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/printing/service/pdf_compositor_service_unittest.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/printing/service/public/cpp/pdf_compositor_client.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/printing/service/public/cpp/pdf_compositor_client.h [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/printing/service/public/cpp/pdf_service_mojo_utils.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/components/viz/client/client_shared_bitmap_manager.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/content/browser/browser_child_process_host_impl.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/content/renderer/media/mojo_audio_input_ipc.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/content/renderer/media/mojo_audio_output_ipc.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/content/renderer/media/video_capture_impl.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/content/renderer/media/video_capture_impl_unittest.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/device/gamepad/gamepad_provider.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/gpu/ipc/client/command_buffer_proxy_impl.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/capture/video/shared_memory_handle_provider.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/capture/video/shared_memory_handle_provider_unittest.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/mojo/clients/mojo_jpeg_decode_accelerator.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/mojo/clients/mojo_video_encode_accelerator.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/mojo/interfaces/jpeg_decode_accelerator_typemap_traits.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/mojo/interfaces/video_frame_struct_traits.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/mojo/services/mojo_audio_input_stream.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/mojo/services/mojo_audio_input_stream_unittest.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/mojo/services/mojo_audio_output_stream.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/mojo/services/mojo_audio_output_stream_unittest.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/media/mojo/services/mojo_video_encode_accelerator_service.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/mojo/edk/system/core.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/mojo/public/c/system/platform_handle.h [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/mojo/public/cpp/system/platform_handle.cc [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/mojo/public/cpp/system/platform_handle.h [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/mojo/public/cpp/system/tests/BUILD.gn [modify] https://crrev.com/673ce95d481ea9368c4d4d43ac756ba1d6d9e608/ui/gfx/mojo/buffer_types_struct_traits.cc
,
Jan 23 2018
,
Jan 23 2018
rockot: Can this be marked as fixed now? Thanks.
,
Jan 23 2018
Sure. There are other bugs tracking remaining work for shared memory API and usage cleanup.
,
Feb 8 2018
,
Feb 8 2018
,
Feb 9 2018
This bug requires manual review: DEPS changes referenced in bugdroid comments. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 9 2018
[Bulk Edit] +awhalley@ (Security TPM) for M65 merge review
,
Feb 9 2018
govind@ - good for 65, unless rockot@ has any objections
,
Feb 9 2018
rockot@, PTAL comment #24. Thank you.
,
Feb 12 2018
govind@ - good for 65, it's had a fair amount of beta coverage now
,
Feb 12 2018
Cl listed at #16 is already in M65 branch (3325). Is there any other merge needed? If not, pls remove "Merge-Review-65" label. Thank you.
,
Feb 13 2018
No merge needed
,
Mar 1 2018
Hi, The 90-day period for this vulnerability is due to expire on March 7 2018. Could you please let me know whether the issue will be fixed before the deadline is reached? Otherwise, is the fix expected to be available within a 14 day period from the original 90-day deadline? Thanks! Gal.
,
Mar 1 2018
It's fixed.
,
Mar 1 2018
The fix will be released with Chrome 65, which is due to go to stable on March 6th :-)
,
Mar 6 2018
,
Mar 6 2018
,
Apr 25 2018
,
May 2 2018
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 14
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by laginimaineb@google.com
, Dec 7 2017