DeviceSensorHost exposes shared memory handles from StartPolling as read-write |
|||||||||
Issue descriptionWhile working on a fix for issue 789959 (https://chromium-review.googlesource.com/c/chromium/src/+/805238), some test failures were encountered in DeviceSensorBrowserTest. From manual auditing, it appears that the implementation of DeviceSensorHost::StartPolling does not drop write permissions from the shared memory handle it vends. Specifically, DeviceSensorHost<T,U>::StartPolling() [1] calls DataFetcherSharedMemoryBase::GetSharedMemoryHandle() [2]. This uses mojo::ScopedSharedBufferHandle::Clone() [3] with no argument to duplicate the handle. The argument to clone is the protection mode, which defaults to READ_WRITE (also note issue 793503 ). Because of that, the memory handle vended by StartPolling() to clients is actually writable, when it seems that the intent is that it should be read-only. [1] https://chromium.googlesource.com/chromium/src/+/f192d5d30952f9372ad8dac37cdbf5c72dba507a/device/sensors/device_sensor_host.cc#52 [2] https://chromium.googlesource.com/chromium/src/+/f192d5d30952f9372ad8dac37cdbf5c72dba507a/device/sensors/data_fetcher_shared_memory_base.cc#185 [3] https://chromium.googlesource.com/chromium/src/+/f192d5d30952f9372ad8dac37cdbf5c72dba507a/mojo/public/cpp/system/buffer.h#63 I spoke with reillyg@ about this, and apparently //device/sensors is scheduled for removal ( issue 721427 and https://chromium-review.googlesource.com/c/chromium/src/+/667784) because the web API has changed to use //services/device/generic_sensor. The code to call StartPolling() was actually removed in https://chromium-review.googlesource.com/c/chromium/src/+/726261 (M64), but this issue does affect stable.
,
Dec 9 2017
This is probably Medium because the data that can be overwritten isn't critical and it only affects other renderers.
,
Dec 9 2017
,
Dec 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c9e5af619bf3bd0e3e56b868945f53b7dad2db49 commit c9e5af619bf3bd0e3e56b868945f53b7dad2db49 Author: Robert Sesek <rsesek@chromium.org> Date: Tue Dec 12 23:27:37 2017 Remove default argument from mojo::SharedBufferHandle::Clone(). This also changes wherever possible from the old implicit value of READ_WRITE to READ_ONLY. Bug: 793503 , 793519 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: I0ebafb2eb93af9718e71a303e239ecaa4e7d8f35 Reviewed-on: https://chromium-review.googlesource.com/820011 Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Christian Fremerey <chfremer@chromium.org> Commit-Queue: Robert Sesek <rsesek@chromium.org> Cr-Commit-Position: refs/heads/master@{#523600} [modify] https://crrev.com/c9e5af619bf3bd0e3e56b868945f53b7dad2db49/components/visitedlink/test/visitedlink_unittest.cc [modify] https://crrev.com/c9e5af619bf3bd0e3e56b868945f53b7dad2db49/content/browser/renderer_host/media/video_capture_controller.cc [modify] https://crrev.com/c9e5af619bf3bd0e3e56b868945f53b7dad2db49/content/common/sandbox_mac_fontloading_unittest.mm [modify] https://crrev.com/c9e5af619bf3bd0e3e56b868945f53b7dad2db49/content/renderer/device_sensors/device_motion_event_pump_unittest.cc [modify] https://crrev.com/c9e5af619bf3bd0e3e56b868945f53b7dad2db49/content/renderer/device_sensors/device_orientation_event_pump_unittest.cc [modify] https://crrev.com/c9e5af619bf3bd0e3e56b868945f53b7dad2db49/device/sensors/data_fetcher_shared_memory_base.cc [modify] https://crrev.com/c9e5af619bf3bd0e3e56b868945f53b7dad2db49/gpu/command_buffer/common/activity_flags.h [modify] https://crrev.com/c9e5af619bf3bd0e3e56b868945f53b7dad2db49/media/mojo/interfaces/video_frame_struct_traits.cc [modify] https://crrev.com/c9e5af619bf3bd0e3e56b868945f53b7dad2db49/mojo/public/cpp/system/buffer.h
,
Dec 13 2017
Going to call this fixed in M64. As noted in the OP, the service is not actually bound anymore so the code is unreachable in M64, even though it is still in the tree and the interface is still listed in the manifest (though also now removed after https://chromium-review.googlesource.com/c/chromium/src/+/818488). And after the CL in #4, the handle is now shared read-only (for the unreachable code).
,
Dec 14 2017
,
Jan 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e2a6576a6acef5cb066e6cc4373f463c857e79bd commit e2a6576a6acef5cb066e6cc4373f463c857e79bd Author: David 'Digit' Turner <digit@google.com> Date: Fri Jan 19 17:51:51 2018 android: Enforce read-only mapping of Ashmem regions. This is a CL to solve the issue raised by 789959. The root of the problem is that Android ashmem regions have a very different security model than regular Posix shared memory regions: - On Posix: the region doesn't have any associated read/write/exec protection mask, mmap() uses the file descriptor's access mode to determine if writable mappings are allowed. E.g. if the descriptor is opened with O_RDONLY, then an mmap() with PROT_WRITE will fail with an EACCES error. - On Android: the Ashmem region has its own read/write/exec protection mask, and the file descriptor's access mode is ignored at mmap() time. Also, it is possible to change the region's protection mask through an ioctl(), that only allows dropping privileges. In other words, once a region has been turned read-only, it cannot be turned back to read-write, and any future mmap() attempts are affected (existing mappings that are read-write survive though). - Also, there is no reliable way in Posix or Android to duplicate a file descriptor as read-only (even when /dev/fd/ or /proc/self/fd/ are available), and no way to re-open an existing Ashmem region as read-only. This means that all Ashmem file descriptors have read-write access anyway. Chromium code assumes that it is possible to create read-only descriptors to shared memory region, then send them through IPC to another process. On Android, this cannot work because all descriptors are read-write anyway. This CL works as follows to route around the issue, in the least invasive possible way: - On Android, add a "read_only_" flag to each SharedMemoryHandle instance, indicating that it is intended to only allow read-only mappings. This can be set with SetReadOnly() and tested with IsReadOnly(). Also ensure the flag is properly maintained when copying the handle instance, or sending it through IPC. - Add a method called SetRegionReadOnly() to the Android version of the SharedMemoryHandle. This drops the write access bits from the _region_, making all future mappings _not_ writable. - Ensure that SetRegionReadOnly() is called at Map() time, when needed. - Ensure that the region is sealed read-only when it is sent through IPC or Mojo through a read-only handle on Android. - Modify the FieldTrial code to set the region read-only as soon as possible. This happens after the writable mapping has been created, and used by the PersistentMemoryAllocator, so it only affects processes that receive the region's file descriptor (i.e. sandboxed processes on Android). Also add a unit-test to verify that this works properly. - Modify the user script loader to set the region read-only as soon as it has been populated. The code clearly throws away the original writable mapping before returning a read-only SharedMemory instance, so this should not create any problem. NOTE: This code is likely not used on Android yet, but better be safe than sorry. - Fix content browser sensor-related tests, which used to map a shared memory region writable _after_ a read-only descriptor to it had been sent through Mojo to clients. - Fix DeviceSensorHost implementation to ensure that its StartPolling() method always sends read-only file descriptors to the callback argument ( crbug.com/793519 , not Android-specific). Also: - Add ashmem_get_prot_region() to third_party/ashmem/ashmem.h, necessary to retrieve a region's current protection mask. This shall probably go into another CL. - Fix a bug where Ashmem regions were blindy created with PROT_EXEC permission, even if the CreateOptions::executable bit was false. - Add a unit-test to verify that anonymous region that are not created with CreateOptions::executable set to true cannot be mapped with PROT_EXEC. This test is Android-specific. It turns out that it fails on Linux (because /dev/shm is mounted on a tmpfs partition without the 'noexe' option, and there is no other way provided by the system to restrict PROT_EXEC-inducing mprotect() calls for these). Bug: 789959 , 793519 Change-Id: Ibb02eddedd84f95462d7f8b94d3f2a100b983661 Reviewed-on: https://chromium-review.googlesource.com/805238 Commit-Queue: David Turner <digit@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> Reviewed-by: Tim Volodine <timvolodine@chromium.org> Reviewed-by: agrieve <agrieve@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Reviewed-by: Brandon Jones <bajones@chromium.org> Reviewed-by: Camille Lamy <clamy@chromium.org> Cr-Commit-Position: refs/heads/master@{#530553} [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/memory/shared_memory.h [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/memory/shared_memory_android.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/memory/shared_memory_handle.h [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/memory/shared_memory_handle_android.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/memory/shared_memory_posix.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/memory/shared_memory_unittest.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/metrics/field_trial.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/metrics/field_trial.h [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/metrics/field_trial_unittest.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/test/BUILD.gn [add] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/test/test_shared_memory_util.cc [add] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/base/test/test_shared_memory_util.h [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/content/browser/device_sensors/device_sensor_browsertest.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/content/browser/generic_sensor_browsertest.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/content/renderer/device_sensors/fake_sensor_and_provider.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/content/renderer/device_sensors/fake_sensor_and_provider.h [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/device/vr/orientation/orientation_device_unittest.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/extensions/browser/user_script_loader.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/ipc/ipc_message_utils.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/mojo/edk/embedder/platform_shared_buffer.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/mojo/edk/embedder/platform_shared_buffer.h [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/mojo/edk/system/shared_buffer_dispatcher.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/mojo/edk/system/shared_buffer_dispatcher_unittest.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/services/device/generic_sensor/platform_sensor_and_provider_unittest.cc [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/third_party/ashmem/README.chromium [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/third_party/ashmem/ashmem-dev.c [modify] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/third_party/ashmem/ashmem.h [add] https://crrev.com/e2a6576a6acef5cb066e6cc4373f463c857e79bd/third_party/ashmem/patches/0001-Add-ashmem-get-prot-region.patch
,
Jan 22 2018
,
Feb 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5826891a08d8c63947d8f41ab9c15c565f96e74e commit 5826891a08d8c63947d8f41ab9c15c565f96e74e Author: David 'Digit' Turner <digit@google.com> Date: Thu Feb 08 19:50:17 2018 android: Enforce read-only mapping of Ashmem regions. This is a CL to solve the issue raised by 789959. The root of the problem is that Android ashmem regions have a very different security model than regular Posix shared memory regions: - On Posix: the region doesn't have any associated read/write/exec protection mask, mmap() uses the file descriptor's access mode to determine if writable mappings are allowed. E.g. if the descriptor is opened with O_RDONLY, then an mmap() with PROT_WRITE will fail with an EACCES error. - On Android: the Ashmem region has its own read/write/exec protection mask, and the file descriptor's access mode is ignored at mmap() time. Also, it is possible to change the region's protection mask through an ioctl(), that only allows dropping privileges. In other words, once a region has been turned read-only, it cannot be turned back to read-write, and any future mmap() attempts are affected (existing mappings that are read-write survive though). - Also, there is no reliable way in Posix or Android to duplicate a file descriptor as read-only (even when /dev/fd/ or /proc/self/fd/ are available), and no way to re-open an existing Ashmem region as read-only. This means that all Ashmem file descriptors have read-write access anyway. Chromium code assumes that it is possible to create read-only descriptors to shared memory region, then send them through IPC to another process. On Android, this cannot work because all descriptors are read-write anyway. This CL works as follows to route around the issue, in the least invasive possible way: - On Android, add a "read_only_" flag to each SharedMemoryHandle instance, indicating that it is intended to only allow read-only mappings. This can be set with SetReadOnly() and tested with IsReadOnly(). Also ensure the flag is properly maintained when copying the handle instance, or sending it through IPC. - Add a method called SetRegionReadOnly() to the Android version of the SharedMemoryHandle. This drops the write access bits from the _region_, making all future mappings _not_ writable. - Ensure that SetRegionReadOnly() is called at Map() time, when needed. - Ensure that the region is sealed read-only when it is sent through IPC or Mojo through a read-only handle on Android. - Modify the FieldTrial code to set the region read-only as soon as possible. This happens after the writable mapping has been created, and used by the PersistentMemoryAllocator, so it only affects processes that receive the region's file descriptor (i.e. sandboxed processes on Android). Also add a unit-test to verify that this works properly. - Modify the user script loader to set the region read-only as soon as it has been populated. The code clearly throws away the original writable mapping before returning a read-only SharedMemory instance, so this should not create any problem. NOTE: This code is likely not used on Android yet, but better be safe than sorry. - Fix content browser sensor-related tests, which used to map a shared memory region writable _after_ a read-only descriptor to it had been sent through Mojo to clients. - Fix DeviceSensorHost implementation to ensure that its StartPolling() method always sends read-only file descriptors to the callback argument ( crbug.com/793519 , not Android-specific). Also: - Add ashmem_get_prot_region() to third_party/ashmem/ashmem.h, necessary to retrieve a region's current protection mask. This shall probably go into another CL. - Fix a bug where Ashmem regions were blindy created with PROT_EXEC permission, even if the CreateOptions::executable bit was false. - Add a unit-test to verify that anonymous region that are not created with CreateOptions::executable set to true cannot be mapped with PROT_EXEC. This test is Android-specific. It turns out that it fails on Linux (because /dev/shm is mounted on a tmpfs partition without the 'noexe' option, and there is no other way provided by the system to restrict PROT_EXEC-inducing mprotect() calls for these). Bug: 789959 , 793519 Change-Id: Ibb02eddedd84f95462d7f8b94d3f2a100b983661 Reviewed-on: https://chromium-review.googlesource.com/805238 Commit-Queue: David Turner <digit@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> Reviewed-by: Tim Volodine <timvolodine@chromium.org> Reviewed-by: agrieve <agrieve@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Reviewed-by: Brandon Jones <bajones@chromium.org> Reviewed-by: Camille Lamy <clamy@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#530553}(cherry picked from commit e2a6576a6acef5cb066e6cc4373f463c857e79bd) Reviewed-on: https://chromium-review.googlesource.com/909628 Cr-Commit-Position: refs/branch-heads/3325@{#386} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/memory/shared_memory.h [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/memory/shared_memory_android.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/memory/shared_memory_handle.h [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/memory/shared_memory_handle_android.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/memory/shared_memory_posix.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/memory/shared_memory_unittest.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/metrics/field_trial.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/metrics/field_trial.h [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/metrics/field_trial_unittest.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/test/BUILD.gn [add] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/test/test_shared_memory_util.cc [add] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/base/test/test_shared_memory_util.h [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/content/browser/device_sensors/device_sensor_browsertest.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/content/browser/generic_sensor_browsertest.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/content/renderer/device_sensors/fake_sensor_and_provider.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/content/renderer/device_sensors/fake_sensor_and_provider.h [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/device/vr/orientation/orientation_device_unittest.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/extensions/browser/user_script_loader.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/ipc/ipc_message_utils.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/mojo/edk/embedder/platform_shared_buffer.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/mojo/edk/embedder/platform_shared_buffer.h [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/mojo/edk/system/shared_buffer_dispatcher.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/mojo/edk/system/shared_buffer_dispatcher_unittest.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/services/device/generic_sensor/platform_sensor_and_provider_unittest.cc [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/third_party/ashmem/README.chromium [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/third_party/ashmem/ashmem-dev.c [modify] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/third_party/ashmem/ashmem.h [add] https://crrev.com/5826891a08d8c63947d8f41ab9c15c565f96e74e/third_party/ashmem/patches/0001-Add-ashmem-get-prot-region.patch
,
Mar 6 2018
,
Mar 22 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 |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by rsesek@chromium.org
, Dec 9 2017