Issue metadata
Sign in to add a comment
|
shared_memory_posix.cc memfd_create does not support read-only segments |
||||||||||||||||||||||
Issue descriptionWhile working issue 789959 , digit@ noticed a security issue in the shared_memory_posix.cc implementation with memfd_create: it does not support creating read-only handles, per this comment: 110 if (options.share_read_only) { 111 // memfd anonymous files do not support dropping write access for a 112 // single fd. 113 int fd_read_only = HANDLE_EINTR(dup(fd->get())); 114 readonly_fd->reset(fd_read_only); 115 if (!readonly_fd->is_valid()) { 116 DPLOG(ERROR) << "memfd(duplicate) failed"; 117 fd->reset(); 118 return false; 119 } 120 } 121 return true; This security issue could allow compromised low-privileged processes (e.g. renderers) to overwrite and corrupt data in the browser process. Any memfd_create-backed shared memory needs to properly use fcntl F_ADD_SEALS to lock down the region. The issue is similar to bug 789959 and ashmem in that protections aren't associated with descriptors/handles but with the actual segment. Because this new memfd_implementation was very recently added in https://chromium-review.googlesource.com/c/chromium/src/+/781160 I think we should just revert it. We will also need to merge the revert to M64.
,
Dec 5 2017
You don't have to fully revert. Just change it so |try_memfd_create| stays as false.
,
Dec 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/60e1e0e9381d9965ee316752eaf3a239a764b03f commit 60e1e0e9381d9965ee316752eaf3a239a764b03f Author: David 'Digit' Turner <digit@google.com> Date: Tue Dec 05 18:57:50 2017 Disable memfd base::SharedMemory implementation. Since there is no way to make read-only file descriptors from Linux memfd-based regions, the security guarantees of GetReadOnlyHandle() cannot be maintained with this implementation. Remove it to fall-back on traditional Posix shared regions instead. Bug: 792117 , 736452 Change-Id: Ie5eb41fc3c4dd02ebdbb77be8375363ba51f1b00 Reviewed-on: https://chromium-review.googlesource.com/809014 Reviewed-by: Mark Mentovai <mark@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Commit-Queue: David Turner <digit@chromium.org> Cr-Commit-Position: refs/heads/master@{#521768} [modify] https://crrev.com/60e1e0e9381d9965ee316752eaf3a239a764b03f/base/memory/shared_memory_posix.cc [modify] https://crrev.com/60e1e0e9381d9965ee316752eaf3a239a764b03f/base/memory/shared_memory_unittest.cc
,
Dec 5 2017
Re: #2: I don't see any discussion on issue 736452 or the review about how read-only should be implemented, which makes me concerned that the flag could just be flipped to true without designing around the security concerns. digit@ is looking at changing the structure of the shared memory classes at https://docs.google.com/document/d/1eiECEj0x8_VFqkdf61zjmh061rgdkfvIOUxle-q0AIE/edit#, which would make it easier to provide the semantics that memfd uses.
,
Dec 5 2017
Sorry about the extra work on reverting this! Regarding read-only: - I don't think this can be done with memfd_create (there's discussion about this in the cl https://chromium-review.googlesource.com/c/chromium/src/+/781160/16/base/memory/shared_memory_posix.cc#103) Applying a SEAL_WRITE disables writing any data to the shared memory object. So the usual case would be: - Invoke memfd_create - Write some data - Apply SEAL_WRITE - Create read only handle - no process can write anymore This is, I think the same case as ashmmem on Android and I, erroneously, assume that it was OK to add the exception. I see that Android has the same problem and has a workaround (based on https://docs.google.com/document/d/1eiECEj0x8_VFqkdf61zjmh061rgdkfvIOUxle-q0AIE) Would something similar be applicable for Linux also?
,
Dec 6 2017
Ashmem is different because any existing writable mapping will survive setting the region read only (only future mmap() are affected). This lets you implement a safe unidirectional writer->reader channel (and the Android system uses this scheme pretty extensively, iirc). But I can see why you thought you could use memfd due to the Android situation (bug percolation, I guess?). So there are really three different models here: Memfd: populate the region once, then seal it and nothing can write to it anymore. Ok if you want to only pass read only buffers once they've been written to and never want to update them. Ashmem: have the writer create a read/write mapping, then seal the region for all others. This is slightly more flexible since you can use that as a unidirectional communication buffer at runtime. Posix: file descriptor access mode determines mapping permissions, so you can map read/write a region any time, and anywhere, as long as you have the right fd. Read-only descriptors are typically sent to unsafe processes / readers. Chromium is designed around the third model. Adapting it to the Ashmem / memfd model requires refactoring a lot of things. I will have a proposal soon. In the meantime, it looks like memfd was introduced to deal with environments where /dev/shm is not available. Maybe falling back to /tmp/ instead would be a solution (unlinking the file just after creation)?
,
Dec 6 2017
Thanks for the Thorough explanation! That aligns with my understanding. I think hopefully after the refractor we can reland using memfd_create, since it works better for VMs in cloud solutions. My proposal is to add a switch to force using /tmp instead of /dev/shm (https://chromium-review.googlesource.com/c/chromium/src/+/810325). The reason is that some environments do have /dev/shm available but with very limited size, so if chrome uses up all the available space until it starts failing back to tmp, then it could cause the VM to misbehave, see https://bugs.chromium.org/p/chromium/issues/detail?id=715363
,
Dec 6 2017
,
Dec 6 2017
,
Dec 7 2017
,
Dec 7 2017
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 7 2017
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 8 2017
,
Dec 8 2017
Hey folks, just checking: did this get reverted on the M64 branch as well? It doesn't look like it did.
,
Dec 8 2017
No, it didn't. I'll do the cherry-pick to M64.
,
Dec 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/44b2c1177059576c9a5c5f9f1eefa9be29d64676 commit 44b2c1177059576c9a5c5f9f1eefa9be29d64676 Author: Robert Sesek <rsesek@chromium.org> Date: Fri Dec 08 18:34:40 2017 Disable memfd base::SharedMemory implementation. Since there is no way to make read-only file descriptors from Linux memfd-based regions, the security guarantees of GetReadOnlyHandle() cannot be maintained with this implementation. Remove it to fall-back on traditional Posix shared regions instead. TBR=digit@google.com (cherry picked from commit 60e1e0e9381d9965ee316752eaf3a239a764b03f) Bug: 792117 , 736452 Change-Id: Ie5eb41fc3c4dd02ebdbb77be8375363ba51f1b00 Reviewed-on: https://chromium-review.googlesource.com/809014 Reviewed-by: Mark Mentovai <mark@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Commit-Queue: David Turner <digit@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#521768} Reviewed-on: https://chromium-review.googlesource.com/817682 Cr-Commit-Position: refs/branch-heads/3282@{#100} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/44b2c1177059576c9a5c5f9f1eefa9be29d64676/base/memory/shared_memory_posix.cc [modify] https://crrev.com/44b2c1177059576c9a5c5f9f1eefa9be29d64676/base/memory/shared_memory_unittest.cc
,
Dec 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1824e5752148268c926f1109ed7e5ef1d937609a commit 1824e5752148268c926f1109ed7e5ef1d937609a Author: David Vallet <dvallet@chromium.org> Date: Thu Dec 14 00:46:08 2017 Add switch to always use temp dir in shared memory instances on Linux builds Modify GetShmemTempDir to check for a runtime switch to always use the temp dir Changes tests in shared_memory_unittest.cc to be parameterized and test also for when /dev/shm is disabled. Bug: 736452 , 792117 Change-Id: Id930ea525980e97c4ab70e37689301370776fc79 Reviewed-on: https://chromium-review.googlesource.com/810325 Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Commit-Queue: David Vallet <dvallet@chromium.org> Cr-Commit-Position: refs/heads/master@{#523959} [modify] https://crrev.com/1824e5752148268c926f1109ed7e5ef1d937609a/base/base_switches.cc [modify] https://crrev.com/1824e5752148268c926f1109ed7e5ef1d937609a/base/base_switches.h [modify] https://crrev.com/1824e5752148268c926f1109ed7e5ef1d937609a/base/files/file_util_posix.cc [modify] https://crrev.com/1824e5752148268c926f1109ed7e5ef1d937609a/base/memory/shared_memory_unittest.cc
,
Mar 15 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
,
Mar 27 2018
,
Mar 27 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by rsesek@chromium.org
, Dec 5 2017