From 39de5b7f82622522a9112f23bd32dbaae5375608 Mon Sep 17 00:00:00 2001 From: Liav A Date: Sat, 4 Mar 2023 20:01:54 +0200 Subject: [PATCH] Kernel: Actually check Process unveil data when creating perfcore dump Before of this patch, we looked at the unveil data of the FinalizerTask, which naturally doesn't have any unveil restrictions, therefore allowing an unveil bypass for a process that enabled performance coredumps. To ensure we always check the dumped process unveil data, an option to pass a Process& has been added to a couple of methods in the class of VirtualFileSystem. --- Kernel/FileSystem/VirtualFileSystem.cpp | 54 ++++++++++++++++++------- Kernel/FileSystem/VirtualFileSystem.h | 7 +++- Kernel/Process.cpp | 2 +- 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/Kernel/FileSystem/VirtualFileSystem.cpp b/Kernel/FileSystem/VirtualFileSystem.cpp index bc73e1f03ff..f7f70ca46ca 100644 --- a/Kernel/FileSystem/VirtualFileSystem.cpp +++ b/Kernel/FileSystem/VirtualFileSystem.cpp @@ -357,17 +357,22 @@ ErrorOr> VirtualFileSystem::find_already } ErrorOr> VirtualFileSystem::open(Credentials const& credentials, StringView path, int options, mode_t mode, Custody& base, Optional owner) +{ + return open(Process::current(), credentials, path, options, mode, base, owner); +} + +ErrorOr> VirtualFileSystem::open(Process const& process, Credentials const& credentials, StringView path, int options, mode_t mode, Custody& base, Optional owner) { if ((options & O_CREAT) && (options & O_DIRECTORY)) return EINVAL; RefPtr parent_custody; - auto custody_or_error = resolve_path(credentials, path, base, &parent_custody, options); + auto custody_or_error = resolve_path(process, credentials, path, base, &parent_custody, options); if (custody_or_error.is_error()) { // NOTE: ENOENT with a non-null parent custody signals us that the immediate parent // of the file exists, but the file itself does not. if ((options & O_CREAT) && custody_or_error.error().code() == ENOENT && parent_custody) - return create(credentials, path, options, mode, *parent_custody, move(owner)); + return create(process, credentials, path, options, mode, *parent_custody, move(owner)); return custody_or_error.release_error(); } @@ -472,11 +477,16 @@ ErrorOr VirtualFileSystem::mknod(Credentials const& credentials, StringVie } ErrorOr> VirtualFileSystem::create(Credentials const& credentials, StringView path, int options, mode_t mode, Custody& parent_custody, Optional owner) +{ + return create(Process::current(), credentials, path, options, mode, parent_custody, owner); +} + +ErrorOr> VirtualFileSystem::create(Process const& process, Credentials const& credentials, StringView path, int options, mode_t mode, Custody& parent_custody, Optional owner) { auto basename = KLexicalPath::basename(path); auto parent_path = TRY(parent_custody.try_serialize_absolute_path()); auto full_path = TRY(KLexicalPath::try_join(parent_path->view(), basename)); - TRY(validate_path_against_process_veil(full_path->view(), options)); + TRY(validate_path_against_process_veil(process, full_path->view(), options)); if (!is_socket(mode) && !is_fifo(mode) && !is_block_device(mode) && !is_character_device(mode)) { // Turn it into a regular file. (This feels rather hackish.) @@ -938,11 +948,10 @@ NonnullRefPtr VirtualFileSystem::root_custody() return m_root_custody.with([](auto& root_custody) -> NonnullRefPtr { return *root_custody; }); } -UnveilNode const& VirtualFileSystem::find_matching_unveiled_path(StringView path) +UnveilNode const& VirtualFileSystem::find_matching_unveiled_path(Process const& process, StringView path) { - auto& current_process = Process::current(); - VERIFY(current_process.veil_state() != VeilState::None); - return current_process.unveil_data().with([&](auto const& unveil_data) -> UnveilNode const& { + VERIFY(process.veil_state() != VeilState::None); + return process.unveil_data().with([&](auto const& unveil_data) -> UnveilNode const& { auto path_parts = KLexicalPath::parts(path); return unveil_data.paths.traverse_until_last_accessible_node(path_parts.begin(), path_parts.end()); }); @@ -950,15 +959,20 @@ UnveilNode const& VirtualFileSystem::find_matching_unveiled_path(StringView path ErrorOr VirtualFileSystem::validate_path_against_process_veil(Custody const& custody, int options) { - if (Process::current().veil_state() == VeilState::None) - return {}; - auto absolute_path = TRY(custody.try_serialize_absolute_path()); - return validate_path_against_process_veil(absolute_path->view(), options); + return validate_path_against_process_veil(Process::current(), custody, options); } -ErrorOr VirtualFileSystem::validate_path_against_process_veil(StringView path, int options) +ErrorOr VirtualFileSystem::validate_path_against_process_veil(Process const& process, Custody const& custody, int options) { - if (Process::current().veil_state() == VeilState::None) + if (process.veil_state() == VeilState::None) + return {}; + auto absolute_path = TRY(custody.try_serialize_absolute_path()); + return validate_path_against_process_veil(process, absolute_path->view(), options); +} + +ErrorOr VirtualFileSystem::validate_path_against_process_veil(Process const& process, StringView path, int options) +{ + if (process.veil_state() == VeilState::None) return {}; VERIFY(path.starts_with('/')); @@ -972,7 +986,7 @@ ErrorOr VirtualFileSystem::validate_path_against_process_veil(StringView p return {}; #endif - auto& unveiled_path = find_matching_unveiled_path(path); + auto& unveiled_path = find_matching_unveiled_path(process, path); if (unveiled_path.permissions() == UnveilAccess::None) { dbgln("Rejecting path '{}' since it hasn't been unveiled.", path); dump_backtrace(); @@ -1026,12 +1040,22 @@ ErrorOr VirtualFileSystem::validate_path_against_process_veil(StringView p return {}; } +ErrorOr VirtualFileSystem::validate_path_against_process_veil(StringView path, int options) +{ + return validate_path_against_process_veil(Process::current(), path, options); +} + ErrorOr> VirtualFileSystem::resolve_path(Credentials const& credentials, StringView path, NonnullRefPtr base, RefPtr* out_parent, int options, int symlink_recursion_level) +{ + return resolve_path(Process::current(), credentials, path, base, out_parent, options, symlink_recursion_level); +} + +ErrorOr> VirtualFileSystem::resolve_path(Process const& process, Credentials const& credentials, StringView path, NonnullRefPtr base, RefPtr* out_parent, int options, int symlink_recursion_level) { // FIXME: The errors returned by resolve_path_without_veil can leak information about paths that are not unveiled, // e.g. when the error is EACCESS or similar. auto custody = TRY(resolve_path_without_veil(credentials, path, base, out_parent, options, symlink_recursion_level)); - if (auto result = validate_path_against_process_veil(*custody, options); result.is_error()) { + if (auto result = validate_path_against_process_veil(process, *custody, options); result.is_error()) { if (out_parent) out_parent->clear(); return result.release_error(); diff --git a/Kernel/FileSystem/VirtualFileSystem.h b/Kernel/FileSystem/VirtualFileSystem.h index 06a8b7abc80..84d2ba37170 100644 --- a/Kernel/FileSystem/VirtualFileSystem.h +++ b/Kernel/FileSystem/VirtualFileSystem.h @@ -61,7 +61,9 @@ public: ErrorOr unmount(Custody& mount_point); ErrorOr> open(Credentials const&, StringView path, int options, mode_t mode, Custody& base, Optional = {}); + ErrorOr> open(Process const&, Credentials const&, StringView path, int options, mode_t mode, Custody& base, Optional = {}); ErrorOr> create(Credentials const&, StringView path, int options, mode_t mode, Custody& parent_custody, Optional = {}); + ErrorOr> create(Process const&, Credentials const&, StringView path, int options, mode_t mode, Custody& parent_custody, Optional = {}); ErrorOr mkdir(Credentials const&, StringView path, mode_t mode, Custody& base); ErrorOr link(Credentials const&, StringView old_path, StringView new_path, Custody& base); ErrorOr unlink(Credentials const&, StringView path, Custody& base); @@ -92,12 +94,15 @@ public: NonnullRefPtr root_custody(); ErrorOr> resolve_path(Credentials const&, StringView path, NonnullRefPtr base, RefPtr* out_parent = nullptr, int options = 0, int symlink_recursion_level = 0); + ErrorOr> resolve_path(Process const&, Credentials const&, StringView path, NonnullRefPtr base, RefPtr* out_parent = nullptr, int options = 0, int symlink_recursion_level = 0); ErrorOr> resolve_path_without_veil(Credentials const&, StringView path, NonnullRefPtr base, RefPtr* out_parent = nullptr, int options = 0, int symlink_recursion_level = 0); private: friend class OpenFileDescription; - UnveilNode const& find_matching_unveiled_path(StringView path); + UnveilNode const& find_matching_unveiled_path(Process const&, StringView path); + ErrorOr validate_path_against_process_veil(Process const&, StringView path, int options); + ErrorOr validate_path_against_process_veil(Process const& process, Custody const& custody, int options); ErrorOr validate_path_against_process_veil(Custody const& path, int options); ErrorOr validate_path_against_process_veil(StringView path, int options); diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 449772a36d1..2f0d91230dd 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -709,7 +709,7 @@ ErrorOr Process::dump_perfcore() LockRefPtr description; auto credentials = this->credentials(); for (size_t attempt = 1; attempt <= 10; ++attempt) { - auto description_or_error = VirtualFileSystem::the().open(credentials, perfcore_filename->view(), O_CREAT | O_EXCL, 0400, current_directory(), UidAndGid { 0, 0 }); + auto description_or_error = VirtualFileSystem::the().open(*this, credentials, perfcore_filename->view(), O_CREAT | O_EXCL, 0400, current_directory(), UidAndGid { 0, 0 }); if (!description_or_error.is_error()) { description = description_or_error.release_value(); break;