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:
Last visit > 30 days ago
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 0
Type: Bug-Security

Blocked on:
issue 320865



Sign in to add a comment

Security: Read-only SharedMemory descriptors on Android are writable

Reported by laginimaineb@google.com, Nov 30 2017

Issue description

VULNERABILITY DETAILS
The base::SharedMemory class represents a shared memory resource that processes can map into their virtual address space.

As shared memory mechanisms differ across operating systems, specialised implementations exist for each OS. In Android's case, the implementation is largely provided by POSIX-specific code (https://cs.chromium.org/chromium/src/base/memory/shared_memory_posix.cc), with some Android-specific parts (https://cs.chromium.org/chromium/src/base/memory/shared_memory_android.cc). 

SharedMemory descriptors are often passed via IPC across process boundaries. In some cases, the 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. To retrieve a read-only descriptor that can be transferred over IPC, the base::SharedMemory::GetReadOnlyHandle() method (https://cs.chromium.org/chromium/src/base/memory/shared_memory.h?l=211) can be invoked. In Android's case, this implementation is provided by the POSIX-specific code path.

Normally, the above code relies on "/dev/shm" in order to produce the shared memory descriptors. Using this approach, the code can keep a descriptor to the same memory region, but opened using O_RDONLY. The same descriptor is then used to provide the read-only handle returned by GetReadOnlyHandle, preventing callers from mapping it as writable.

However, on Android the underlying descriptors are provided using Android's shared memory subsystem - "ashmem". Unlike the /dev/shm descriptors used in the generic POSIX implementation, ashmem descriptors do not allow setting mapping permissions on a per-descriptor basis, but rather enforce memory protections on the underlying shared memory region using a special ioctl - ASHMEM_SET_PROT_MASK (http://androidxref.com/kernel_3.18/xref/drivers/staging/android/ashmem.c#763).

The above ioctl can be issued on an opened ashmem descriptor to set the "protection mask" associated with the underlying memory region. Subsequent mmap's are compared against the current protection mask to ensure that they are allowed. Permissions can only be dropped from the mask, but never added.

Here is a snippet from the Android-specific implementation of base::SharedMemory (https://cs.chromium.org/chromium/src/base/memory/shared_memory_android.cc?l=20):

bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
  ...

  int fd = ashmem_create_region(
      options.name_deprecated == NULL ? "" : options.name_deprecated->c_str(),
      options.size);

  ...

  int err = ashmem_set_prot_region(shm_.GetHandle(),
                                   PROT_READ | PROT_WRITE | PROT_EXEC);
 
  ...
 
  // Android doesn't appear to have a way to drop write access on an ashmem
  // segment for a single descriptor.   http://crbug.com/320865 
  readonly_shm_ = shm_.Duplicate();
  ...
} 

As can be seen above (and as noted in the above comment), since ashmem doesn't allow setting permissions on a per-descriptor basis, this operation is simply skipped on Android, setting the "read-only" copy of the descriptor to a duplicate of the original descriptor, with the same (permissive) protection mask. Android's base::SharedMemory uses the POSIX-specific implementation of GetReadOnlyHandle, which simply returns the "read-only" descriptor above. Therefore, calling base::SharedMemory::GetReadOnlyHandle() on Android produces an ashmem descriptor which can be mapped as writable.

This issue is somewhat similar to the one discovered by James Forshaw (https://googleprojectzero.blogspot.com/2014/10/did-man-with-no-name-feel-insecure.html). However, in this case user scripts cannot be used to exploit the issue, as Chromium on Android does not support extensions (and therefore doesn't include user scripts).

Nonetheless, some resources are shared on an inter-process boundary and are reliant on the fact that they are made read-only be calling GetReadOnlyHandle(). For example, field trials are shared from the Zygote process to all processes (privileged and non-privileged), by sharing a read-only descriptor acquired via GetReadOnlyHandle(). On Android, this allows unprivileged processes (i.e. an unprivileged renderer) to remap the field trials buffer as writable, injecting new trials and parameter values to other (privileged) processes.

Similarly, Mojo relies on GetReadOnlyHandle() to produce read-only memory handles in several different bindings -- for example, the C++ bindings allow the creation of a read-only handle by supplying the mojo::SharedBufferHandle::AccessMode::READ_ONLY flag to SharedBufferHandle::Clone (see https://cs.chromium.org/chromium/src/mojo/public/cpp/system/buffer.cc?type=cs&l=26).

This is used by several Mojo services to create read-only handles that can be safely sent to their clients, such as the visited links hash table managed by the browser process (shared to renderers via visitedlink::mojom::VisitedLinkNotificationSink), the status of current sensors (via device::mojom::SensorProvider), the current cursor location, etc.

VERSION
Chrome Version: 64.0.3242.0 
Revision: e325c0c757496334161119ec57db960b8e792375
Operating System: Android 8.0.0 AOSP on msm8996

REPRODUCTION CASE
I'm attaching a patch which maps the field trials descriptor as writable and corrupts the PersistentMemoryAllocator's cookie from the renderer process. Applying the patch and opening a webpage should result in the following logcat output:

11-30 13:52:36.225 29571 29586 D SharedMemoryPoC: Corrupting cookie (was: 408305DC)
...
11-30 13:52:36.891 29608 29627 I chromium: [INFO:library_loader_hooks.cc(36)] Chromium logging enabled: level = 0, default verbosity = 0
11-30 13:52:36.911 29608 29627 E chromium: [ERROR:persistent_memory_allocator.cc(857)] Corruption detected in shared-memory segment
...

(where 29571 is the pid of the renderer process, and 29608 is the pid of the privileged (unsandboxed) process).


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.
 
corrupt_cookie.diff
1.4 KB Download

Comment 1 by palmer@chromium.org, Nov 30 2017

Cc: tsepez@chromium.org roc...@chromium.org jam@chromium.org
Components: Internals>Core Internals>Mojo
Labels: Security_Impact-Stable OS-Android Pri-1
Owner: rsesek@chromium.org
Status: Assigned (was: Unconfirmed)
rsesek: Can you please triage this further? (Are you the right person to fix it? Can it be fixed without a change in Android?)

Comment 2 by rsesek@chromium.org, Nov 30 2017

Cc: michaelbai@chromium.org klo...@chromium.org digit@chromium.org
I just filed http://b/69973388 with Android as a feature request to support per-descriptor protections on ashmem segments (which we should have done ages ago). Realistically, even if that were fixed in P, we won't be able to make use of it immediately.

I'm inclined for us to just drop the ashmem shared memory implementation and use the temporary file mechanism provided by shared_memory_posix.cc. I wanted to understand why that wasn't done initially, so here's some archaeology:

shared_memory_android.cc was introduced at https://codereview.chromium.org/7184032, which upstreamed the implementation from the internal Chrome-Android fork. The internal implementation was initially landed at https://chromereviews.googleplex.com/1479004/, which says it was based on the WIP at https://chromereviews.googleplex.com/1464003/. There were some minor follow-up internal changes, including https://chrome-internal-review.googlesource.com/c/180, but I can't find any explanation for why ashmem was used instead of the common POSIX implementation. I did find this thread https://groups.google.com/a/google.com/d/topic/android-native/z4T6o_TYy04/discussion with an interesting exchange:

>> I don't know the history of this, but Chrome already runs on Linux, so why can't
>> they just use whatever mechanism they already use there?
>
> Because we don't provide these APIs in the C library because they are
> ill-designed (they rely on global kernel resources that cannot be reclaimed
> sanely when a process is forced-kill). Ashmem is the only reasonable way to
> share memory segments in Android, without risking filling up kernel global
> tables that then require a reboot to fix it.

But that sounds like it's referring to SysV shmem, which is not how the current Chromium POSIX shmem works, so I'm not sure that is relevant. It should also be noted that ashmem is still not a public interface (http://b/5995644 is about exposing it to the NDK), so maybe that's a further argument for switching off it.

The other option to explore, and for which an implementation already exists on OS_CHROMEOS, is to use memfd_create(). However there is no Bionic wrapper for that syscall, and it was introduced relatively recently (kernel 3.17), so we couldn't use it everywhere. It is on the platform's seccomp whitelist, though, so it is ostensibly safe to call.

klobag/digit/michaelbai: Can you think of any reason to not switch to using the shared_memory_posix.cc implementation instead of ashmem on Android?

Comment 3 by raymes@chromium.org, Nov 30 2017

rsesek: Do  you have an estimate of the severity of this issue? (https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md)

Comment 4 by rsesek@chromium.org, Nov 30 2017

Labels: Security_Severity-Critical
I'm inclined to mark this as Critical because it permits "memory corruption in the browser process controllable by a malicious web site." Through the field trail descriptor, I think a website could even enable features that are disabled by default (and don't have security scrutiny because we consider them non-shipping).

Comment 5 by digit@chromium.org, Dec 1 2017

Thanks for putting me on cc of this issue. Here is a little more information:

* We cannot used shared_memory_posix.cc on Android because it relies on /dev/shm, which it typically not available / enabled by the kernel, and because Sys V IPC are leaky by design: the resources must be explicitly released by the processes that created/used them, which never happens when they are killed (as commonly happens on Android).

That's why Ashmem was designed as a replacement. It is the only supported mechanism to provide shared memory mappings that are not backed by the file system (and the latter sucks for other obvious reasons).

* The typical way to ensure safe sharing of read-only Ashmem regions is to do the following:

  1) Create the region with a given size, this gives you an fd.

  2) Map it in the current process with read+write options.

  3) Change the region's protection to read-only. From then on,
     all file descriptors for it are read-only. The mapping in 2)
     is still read-write through.

  4) Pass the fd to another process, it won't (and *should* not be able)
     to map it with write permissions at all.

Also, as soon as you receive the fd and have mapped it read-only, you can close it (the mapping will survive).

It looks like the problem is that Chrome keeps a list of global file descriptors with read+write protection, and expects to be able to duplicate them *later* as read-only. This simply cannot work with Ashmem.

I suggest an alternative where we keep a global list of shareable Memory Mappings. The default implementation could be based on the existing base::GlobalFileDescriptors, but for Android, would use (Memory-Mappings, read-only file descriptors) tuples instead. With only the mappings from the browser process having read+write permission.

* Finally, Ashmem is now officially supported by the Android NDK, so it's very very unlikely that they will change its implementation, especially since there is already a way to share mappings safely: https://developer.android.com/ndk/reference/group___memory.html

In other words, I suggest to abandon the Android bug, and change the way we handle shared memory segments. I can work on that if you want, let me know :)

Project Member

Comment 6 by sheriffbot@chromium.org, Dec 1 2017

Labels: M-62
Project Member

Comment 7 by sheriffbot@chromium.org, Dec 1 2017

Labels: ReleaseBlock-Beta
This is a critical security issue. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 8 by sheriffbot@chromium.org, Dec 1 2017

Labels: -Pri-1 Pri-0

Comment 9 by digit@chromium.org, Dec 1 2017

I'm currently looking into this. This won't be easy, since SharedMemoryHandle seems to have weird copying semantics (instead of begin a move-only type with strong ownership semantics). There is even a bug to deal with that: https://crbug.com/640840

Also, the current SharedMemory API does weird things with regards to handles, probably due to the issue above :-/

I propose to work on all the client code that uses base::GlobalDescriptors to get FDs for file mapping, and replace that with another base::GlobalMappings or something similar. Hopefully this should take care of most of the security issue for Android. The long-term cleanup will take a lot more time it seems...
Blockedon: 320865
Cc: rsesek@chromium.org
Owner: digit@chromium.org
Thanks for hopping on this digit@! We just chatted and he thinks there's a path forward by making some (minor?) changes to the base API, though it's unclear if the general plan will work for all clients. And it will necessitate changes to how the GlobalDescriptor stuff works as noted in #9.

The general idea is to add a new SetReadOnly() method to SharedMemory that drops the permissions and makes it safe to share to unprivileged processes. The protections can't be raised after that is done, so the browser will always have to create a read-write handle first, map it, and then drop to make the segment shareable. It can't be done upfront because Map() is an independent operation from Create(). It's unclear if Chrome currently needs to pass any read-write handles to segments after it has already passed one read-only, which may become a sticking point.

Passing ownership on the bug to digit@ who's now working on this. Let me know if I can be of assistance, though.
Cc: awhalley@chromium.org
Is this a Critical/Pri-0, or a High/Pri-1? From the Severity Guidelines:

"""Bugs which would normally be critical severity with unusual mitigating factors may be rated as high severity. For example, renderer sandbox escapes fall into this category as their impact is that of a critical severity bug, but they require the precondition of a compromised renderer."""

Does exploiting this vulnerability require the attacker to have first compromised a renderer? Or can they exploit from normal web contents/JavaScript/et c.?
A first patch has been uploaded here: https://chromium-review.googlesource.com/c/chromium/src/+/805238

I have spent a considerable amount of time this week-end trying to understand how shared memory regions and handles are managed in Chromium. The situation is really messy, due to historical baggage, and I think that properly fixing this will require a radical change on the way all this is performed.

For now, I'm trying to get an Android-specific fix that does the least amount of badness, but I'll write a shared-memory-region redesign proposal later.

One core issue is that there is some code in Chromium that assumes it is possible to perform writable mappings on a region once it has been mapped read-only (which is reasonable under Posix and other OSes, but not Android).

Cc: lizeb@chromium.org
Cc: agrieve@chromium.org
I still can't find why Chrome is crashing with my latest patch. I need to leave the office now, and will try to work on this tomorrow (Paris time). Feel free to experiment in the meantime.

Here's a link to my proposal for a long-term fix of the situation, feel free to comment (not completely finished, but major ideas are here):

https://docs.google.com/a/google.com/document/d/1eiECEj0x8_VFqkdf61zjmh061rgdkfvIOUxle-q0AIE/edit?usp=sharing


Labels: -Security_Severity-Critical Security_Severity-High
Re: #12: I thought about this today and I think High is probably right for the reported issue. But I'm not totally convinced that there's not any blob-like object that we send from the browser>renderer and hand off to something JS-accessible.
Hello, I suspect that the issue can also exist on Linux / ChromeOS as well, since the base::SharedMemory::Create() method has something like:

    bool result = false;
#if defined(__NR_memfd_create)
    result = CreateMemFDSharedMemory(options, &fd, &readonly_fd, &path);
#endif  // defined(__NR_memfd_create)
    if (!result)
      result = CreateAnonymousSharedMemory(options, &fd, &readonly_fd, &path);
    if (!result)
      return false;

And the CreateMemFDSharedMemory() functions as the following fragment:

  if (options.share_read_only) {
    // memfd anonymous files do not support dropping write access for a
    // single fd.
    int fd_read_only = HANDLE_EINTR(dup(fd->get()));
    readonly_fd->reset(fd_read_only);
    if (!readonly_fd->is_valid()) {
      DPLOG(ERROR) << "memfd(duplicate) failed";
      fd->reset();
      return false;
    }
  }

If I read that correctly, it means memfd-based regions cannot have a read-only file descriptor, just like Ashmem ones. And fcntl() can be used to "seal" the region (e.g. make it read-only) immediately [1]

It looks like that the sealing model is even stricter than the Android one: with Ashmem, any existing read-write mapping of the region survives, while with memfd, sealing requires all such mapping to be removed first!!

I don't see any use of sealing in the Chromium code though. It looks like memfd-based regions are just as open to corruptions as Ashmem ones are.

A simple fix would be to remove memfd-based support though. For Android, there is no corresponding alternative :-/

[1] http://man7.org/linux/man-pages/man2/fcntl.2.html
For the record, a potential fix for the memfd issue:
https://chromium-review.googlesource.com/#/c/chromium/src/+/809014

Wow, thanks for catching that too. I just filed  issue 792117  to track it, since we'll want to merge a fix for it to M64, and this bug may have different remediation.
Project Member

Comment 21 by sheriffbot@chromium.org, Dec 7 2017

Labels: -M-62 M-63
Project Member

Comment 22 by sheriffbot@chromium.org, Dec 20 2017

digit: 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

Comment 23 by digit@chromium.org, Dec 21 2017

Status: Started (was: Assigned)
Issue is nearly fixed with working patch here: https://chromium-review.googlesource.com/c/chromium/src/+/805238

Just waiting for a few more OWNERS approvals to get through.
Project Member

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

Comment 25 by digit@chromium.org, Jan 23 2018

Status: Fixed (was: Started)
Cc: mattcary@chromium.org alexilin@chromium.org
Labels: -M-63 M-65 Merge-Request-65
This just missed M65, so we should merge it.
Cc: gov...@chromium.org
govind@ - good for 65
Labels: -Merge-Request-65 Merge-Approved-65
Approving merge to M65 branch 3325 based on commenet #28.
Friendly reminder to please merge to M65 branch 3325 soon.
Cc: cma...@chromium.org
+cmasso@ as FYI, I approved M65 merge based on comment #28 (didn't realize it was for Android, sorry).
Cc: jdmuys@chromium.org
Reminder to please merge to M65 branch 3325.
Project Member

Comment 34 by sheriffbot@chromium.org, Feb 8

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

Comment 35 by bugdroid1@chromium.org, Feb 8

Labels: -merge-approved-65 merge-merged-3325
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

Labels: -ReleaseBlock-Beta
Hi,

The 90-day period for this vulnerability is due to expire on February 28 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.
The fix has been merged to M65, which is currently on Chrome Beta and is slated to be promoted to stable on March 6. So we would like to exercise the grace period.

Thanks!
No problem! I'll update our tracker.
By the way, has a CVE been assigned to this issue? If not, would it be possible to assign one?

Thanks!
Labels: CVE-2018-6057
We usually assign CVEs when the fix is released in the stable channel, but if you need it sooner, this is CVE-2018-6057
Labels: Release-0-M65
Labels: CVE_description-missing
Project Member

Comment 44 by sheriffbot@chromium.org, May 1

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