Skip to content

Commit

Permalink
Implement SharedMemory::ShareReadOnlyToProcess().
Browse files Browse the repository at this point in the history 
This avoids potential security holes where the renderer could be exploited and
then write into space shared by other renderers or even the browser.

I've done this on Posix by opening both a read/write and read-only file descriptor to the same file. Then ShareReadOnlyToProcess dup()s the read-only descriptor instead of the read/write one. It's an error to try to ShareReadOnly from a SharedMemory that was created from a single SharedMemoryHandle.

The test checks that operations strictly through the file handle can't get
write access to the memory. On Linux there's still a hole through /dev/fd
in the filesystem, but jln@ assures me that the sandbox prevents the
filesystem-based attack. We should eventually write an explicit test for this.

Android needs http://crbug.com/320865 figured out.

BUG=302724,320865

Review URL: https://codereview.chromium.org/27265002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@236347 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
jyasskin@chromium.org committed Nov 20, 2013
1 parent 0c9869d commit 5f58ada
Show file tree
Hide file tree
Showing 7 changed files with 238 additions and 42 deletions.
48 changes: 43 additions & 5 deletions base/memory/shared_memory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

#if defined(OS_POSIX)
#include "base/file_descriptor_posix.h"
#include "base/file_util.h"
#endif

namespace base {
Expand Down Expand Up @@ -80,6 +81,11 @@ class BASE_EXPORT SharedMemory {

// Create a new SharedMemory object from an existing, open
// shared memory file.
//
// WARNING: This does not reduce the OS-level permissions on the handle; it
// only affects how the SharedMemory will be mmapped. Use
// ShareReadOnlyToProcess to drop permissions. TODO(jln,jyasskin): DCHECK
// that |read_only| matches the permissions of the handle.
SharedMemory(SharedMemoryHandle handle, bool read_only);

// Create a new SharedMemory object from an existing, open
Expand Down Expand Up @@ -189,15 +195,41 @@ class BASE_EXPORT SharedMemory {
// It is safe to call Close repeatedly.
void Close();

// Shares the shared memory to another process. Attempts to create a
// platform-specific new_handle which can be used in a remote process to read
// the shared memory file. new_handle is an output parameter to receive the
// handle for use in the remote process.
//
// |*this| must have been initialized using one of the Create*() or Open()
// methods. If it was constructed from a SharedMemoryHandle, this call will
// CHECK-fail.
//
// Returns true on success, false otherwise.
bool ShareReadOnlyToProcess(ProcessHandle process,
SharedMemoryHandle* new_handle) {
return ShareToProcessCommon(process, new_handle, false, SHARE_READONLY);
}

// Logically equivalent to:
// bool ok = ShareReadOnlyToProcess(process, new_handle);
// Close();
// return ok;
// Note that the memory is unmapped by calling this method, regardless of the
// return value.
bool GiveReadOnlyToProcess(ProcessHandle process,
SharedMemoryHandle* new_handle) {
return ShareToProcessCommon(process, new_handle, true, SHARE_READONLY);
}

// Shares the shared memory to another process. Attempts
// to create a platform-specific new_handle which can be
// used in a remote process to access the shared memory
// file. new_handle is an ouput parameter to receive
// file. new_handle is an output parameter to receive
// the handle for use in the remote process.
// Returns true on success, false otherwise.
bool ShareToProcess(ProcessHandle process,
SharedMemoryHandle* new_handle) {
return ShareToProcessCommon(process, new_handle, false);
return ShareToProcessCommon(process, new_handle, false, SHARE_CURRENT_MODE);
}

// Logically equivalent to:
Expand All @@ -208,7 +240,7 @@ class BASE_EXPORT SharedMemory {
// return value.
bool GiveToProcess(ProcessHandle process,
SharedMemoryHandle* new_handle) {
return ShareToProcessCommon(process, new_handle, true);
return ShareToProcessCommon(process, new_handle, true, SHARE_CURRENT_MODE);
}

// Locks the shared memory.
Expand All @@ -232,19 +264,25 @@ class BASE_EXPORT SharedMemory {

private:
#if defined(OS_POSIX) && !defined(OS_NACL)
bool PrepareMapFile(FILE *fp);
bool PrepareMapFile(file_util::ScopedFILE fp, file_util::ScopedFD readonly);
bool FilePathForMemoryName(const std::string& mem_name, FilePath* path);
void LockOrUnlockCommon(int function);
#endif
enum ShareMode {
SHARE_READONLY,
SHARE_CURRENT_MODE,
};
bool ShareToProcessCommon(ProcessHandle process,
SharedMemoryHandle* new_handle,
bool close_self);
bool close_self,
ShareMode);

#if defined(OS_WIN)
std::wstring name_;
HANDLE mapped_file_;
#elif defined(OS_POSIX)
int mapped_file_;
int readonly_mapped_file_;
ino_t inode_;
#endif
size_t mapped_size_;
Expand Down
9 changes: 9 additions & 0 deletions base/memory/shared_memory_android.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
DLOG(ERROR) << "Error " << err << " when setting protection of ashmem";
return false;
}

// 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_mapped_file_ = dup(mapped_file_);
if (-1 == readonly_mapped_file_) {
DPLOG(ERROR) << "dup() failed";
return false;
}

requested_size_ = options.size;

return true;
Expand Down
8 changes: 7 additions & 1 deletion base/memory/shared_memory_nacl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,13 @@ void SharedMemory::Unlock() {

bool SharedMemory::ShareToProcessCommon(ProcessHandle process,
SharedMemoryHandle *new_handle,
bool close_self) {
bool close_self,
ShareMode share_mode) {
if (share_mode == SHARE_READONLY) {
// Untrusted code can't create descriptors or handles, which is needed to
// drop permissions.
return false;
}
const int new_fd = dup(mapped_file_);
if (new_fd < 0) {
DPLOG(ERROR) << "dup() failed.";
Expand Down
110 changes: 80 additions & 30 deletions base/memory/shared_memory_posix.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@
#include "third_party/ashmem/ashmem.h"
#endif

using file_util::ScopedFD;
using file_util::ScopedFILE;

namespace base {

namespace {
Expand All @@ -40,6 +43,7 @@ LazyInstance<Lock>::Leaky g_thread_lock_ = LAZY_INSTANCE_INITIALIZER;

SharedMemory::SharedMemory()
: mapped_file_(-1),
readonly_mapped_file_(-1),
inode_(0),
mapped_size_(0),
memory_(NULL),
Expand All @@ -49,6 +53,7 @@ SharedMemory::SharedMemory()

SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
: mapped_file_(handle.fd),
readonly_mapped_file_(-1),
inode_(0),
mapped_size_(0),
memory_(NULL),
Expand All @@ -65,6 +70,7 @@ SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only,
ProcessHandle process)
: mapped_file_(handle.fd),
readonly_mapped_file_(-1),
inode_(0),
mapped_size_(0),
memory_(NULL),
Expand Down Expand Up @@ -92,7 +98,7 @@ SharedMemoryHandle SharedMemory::NULLHandle() {
// static
void SharedMemory::CloseHandle(const SharedMemoryHandle& handle) {
DCHECK_GE(handle.fd, 0);
if (HANDLE_EINTR(close(handle.fd)) < 0)
if (close(handle.fd) < 0)
DPLOG(ERROR) << "close";
}

Expand Down Expand Up @@ -124,21 +130,30 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
// and be deleted before they ever make it out to disk.
base::ThreadRestrictions::ScopedAllowIO allow_io;

FILE *fp;
ScopedFILE fp;
bool fix_size = true;
int readonly_fd_storage = -1;
ScopedFD readonly_fd(&readonly_fd_storage);

FilePath path;
if (options.name == NULL || options.name->empty()) {
// It doesn't make sense to have a open-existing private piece of shmem
DCHECK(!options.open_existing);
// Q: Why not use the shm_open() etc. APIs?
// A: Because they're limited to 4mb on OS X. FFFFFFFUUUUUUUUUUU
fp = file_util::CreateAndOpenTemporaryShmemFile(&path, options.executable);
fp.reset(
file_util::CreateAndOpenTemporaryShmemFile(&path, options.executable));

// Deleting the file prevents anyone else from mapping it in (making it
// private), and prevents the need for cleanup (once the last fd is closed,
// it is truly freed).
if (fp) {
// Also open as readonly so that we can ShareReadOnlyToProcess.
*readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
if (*readonly_fd < 0) {
DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
fp.reset();
}
// Deleting the file prevents anyone else from mapping it in (making it
// private), and prevents the need for cleanup (once the last fd is
// closed, it is truly freed).
if (unlink(path.value().c_str()))
PLOG(WARNING) << "unlink";
}
Expand Down Expand Up @@ -175,32 +190,35 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
sb.st_uid != effective_uid)) {
LOG(ERROR) <<
"Invalid owner when opening existing shared memory file.";
HANDLE_EINTR(close(fd));
close(fd);
return false;
}

// An existing file was opened, so its size should not be fixed.
fix_size = false;
}
fp = NULL;

// Also open as readonly so that we can ShareReadOnlyToProcess.
*readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
if (*readonly_fd < 0) {
DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
close(fd);
fd = -1;
}
if (fd >= 0) {
// "a+" is always appropriate: if it's a new file, a+ is similar to w+.
fp = fdopen(fd, "a+");
fp.reset(fdopen(fd, "a+"));
}
}
if (fp && fix_size) {
// Get current size.
struct stat stat;
if (fstat(fileno(fp), &stat) != 0) {
file_util::CloseFile(fp);
if (fstat(fileno(fp.get()), &stat) != 0)
return false;
}
const size_t current_size = stat.st_size;
if (current_size != options.size) {
if (HANDLE_EINTR(ftruncate(fileno(fp), options.size)) != 0) {
file_util::CloseFile(fp);
if (HANDLE_EINTR(ftruncate(fileno(fp.get()), options.size)) != 0)
return false;
}
}
requested_size_ = options.size;
}
Expand All @@ -221,7 +239,7 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
return false;
}

return PrepareMapFile(fp);
return PrepareMapFile(fp.Pass(), readonly_fd.Pass());
}

// Our current implementation of shmem is with mmap()ing of files.
Expand All @@ -247,8 +265,14 @@ bool SharedMemory::Open(const std::string& name, bool read_only) {
read_only_ = read_only;

const char *mode = read_only ? "r" : "r+";
FILE *fp = file_util::OpenFile(path, mode);
return PrepareMapFile(fp);
ScopedFILE fp(file_util::OpenFile(path, mode));
int readonly_fd_storage = -1;
ScopedFD readonly_fd(&readonly_fd_storage);
*readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
if (*readonly_fd < 0) {
DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
}
return PrepareMapFile(fp.Pass(), readonly_fd.Pass());
}

#endif // !defined(OS_ANDROID)
Expand Down Expand Up @@ -305,10 +329,15 @@ void SharedMemory::Close() {
Unmap();

if (mapped_file_ > 0) {
if (HANDLE_EINTR(close(mapped_file_)) < 0)
if (close(mapped_file_) < 0)
PLOG(ERROR) << "close";
mapped_file_ = -1;
}
if (readonly_mapped_file_ > 0) {
if (close(readonly_mapped_file_) < 0)
PLOG(ERROR) << "close";
readonly_mapped_file_ = -1;
}
}

void SharedMemory::Lock() {
Expand All @@ -322,18 +351,28 @@ void SharedMemory::Unlock() {
}

#if !defined(OS_ANDROID)
bool SharedMemory::PrepareMapFile(FILE *fp) {
bool SharedMemory::PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd) {
DCHECK_EQ(-1, mapped_file_);
if (fp == NULL) return false;
DCHECK_EQ(-1, readonly_mapped_file_);
if (fp == NULL || *readonly_fd < 0) return false;

// This function theoretically can block on the disk, but realistically
// the temporary files we create will just go into the buffer cache
// and be deleted before they ever make it out to disk.
base::ThreadRestrictions::ScopedAllowIO allow_io;

file_util::ScopedFILE file_closer(fp);
struct stat st = {};
struct stat readonly_st = {};
if (fstat(fileno(fp.get()), &st))
NOTREACHED();
if (fstat(*readonly_fd, &readonly_st))
NOTREACHED();
if (st.st_dev != readonly_st.st_dev || st.st_ino != readonly_st.st_ino) {
LOG(ERROR) << "writable and read-only inodes don't match; bailing";
return false;
}

mapped_file_ = dup(fileno(fp));
mapped_file_ = dup(fileno(fp.get()));
if (mapped_file_ == -1) {
if (errno == EMFILE) {
LOG(WARNING) << "Shared memory creation failed; out of file descriptors";
Expand All @@ -342,11 +381,8 @@ bool SharedMemory::PrepareMapFile(FILE *fp) {
NOTREACHED() << "Call to dup failed, errno=" << errno;
}
}

struct stat st;
if (fstat(mapped_file_, &st))
NOTREACHED();
inode_ = st.st_ino;
readonly_mapped_file_ = *readonly_fd.release();

return true;
}
Expand Down Expand Up @@ -399,9 +435,23 @@ void SharedMemory::LockOrUnlockCommon(int function) {
}

bool SharedMemory::ShareToProcessCommon(ProcessHandle process,
SharedMemoryHandle *new_handle,
bool close_self) {
const int new_fd = dup(mapped_file_);
SharedMemoryHandle* new_handle,
bool close_self,
ShareMode share_mode) {
int handle_to_dup = -1;
switch(share_mode) {
case SHARE_CURRENT_MODE:
handle_to_dup = mapped_file_;
break;
case SHARE_READONLY:
// We could imagine re-opening the file from /dev/fd, but that can't make
// it readonly on Mac: https://codereview.chromium.org/27265002/#msg10
CHECK(readonly_mapped_file_ >= 0);
handle_to_dup = readonly_mapped_file_;
break;
}

const int new_fd = dup(handle_to_dup);
if (new_fd < 0) {
DPLOG(ERROR) << "dup() failed.";
return false;
Expand Down
Loading

0 comments on commit 5f58ada

Please sign in to comment.