New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: Calling "mojo::WrapSharedMemoryHandle" is insufficient to produce read-only descriptors for IPC

Reported by laginimaineb@google.com, Dec 7 2017

Issue description

VULNERABILITY 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.
 
 
wrap_shared_memory.diff
2.4 KB Download
EDIT: The "device::mojom::GamepadMonitor" interface is exposed by the "content_browser" service, not the "device" service.
Cc: palmer@chromium.org rsesek@chromium.org
Components: Internals>Mojo
Labels: Security_Severity-High Security_Impact-Stable OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows Pri-1
Status: Untriaged (was: Unconfirmed)
This sounds similar to  Issue 789959 . Any thoughts on a good owner for this one?
Cc: jam@chromium.org digit@chromium.org
Owner: roc...@chromium.org
Status: Assigned (was: Untriaged)
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
Components: Internals>Core
Labels: M-64 OS-Fuchsia
In principle this should affect Fuchsia too, right?
Cc: dcheng@chromium.org
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.
I will update the API documentation accordingly and audit (and if necessary, fix) existing uses.
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.
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.
> 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.
+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)
Project Member

Comment 12 by sheriffbot@chromium.org, 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
Project Member

Comment 13 by sheriffbot@chromium.org, 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
Cc: m...@chromium.org

Comment 15 by m...@chromium.org, 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. ;-)
Project Member

Comment 16 by bugdroid1@chromium.org, 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

Cc: mattcary@chromium.org alexilin@chromium.org
rockot: Can this be marked as fixed now? Thanks.
Status: Fixed (was: Assigned)
Sure. There are other bugs tracking remaining work for shared memory API and usage cleanup.
Project Member

Comment 20 by sheriffbot@chromium.org, Feb 8

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 21 by sheriffbot@chromium.org, Feb 8

Labels: Merge-Request-65
Project Member

Comment 22 by sheriffbot@chromium.org, Feb 9

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
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
Cc: awhalley@chromium.org
[Bulk Edit]

+awhalley@ (Security TPM) for M65 merge review
govind@ - good for 65, unless rockot@ has any objections
rockot@, PTAL comment #24. Thank you.
govind@ - good for 65, it's had a fair amount of beta coverage now
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.
Labels: -Merge-Review-65
No merge needed
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.
It's fixed.
Labels: -M-64 M-65
The fix will be released with Chrome 65, which is due to go to stable on March 6th :-)
Labels: Release-0-M65
Labels: CVE-2018-6063
Labels: CVE_description-missing
Project Member

Comment 35 by sheriffbot@chromium.org, May 2

Labels: -Restrict-View-SecurityNotify allpublic
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

Sign in to add a comment