Commit 318fca2d authored by Tom Sepez's avatar Tom Sepez Committed by Commit Bot

Proxy mkdir, rmdir, and unlink subject to permissions.

Network service requires this in some situtations.

Additionally, alphabetize methods and consolidate some logic
into helper methods along the way. As part of the consolidation,
need to mask open() flags before checking "if allowed" and test
for "EFAULT" earlier in some situations.

Require "Create" permission for rename() for consistency.
Use initializer-list for vectors where possible.
Clean up hooks and tests to use bitset creation helper function.

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ide22e759692baf4350b643400619885f1676db15
Reviewed-on: https://chromium-review.googlesource.com/812452
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: default avatarRobert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523255}
parent a37803c9
......@@ -9,23 +9,24 @@
#include "base/sys_info.h"
using sandbox::syscall_broker::BrokerFilePermission;
using sandbox::syscall_broker::MakeBrokerCommandSet;
namespace content {
bool NetworkPreSandboxHook(service_manager::SandboxLinux::Options options) {
sandbox::syscall_broker::BrokerCommandSet command_set;
command_set.set(sandbox::syscall_broker::COMMAND_ACCESS);
command_set.set(sandbox::syscall_broker::COMMAND_OPEN);
command_set.set(sandbox::syscall_broker::COMMAND_READLINK);
command_set.set(sandbox::syscall_broker::COMMAND_RENAME);
command_set.set(sandbox::syscall_broker::COMMAND_STAT);
// TODO(tsepez): FIX THIS.
std::vector<BrokerFilePermission> file_permissions = {
BrokerFilePermission::ReadWriteCreateRecursive("/")};
// TODO(tsepez): remove universal permission under filesytem root.
service_manager::SandboxLinux::GetInstance()->StartBrokerProcess(
command_set, std::move(file_permissions),
MakeBrokerCommandSet({
sandbox::syscall_broker::COMMAND_ACCESS,
sandbox::syscall_broker::COMMAND_MKDIR,
sandbox::syscall_broker::COMMAND_OPEN,
sandbox::syscall_broker::COMMAND_READLINK,
sandbox::syscall_broker::COMMAND_RENAME,
sandbox::syscall_broker::COMMAND_RMDIR,
sandbox::syscall_broker::COMMAND_STAT,
sandbox::syscall_broker::COMMAND_UNLINK,
}),
{BrokerFilePermission::ReadWriteCreateRecursive("/")},
service_manager::SandboxLinux::PreSandboxHook(), options);
return true;
......
......@@ -10,11 +10,11 @@
#include <unistd.h>
#include "base/macros.h"
#include "base/pickle.h"
#include "sandbox/linux/syscall_broker/broker_channel.h"
#include "sandbox/linux/syscall_broker/broker_command.h"
namespace sandbox {
namespace syscall_broker {
class BrokerPermissionList;
......@@ -53,31 +53,52 @@ class BrokerClient {
// doesn't support execute permissions.
int Access(const char* pathname, int mode) const;
// Can be used in place of mkdir().
int Mkdir(const char* path, int mode) const;
// Can be used in place of open().
// The implementation only supports certain white listed flags and will
// return -EPERM on other flags.
int Open(const char* pathname, int flags) const;
// Can be used in place of stat()/stat64().
int Stat(const char* pathname, struct stat* sb);
int Stat64(const char* pathname, struct stat64* sb);
// Can be used in place of Readlink().
int Readlink(const char* path, char* buf, size_t bufsize) const;
// Can be used in place of rename().
int Rename(const char* oldpath, const char* newpath);
int Rename(const char* oldpath, const char* newpath) const;
// Can be used in place of Readlink().
int Readlink(const char* path, char* buf, size_t bufsize);
// Can be used in place of rmdir().
int Rmdir(const char* path) const;
// Can be used in place of stat()/stat64().
int Stat(const char* pathname, struct stat* sb) const;
int Stat64(const char* pathname, struct stat64* sb) const;
// Can be used in place of rmdir().
int Unlink(const char* unlink) const;
private:
int PathOnlySyscall(BrokerCommand syscall_type, const char* pathname) const;
int PathAndFlagsSyscall(BrokerCommand syscall_type,
const char* pathname,
int flags) const;
int PathAndFlagsSyscallReturningFD(BrokerCommand syscall_type,
const char* pathname,
int flags) const;
int StatFamilySyscall(BrokerCommand syscall_type,
const char* pathname,
void* result_ptr,
size_t expected_result_size) const;
ssize_t SendRecvRequest(const base::Pickle& request_pickle,
int recvmsg_flags,
uint8_t* reply_buf,
size_t reply_buf_size,
int* returned_fd) const;
const BrokerPermissionList& broker_permission_list_;
const BrokerChannel::EndPoint ipc_channel_;
const BrokerCommandSet allowed_command_set_;
......
......@@ -18,6 +18,16 @@ bool CommandAccessIsSafe(const BrokerCommandSet& command_set,
filename_to_use);
}
bool CommandMkdirIsSafe(const BrokerCommandSet& command_set,
const BrokerPermissionList& policy,
const char* requested_filename,
const char** filename_to_use) {
return command_set.test(COMMAND_MKDIR) &&
policy.GetFileNameIfAllowedToOpen(requested_filename,
O_RDWR | O_CREAT | O_EXCL,
filename_to_use, nullptr);
}
bool CommandOpenIsSafe(const BrokerCommandSet& command_set,
const BrokerPermissionList& policy,
const char* requested_filename,
......@@ -25,8 +35,10 @@ bool CommandOpenIsSafe(const BrokerCommandSet& command_set,
const char** filename_to_use,
bool* unlink_after_open) {
return command_set.test(COMMAND_OPEN) &&
policy.GetFileNameIfAllowedToOpen(requested_filename, requested_flags,
filename_to_use, unlink_after_open);
policy.GetFileNameIfAllowedToOpen(
requested_filename,
requested_flags & ~kCurrentProcessOpenFlagsMask, filename_to_use,
unlink_after_open);
}
bool CommandReadlinkIsSafe(const BrokerCommandSet& command_set,
......@@ -45,12 +57,24 @@ bool CommandRenameIsSafe(const BrokerCommandSet& command_set,
const char** old_filename_to_use,
const char** new_filename_to_use) {
return command_set.test(COMMAND_RENAME) &&
policy.GetFileNameIfAllowedToOpen(old_filename, O_RDWR,
policy.GetFileNameIfAllowedToOpen(old_filename,
O_RDWR | O_CREAT | O_EXCL,
old_filename_to_use, nullptr) &&
policy.GetFileNameIfAllowedToOpen(new_filename, O_RDWR,
policy.GetFileNameIfAllowedToOpen(new_filename,
O_RDWR | O_CREAT | O_EXCL,
new_filename_to_use, nullptr);
}
bool CommandRmdirIsSafe(const BrokerCommandSet& command_set,
const BrokerPermissionList& policy,
const char* requested_filename,
const char** filename_to_use) {
return command_set.test(COMMAND_RMDIR) &&
policy.GetFileNameIfAllowedToOpen(requested_filename,
O_RDWR | O_CREAT | O_EXCL,
filename_to_use, nullptr);
}
bool CommandStatIsSafe(const BrokerCommandSet& command_set,
const BrokerPermissionList& policy,
const char* requested_filename,
......@@ -60,5 +84,15 @@ bool CommandStatIsSafe(const BrokerCommandSet& command_set,
filename_to_use);
}
bool CommandUnlinkIsSafe(const BrokerCommandSet& command_set,
const BrokerPermissionList& policy,
const char* requested_filename,
const char** filename_to_use) {
return command_set.test(COMMAND_UNLINK) &&
policy.GetFileNameIfAllowedToOpen(requested_filename,
O_RDWR | O_CREAT | O_EXCL,
filename_to_use, nullptr);
}
} // namespace syscall_broker
} // namespace sandbox
......@@ -10,6 +10,7 @@
#include <stdint.h>
#include <bitset>
#include <initializer_list>
namespace sandbox {
namespace syscall_broker {
......@@ -35,18 +36,30 @@ constexpr int kCurrentProcessOpenFlagsMask = O_CLOEXEC;
enum BrokerCommand {
COMMAND_INVALID = 0,
COMMAND_ACCESS,
COMMAND_MKDIR,
COMMAND_OPEN,
COMMAND_READLINK,
COMMAND_RENAME,
COMMAND_RMDIR,
COMMAND_STAT,
COMMAND_STAT64,
COMMAND_UNLINK,
// NOTE: update when adding new commands.
COMMAND_MAX = COMMAND_STAT64
COMMAND_MAX = COMMAND_UNLINK
};
using BrokerCommandSet = std::bitset<COMMAND_MAX + 1>;
// Helper function since std::bitset lacks an initializer list constructor.
inline BrokerCommandSet MakeBrokerCommandSet(
const std::initializer_list<BrokerCommand>& args) {
BrokerCommandSet result;
for (const auto& arg : args)
result.set(arg);
return result;
}
// Helper functions to perform the same permissions test on either side
// (client or broker process) of a broker IPC command. The implementations
// must be safe when called from an async signal handler.
......@@ -56,6 +69,11 @@ bool CommandAccessIsSafe(const BrokerCommandSet& command_set,
int requested_mode, // e.g. F_OK, R_OK, W_OK.
const char** filename_to_use);
bool CommandMkdirIsSafe(const BrokerCommandSet& command_set,
const BrokerPermissionList& policy,
const char* requested_filename,
const char** filename_to_use);
bool CommandOpenIsSafe(const BrokerCommandSet& command_set,
const BrokerPermissionList& policy,
const char* requested_filename,
......@@ -75,11 +93,21 @@ bool CommandRenameIsSafe(const BrokerCommandSet& command_set,
const char** old_filename_to_use,
const char** new_filename_to_use);
bool CommandRmdirIsSafe(const BrokerCommandSet& command_set,
const BrokerPermissionList& policy,
const char* requested_filename,
const char** filename_to_use);
bool CommandStatIsSafe(const BrokerCommandSet& command_set,
const BrokerPermissionList& policy,
const char* requested_filename,
const char** filename_to_use);
bool CommandUnlinkIsSafe(const BrokerCommandSet& command_set,
const BrokerPermissionList& policy,
const char* requested_filename,
const char** filename_to_use);
} // namespace syscall_broker
} // namespace sandbox
......
This diff is collapsed.
......@@ -112,11 +112,31 @@ int BrokerProcess::Access(const char* pathname, int mode) const {
return broker_client_->Access(pathname, mode);
}
int BrokerProcess::Mkdir(const char* path, int mode) const {
RAW_CHECK(initialized_);
return broker_client_->Mkdir(path, mode);
}
int BrokerProcess::Open(const char* pathname, int flags) const {
RAW_CHECK(initialized_);
return broker_client_->Open(pathname, flags);
}
int BrokerProcess::Readlink(const char* path, char* buf, size_t bufsize) const {
RAW_CHECK(initialized_);
return broker_client_->Readlink(path, buf, bufsize);
}
int BrokerProcess::Rename(const char* oldpath, const char* newpath) const {
RAW_CHECK(initialized_);
return broker_client_->Rename(oldpath, newpath);
}
int BrokerProcess::Rmdir(const char* pathname) const {
RAW_CHECK(initialized_);
return broker_client_->Rmdir(pathname);
}
int BrokerProcess::Stat(const char* pathname, struct stat* sb) const {
RAW_CHECK(initialized_);
return broker_client_->Stat(pathname, sb);
......@@ -127,14 +147,9 @@ int BrokerProcess::Stat64(const char* pathname, struct stat64* sb) const {
return broker_client_->Stat64(pathname, sb);
}
int BrokerProcess::Rename(const char* oldpath, const char* newpath) const {
int BrokerProcess::Unlink(const char* pathname) const {
RAW_CHECK(initialized_);
return broker_client_->Rename(oldpath, newpath);
}
int BrokerProcess::Readlink(const char* path, char* buf, size_t bufsize) const {
RAW_CHECK(initialized_);
return broker_client_->Readlink(path, buf, bufsize);
return broker_client_->Unlink(pathname);
}
#if defined(MEMORY_SANITIZER)
......@@ -154,24 +169,6 @@ intptr_t BrokerProcess::SIGSYS_Handler(const sandbox::arch_seccomp_data& args,
return broker_process->Access(reinterpret_cast<const char*>(args.args[0]),
static_cast<int>(args.args[1]));
#endif
#if defined(__NR_open)
case __NR_open:
// http://crbug.com/372840
BROKER_UNPOISON_STRING(reinterpret_cast<const char*>(args.args[0]));
return broker_process->Open(reinterpret_cast<const char*>(args.args[0]),
static_cast<int>(args.args[1]));
#endif
#if defined(__NR_stat)
case __NR_stat:
return broker_process->Stat(reinterpret_cast<const char*>(args.args[0]),
reinterpret_cast<struct stat*>(args.args[1]));
#endif
#if defined(__NR_stat64)
case __NR_stat64:
return broker_process->Stat64(
reinterpret_cast<const char*>(args.args[0]),
reinterpret_cast<struct stat64*>(args.args[1]));
#endif
#if defined(__NR_faccessat)
case __NR_faccessat:
if (static_cast<int>(args.args[0]) != AT_FDCWD)
......@@ -179,30 +176,33 @@ intptr_t BrokerProcess::SIGSYS_Handler(const sandbox::arch_seccomp_data& args,
return broker_process->Access(reinterpret_cast<const char*>(args.args[1]),
static_cast<int>(args.args[2]));
#endif
#if defined(__NR_openat)
case __NR_openat:
if (static_cast<int>(args.args[0]) != AT_FDCWD)
return -EPERM;
return broker_process->Open(reinterpret_cast<const char*>(args.args[1]),
static_cast<int>(args.args[2]));
#if defined(__NR_mkdir)
case __NR_mkdir:
return broker_process->Mkdir(reinterpret_cast<const char*>(args.args[0]),
static_cast<int>(args.args[1]));
#endif
#if defined(__NR_fstatat)
case __NR_fstatat:
#if defined(__NR_mkdirat)
case __NR_mkdirat:
if (static_cast<int>(args.args[0]) != AT_FDCWD)
return -EPERM;
if (static_cast<int>(args.args[3]) != 0)
return -EINVAL;
return broker_process->Stat(reinterpret_cast<const char*>(args.args[1]),
reinterpret_cast<struct stat*>(args.args[2]));
return broker_process->Mkdir(reinterpret_cast<const char*>(args.args[1]),
static_cast<int>(args.args[2]));
#endif
#if defined(__NR_newfstatat)
case __NR_newfstatat:
#if defined(__NR_open)
case __NR_open:
// http://crbug.com/372840
BROKER_UNPOISON_STRING(reinterpret_cast<const char*>(args.args[0]));
return broker_process->Open(reinterpret_cast<const char*>(args.args[0]),
static_cast<int>(args.args[1]));
#endif
#if defined(__NR_openat)
case __NR_openat:
if (static_cast<int>(args.args[0]) != AT_FDCWD)
return -EPERM;
if (static_cast<int>(args.args[3]) != 0)
return -EINVAL;
return broker_process->Stat(reinterpret_cast<const char*>(args.args[1]),
reinterpret_cast<struct stat*>(args.args[2]));
// http://crbug.com/372840
BROKER_UNPOISON_STRING(reinterpret_cast<const char*>(args.args[1]));
return broker_process->Open(reinterpret_cast<const char*>(args.args[1]),
static_cast<int>(args.args[2]));
#endif
#if defined(__NR_readlink)
case __NR_readlink:
......@@ -247,6 +247,52 @@ intptr_t BrokerProcess::SIGSYS_Handler(const sandbox::arch_seccomp_data& args,
return broker_process->Rename(
reinterpret_cast<const char*>(args.args[1]),
reinterpret_cast<const char*>(args.args[3]));
#endif
#if defined(__NR_rmdir)
case __NR_rmdir:
return broker_process->Rmdir(reinterpret_cast<const char*>(args.args[0]));
#endif
#if defined(__NR_stat)
case __NR_stat:
return broker_process->Stat(reinterpret_cast<const char*>(args.args[0]),
reinterpret_cast<struct stat*>(args.args[1]));
#endif
#if defined(__NR_stat64)
case __NR_stat64:
return broker_process->Stat64(
reinterpret_cast<const char*>(args.args[0]),
reinterpret_cast<struct stat64*>(args.args[1]));
#endif
#if defined(__NR_fstatat)
case __NR_fstatat:
if (static_cast<int>(args.args[0]) != AT_FDCWD)
return -EPERM;
if (static_cast<int>(args.args[3]) != 0)
return -EINVAL;
return broker_process->Stat(reinterpret_cast<const char*>(args.args[1]),
reinterpret_cast<struct stat*>(args.args[2]));
#endif
#if defined(__NR_newfstatat)
case __NR_newfstatat:
if (static_cast<int>(args.args[0]) != AT_FDCWD)
return -EPERM;
if (static_cast<int>(args.args[3]) != 0)
return -EINVAL;
return broker_process->Stat(reinterpret_cast<const char*>(args.args[1]),
reinterpret_cast<struct stat*>(args.args[2]));
#endif
#if defined(__NR_unlink)
case __NR_unlink:
return broker_process->Unlink(
reinterpret_cast<const char*>(args.args[0]));
#endif
#if defined(__NR_unlinkat)
case __NR_unlinkat:
// TODO(tsepez): does not support AT_REMOVEDIR flag.
if (static_cast<int>(args.args[0]) != AT_FDCWD)
return -EPERM;
return broker_process->Unlink(
reinterpret_cast<const char*>(args.args[1]));
#endif
default:
RAW_CHECK(false);
......
......@@ -84,20 +84,29 @@ class SANDBOX_EXPORT BrokerProcess {
// doesn't support execute permissions.
int Access(const char* pathname, int mode) const;
// Can be used in place of mkdir().
int Mkdir(const char* path, int mode) const;
// Can be used in place of open()
// The implementation only supports certain white listed flags and will
// return -EPERM on other flags.
int Open(const char* pathname, int flags) const;
// Can be used in place of stat()/stat64().
int Stat(const char* pathname, struct stat* sb) const;
int Stat64(const char* pathname, struct stat64* sb) const;
// Can be used in place of readlink().
int Readlink(const char* path, char* buf, size_t bufsize) const;
// Can be used in place of rename().
int Rename(const char* oldpath, const char* newpath) const;
// Can be used in place of readlink().
int Readlink(const char* path, char* buf, size_t bufsize) const;
// Can be used in place of rmdir().
int Rmdir(const char* path) const;
// Can be used in place of stat()/stat64().
int Stat(const char* pathname, struct stat* sb) const;
int Stat64(const char* pathname, struct stat64* sb) const;
// Can be used in place of unlink().
int Unlink(const char* path) const;
private:
friend class BrokerProcessTestHelper;
......
......@@ -32,6 +32,18 @@ ResultExpr BrokerProcessPolicy::EvaluateSyscall(int sysno) const {
return Allow();
break;
#endif
#if defined(__NR_mkdir)
case __NR_mkdir:
if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_MKDIR))
return Allow();
break;
#endif
#if defined(__NR_mkdirat)
case __NR_mkdirat:
if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_MKDIR))
return Allow();
break;
#endif
#if defined(__NR_open)
case __NR_open:
if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_OPEN))
......@@ -44,16 +56,18 @@ ResultExpr BrokerProcessPolicy::EvaluateSyscall(int sysno) const {
return Allow();
break;
#endif
#if defined(__NR_unlink)
case __NR_unlink:
return Allow();
#endif
#if defined(__NR_rename)
case __NR_rename:
if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_RENAME))
return Allow();
break;
#endif
#if defined(__NR_renameat)
case __NR_renameat:
if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_RENAME))
return Allow();
break;
#endif
#if defined(__NR_stat)
case __NR_stat:
if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_STAT))
......@@ -89,6 +103,28 @@ ResultExpr BrokerProcessPolicy::EvaluateSyscall(int sysno) const {
if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_READLINK))
return Allow();
break;
#endif
#if defined(__NR_rmdir)
case __NR_rmdir:
if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_RMDIR))
return Allow();
break;
#endif
#if defined(__NR_unlink)
case __NR_unlink:
// NOTE: Open() uses unlink() to make "temporary" files.
if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_OPEN) ||
allowed_command_set_.test(sandbox::syscall_broker::COMMAND_UNLINK)) {
return Allow();
}
#endif
#if defined(__NR_unlinkat)
case __NR_unlinkat:
// NOTE: Open() uses unlink() to make "temporary" files.
if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_OPEN) ||
allowed_command_set_.test(sandbox::syscall_broker::COMMAND_UNLINK)) {
return Allow();
}
#endif
default:
break;
......
......@@ -38,15 +38,36 @@ ResultExpr NetworkProcessPolicy::EvaluateSyscall(int sysno) const {
#if defined(__NR_access)
case __NR_access:
#endif
#if defined(__NR_open)
case __NR_open:
#endif
#if defined(__NR_faccessat)
case __NR_faccessat:
#endif
#if defined(__NR_mkdir)
case __NR_mkdir:
#endif
#if defined(__NR_mkdirat)
case __NR_mkdirat:
#endif
#if defined(__NR_open)
case __NR_open:
#endif
#if defined(__NR_openat)
case __NR_openat:
#endif
#if defined(__NR_readlink)
case __NR_readlink:
#endif
#if defined(__NR_readlinkat)
case __NR_readlinkat:
#endif
#if defined(__NR_rmdir)
case __NR_rmdir:
#endif
#if defined(__NR_rename)
case __NR_rename:
#endif
#if defined(__NR_renameat)
case __NR_renameat:
#endif
#if defined(__NR_stat)
case __NR_stat:
#endif
......@@ -59,14 +80,11 @@ ResultExpr NetworkProcessPolicy::EvaluateSyscall(int sysno) const {
#if defined(__NR_newfstatat)
case __NR_newfstatat:
#endif
#if defined(__NR_rename)
case __NR_rename:
#endif
#if defined(__NR_readlink)
case __NR_readlink:
#if defined(__NR_unlink)
case __NR_unlink:
#endif
#if defined(__NR_readlinkat)
case __NR_readlinkat:
#if defined(__NR_unlinkat)
case __NR_unlinkat:
#endif
return Trap(BrokerProcess::SIGSYS_Handler,
SandboxLinux::GetInstance()->broker_process());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment