Skip to content

Commit

Permalink
Kernel: Add a basic implementation of unveil()
Browse files Browse the repository at this point in the history
This syscall is a complement to pledge() and adds the same sort of
incremental relinquishing of capabilities for filesystem access.

The first call to unveil() will "drop a veil" on the process, and from
now on, only unveiled parts of the filesystem are visible to it.

Each call to unveil() specifies a path to either a directory or a file
along with permissions for that path. The permissions are a combination
of the following:

- r: Read access (like the "rpath" promise)
- w: Write access (like the "wpath" promise)
- x: Execute access
- c: Create/remove access (like the "cpath" promise)

Attempts to open a path that has not been unveiled with fail with
ENOENT. If the unveiled path lacks sufficient permissions, it will fail
with EACCES.

Like pledge(), subsequent calls to unveil() with the same path can only
remove permissions, not add them.

Once you call unveil(nullptr, nullptr), the veil is locked, and it's no
longer possible to unveil any more paths for the process, ever.

This concept comes from OpenBSD, and their implementation does various
things differently, I'm sure. This is just a first implementation for
SerenityOS, and we'll keep improving on it as we go. :^)
  • Loading branch information
awesomekling committed Jan 20, 2020
1 parent e711936 commit 0569123
Show file tree
Hide file tree
Showing 7 changed files with 188 additions and 3 deletions.
64 changes: 63 additions & 1 deletion Kernel/FileSystem/VirtualFileSystem.cpp
Expand Up @@ -564,7 +564,7 @@ KResult VFS::link(StringView old_path, StringView new_path, Custody& base)
KResult VFS::unlink(StringView path, Custody& base)
{
RefPtr<Custody> parent_custody;
auto custody_or_error = resolve_path(path, base, &parent_custody, O_NOFOLLOW_NOERROR);
auto custody_or_error = resolve_path(path, base, &parent_custody, O_NOFOLLOW_NOERROR | O_UNLINK_INTERNAL);

This comment has been minimized.

Copy link
@bugaevc

bugaevc Jan 21, 2020

Member

It's kinda strange to introduce a flag for one of the many places that uses resolve_path(), don't you think? Or do all other call sites pass appropriate options already? (Didn't look that way to me last time I checked...)

This comment has been minimized.

Copy link
@awesomekling

awesomekling Jan 21, 2020

Author Member

Yeah, there's definitely a better way to do this. How about a mandatory bitmask argument with r/w/c/x bits that correspond to the rpath/wpath/cpath pledges and r/w/c/x unveil permissions? Then every call to resolve_path() would be forced to explain what kind of access it is.

This comment has been minimized.

Copy link
@bugaevc

bugaevc Jan 21, 2020

Member

That's exactly what options already is (O_RDONLY, O_CREAT, etc.) Maybe make it mandatory then?

My only gripe is that O_RDONLY is defined to be zero, so you have to infer its presence from absence of other flags, which works but is meh. Why do we have to do that? I would prefer the GNU Hurd way, with separate (nonzero) O_READ and O_WRITE, and then O_RDWR defined to O_READ | O_WRITE and so on.

That would straighten things out, and that is essentially the bitmask argument you're thinking about, wdyt?

This comment has been minimized.

Copy link
@bugaevc

bugaevc Jan 21, 2020

Member

Oh, and in that note, UnveiledPath should use the same values, not its own enum. Would actually make it easier to validate one against other without much boilerplate.

if (custody_or_error.is_error())
return custody_or_error.error();
auto& custody = *custody_or_error.value();
Expand Down Expand Up @@ -709,8 +709,70 @@ Custody& VFS::root_custody()
return *m_root_custody;
}

const UnveiledPath* VFS::find_matching_unveiled_path(StringView path)
{
for (auto& unveiled_path : current->process().unveiled_paths()) {
if (path == unveiled_path.path)
return &unveiled_path;
if (path.starts_with(unveiled_path.path) && path.length() > unveiled_path.path.length() && path[unveiled_path.path.length()] == '/')
return &unveiled_path;
}
return nullptr;
}

KResult VFS::validate_path_against_process_veil(StringView path, int options)
{
if (current->process().unveil_state() == UnveilState::None)
return KSuccess;

// FIXME: Figure out a nicer way to do this.
if (String(path).contains("/.."))
return KResult(-EINVAL);

auto* unveiled_path = find_matching_unveiled_path(path);
if (!unveiled_path) {
dbg() << *current << " rejecting path '" << path << "' since it hasn't been unveiled.";
return KResult(-ENOENT);
}

if (options & O_CREAT) {
if (!(unveiled_path->permissions & UnveiledPath::Access::CreateOrRemove)) {
dbg() << *current << " rejecting path '" << path << "' since it hasn't been unveiled with 'c' permission.";
return KResult(-EACCES);
}
}
if (options & O_UNLINK_INTERNAL) {
if (!(unveiled_path->permissions & UnveiledPath::Access::CreateOrRemove)) {
dbg() << *current << " rejecting path '" << path << "' for unlink since it hasn't been unveiled with 'c' permission.";
return KResult(-EACCES);
}
return KSuccess;
}
if ((options & O_RDWR) || (options & O_WRONLY)) {
if (!(unveiled_path->permissions & UnveiledPath::Access::Write)) {
dbg() << *current << " rejecting path '" << path << "' since it hasn't been unveiled with 'w' permission.";
return KResult(-EACCES);
}
} else if (options & O_EXEC) {
if (!(unveiled_path->permissions & UnveiledPath::Access::Execute)) {
dbg() << *current << " rejecting path '" << path << "' since it hasn't been unveiled with 'x' permission.";
return KResult(-EACCES);
}
} else {
if (!(unveiled_path->permissions & UnveiledPath::Access::Read)) {
dbg() << *current << " rejecting path '" << path << "' since it hasn't been unveiled with 'r' permission.";
return KResult(-EACCES);
}
}
return KSuccess;
}

KResultOr<NonnullRefPtr<Custody>> VFS::resolve_path(StringView path, Custody& base, RefPtr<Custody>* out_parent, int options, int symlink_recursion_level)
{
auto result = validate_path_against_process_veil(path, options);
if (result.is_error())
return result;

if (symlink_recursion_level >= symlink_recursion_limit)
return KResult(-ELOOP);

Expand Down
5 changes: 5 additions & 0 deletions Kernel/FileSystem/VirtualFileSystem.h
Expand Up @@ -53,6 +53,7 @@
#define O_CLOEXEC 02000000
#define O_DIRECT 04000000
#define O_NOFOLLOW_NOERROR 0x4000000
#define O_UNLINK_INTERNAL 0x8000000

#define MS_NODEV 1
#define MS_NOEXEC 2
Expand All @@ -62,6 +63,7 @@
class Custody;
class Device;
class FileDescription;
class UnveiledPath;

struct UidAndGid {
uid_t uid;
Expand Down Expand Up @@ -134,6 +136,9 @@ class VFS {
private:
friend class FileDescription;

const UnveiledPath* find_matching_unveiled_path(StringView path);
KResult validate_path_against_process_veil(StringView path, int options);

RefPtr<Inode> get_inode(InodeIdentifier);

bool is_vfs_root(InodeIdentifier) const;
Expand Down
78 changes: 78 additions & 0 deletions Kernel/Process.cpp
Expand Up @@ -1820,6 +1820,10 @@ bool Process::validate(const Syscall::ImmutableBufferArgument<DataType, SizeType

String Process::validate_and_copy_string_from_user(const char* user_characters, size_t user_length) const
{
if (!user_characters)
return {};
if (user_length == 0)
return String::empty();
if (!validate_read(user_characters, user_length))
return {};
SmapDisabler disabler;
Expand Down Expand Up @@ -1933,6 +1937,9 @@ int Process::sys$open(const Syscall::SC_open_params* user_params)
if (options & O_NOFOLLOW_NOERROR)
return -EINVAL;

if (options & O_UNLINK_INTERNAL)
return -EINVAL;

if ((options & O_RDWR) || (options & O_WRONLY))
REQUIRE_PROMISE(wpath);
else
Expand Down Expand Up @@ -4588,3 +4595,74 @@ Region& Process::add_region(NonnullOwnPtr<Region> region)
m_regions.append(move(region));
return *ptr;
}

int Process::sys$unveil(const Syscall::SC_unveil_params* user_params)
{
Syscall::SC_unveil_params params;
if (!validate_read_and_copy_typed(&params, user_params))
return -EFAULT;

if (!params.path.characters && !params.permissions.characters) {
m_unveil_state = UnveilState::VeilLocked;
return 0;
}

if (m_unveil_state == UnveilState::VeilLocked)
return -EPERM;

if (!params.path.characters || !params.permissions.characters)
return -EINVAL;

if (params.permissions.length > 4)
return -EINVAL;

auto path = get_syscall_path_argument(params.path);
if (path.is_error())
return path.error();

if (path.value().is_empty() || path.value().characters()[0] != '/')
return -EINVAL;

auto permissions = validate_and_copy_string_from_user(params.permissions);
if (permissions.is_null())
return -EFAULT;

unsigned new_permissions = 0;
for (size_t i = 0; i < permissions.length(); ++i) {
switch (permissions[i]) {
case 'r':
new_permissions |= UnveiledPath::Access::Read;
break;
case 'w':
new_permissions |= UnveiledPath::Access::Write;
break;
case 'x':
new_permissions |= UnveiledPath::Access::Execute;
break;
case 'c':
new_permissions |= UnveiledPath::Access::CreateOrRemove;
break;
default:
return -EINVAL;
}
}

for (int i = 0; i < m_unveiled_paths.size(); ++i) {
auto& unveiled_path = m_unveiled_paths[i];
if (unveiled_path.path == path.value()) {
if (new_permissions & ~unveiled_path.permissions)
return -EPERM;
if (!new_permissions) {
m_unveiled_paths.remove(i);

This comment has been minimized.

Copy link
@bugaevc

bugaevc Jan 21, 2020

Member

This allows a process to workaround the above check by removing a path and then adding it back.

This comment has been minimized.

Copy link
@awesomekling

awesomekling Jan 21, 2020

Author Member

Duh! Yeah, we should be keeping empty permissions regardless, to allow completely locking out specific subtrees/files.

return 0;
}
unveiled_path.permissions = new_permissions;
return 0;
}
}

m_unveiled_paths.append({ path.value(), new_permissions });
ASSERT(m_unveil_state != UnveilState::VeilLocked);
m_unveil_state = UnveilState::VeilDropped;
return 0;
}
25 changes: 25 additions & 0 deletions Kernel/Process.h
Expand Up @@ -82,6 +82,24 @@ enum class Pledge : u32 {
#undef __ENUMERATE_PLEDGE_PROMISE
};

enum class UnveilState {
None,
VeilDropped,
VeilLocked,
};

struct UnveiledPath {
enum Access {
Read = 1,
Write = 2,
Execute = 4,
CreateOrRemove = 8,
};

String path;
unsigned permissions { 0 };
};

class Process : public InlineLinkedListNode<Process>
, public Weakable<Process> {
friend class InlineLinkedListNode<Process>;
Expand Down Expand Up @@ -282,6 +300,7 @@ class Process : public InlineLinkedListNode<Process>
int sys$set_process_boost(pid_t, int amount);
int sys$chroot(const char* path, size_t path_length, int mount_flags);
int sys$pledge(const Syscall::SC_pledge_params*);
int sys$unveil(const Syscall::SC_unveil_params*);

static void initialize();

Expand Down Expand Up @@ -380,6 +399,9 @@ class Process : public InlineLinkedListNode<Process>
bool has_promises() const { return m_promises; }
bool has_promised(Pledge pledge) const { return m_promises & (1u << (u32)pledge); }

UnveilState unveil_state() const { return m_unveil_state; }
const Vector<UnveiledPath>& unveiled_paths() const { return m_unveiled_paths; }

private:
friend class MemoryManager;
friend class Scheduler;
Expand Down Expand Up @@ -481,6 +503,9 @@ class Process : public InlineLinkedListNode<Process>
u32 m_promises { 0 };
u32 m_execpromises { 0 };

UnveilState m_unveil_state { UnveilState::None };
Vector<UnveiledPath> m_unveiled_paths;

WaitQueue& futex_queue(i32*);
HashMap<u32, OwnPtr<WaitQueue>> m_futex_queues;
};
Expand Down
8 changes: 7 additions & 1 deletion Kernel/Syscall.h
Expand Up @@ -174,7 +174,8 @@ typedef u32 socklen_t;
__ENUMERATE_SYSCALL(set_thread_boost) \
__ENUMERATE_SYSCALL(set_process_boost) \
__ENUMERATE_SYSCALL(chroot) \
__ENUMERATE_SYSCALL(pledge)
__ENUMERATE_SYSCALL(pledge) \
__ENUMERATE_SYSCALL(unveil)

namespace Syscall {

Expand Down Expand Up @@ -385,6 +386,11 @@ struct SC_pledge_params {
StringArgument execpromises;
};

struct SC_unveil_params {
StringArgument path;
StringArgument permissions;
};

void initialize();
int sync();

Expand Down
10 changes: 9 additions & 1 deletion Libraries/LibC/unistd.cpp
Expand Up @@ -694,5 +694,13 @@ int pledge(const char* promises, const char* execpromises)
__RETURN_WITH_ERRNO(rc, rc, -1);
}

int unveil(const char* path, const char* permissions)
{
Syscall::SC_unveil_params params {
{ path, path ? strlen(path) : 0 },
{ permissions, permissions ? strlen(permissions) : 0 }
};
int rc = syscall(SC_unveil, &params);
__RETURN_WITH_ERRNO(rc, rc, -1);
}
}

1 change: 1 addition & 0 deletions Libraries/LibC/unistd.h
Expand Up @@ -141,6 +141,7 @@ int reboot();
int mount(const char* source, const char* target, const char* fs_type, int flags);
int umount(const char* mountpoint);
int pledge(const char* promises, const char* execpromises);
int unveil(const char* path, const char* permissions);

enum {
_PC_NAME_MAX,
Expand Down

2 comments on commit 0569123

@bugaevc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also shouldn't this preserved at least across fork()?

@awesomekling
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also shouldn't this preserved at least across fork()?

Definitely.

Please sign in to comment.