From e598f22768aa281dbf1f907f652472b3bc088271 Mon Sep 17 00:00:00 2001
From: Liav A <liavalb@gmail.com>
Date: Fri, 23 Dec 2022 13:51:47 +0200
Subject: [PATCH] Kernel: Disallow executing SUID binaries if process is jailed

Check if the process we are currently running is in a jail, and if that
is the case, fail early with the EPERM error code.

Also, as Brian noted, we should also disallow attaching to a jail in
case of already running within a setid executable, as this leaves the
user with false thinking of being secure (because you can't exec new
setid binaries), but the current program is still marked setid, which
means that at the very least we gained permissions while we didn't
expect it, so let's block it.
---
 Base/usr/share/man/man7/Mitigations.md |  1 +
 Kernel/Process.h                       |  1 +
 Kernel/Syscalls/execve.cpp             | 12 ++++++++++--
 Kernel/Syscalls/jail.cpp               | 11 +++++++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/Base/usr/share/man/man7/Mitigations.md b/Base/usr/share/man/man7/Mitigations.md
index e7700b4dedd..299dea110ba 100644
--- a/Base/usr/share/man/man7/Mitigations.md
+++ b/Base/usr/share/man/man7/Mitigations.md
@@ -105,6 +105,7 @@ Special restrictions on filesystem also apply:
 - Write access is forbidden to kernel variables (which are located in `/sys/kernel/variables`).
 - Open access is forbidden to all device nodes except for `/dev/full`, `/dev/null`, `/dev/zero`, `/dev/random` and various
     other TTY/PTY devices (not including Kernel virtual consoles).
+- Executing SUID binaries is forbidden.
 
 It was first added in the following [commit](https://github.com/SerenityOS/serenity/commit/5e062414c11df31ed595c363990005eef00fa263),
 for kernel support, and the following commits added basic userspace utilities:
diff --git a/Kernel/Process.h b/Kernel/Process.h
index bf9d2d0c0cc..c897d4f962c 100644
--- a/Kernel/Process.h
+++ b/Kernel/Process.h
@@ -118,6 +118,7 @@ class Process final
         // FIXME: This should be a NonnullRefPtr
         RefPtr<Credentials> credentials;
         bool dumpable { false };
+        bool executable_is_setid { false };
         Atomic<bool> has_promises { false };
         Atomic<u32> promises { 0 };
         Atomic<bool> has_execpromises { false };
diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp
index d201b226162..4f1e06d149e 100644
--- a/Kernel/Syscalls/execve.cpp
+++ b/Kernel/Syscalls/execve.cpp
@@ -472,6 +472,15 @@ ErrorOr<void> Process::do_exec(NonnullLockRefPtr<OpenFileDescription> main_progr
 {
     VERIFY(is_user_process());
     VERIFY(!Processor::in_critical());
+    auto main_program_metadata = main_program_description->metadata();
+    // NOTE: Don't allow running SUID binaries at all if we are in a jail.
+    TRY(Process::current().jail().with([&](auto& my_jail) -> ErrorOr<void> {
+        if (my_jail && (main_program_metadata.is_setuid() || main_program_metadata.is_setgid())) {
+            return Error::from_errno(EPERM);
+        }
+        return {};
+    }));
+
     // Although we *could* handle a pseudo_path here, trying to execute something that doesn't have
     // a custody (e.g. BlockDevice or RandomDevice) is pretty suspicious anyway.
     auto path = TRY(main_program_description->original_absolute_path());
@@ -507,8 +516,6 @@ ErrorOr<void> Process::do_exec(NonnullLockRefPtr<OpenFileDescription> main_progr
     bool executable_is_setid = false;
 
     if (!(main_program_description->custody()->mount_flags() & MS_NOSUID)) {
-        auto main_program_metadata = main_program_description->metadata();
-
         auto new_euid = old_credentials->euid();
         auto new_egid = old_credentials->egid();
         auto new_suid = old_credentials->suid();
@@ -551,6 +558,7 @@ ErrorOr<void> Process::do_exec(NonnullLockRefPtr<OpenFileDescription> main_progr
     with_mutable_protected_data([&](auto& protected_data) {
         protected_data.credentials = move(new_credentials);
         protected_data.dumpable = !executable_is_setid;
+        protected_data.executable_is_setid = executable_is_setid;
     });
 
     // We make sure to enter the new address space before destroying the old one.
diff --git a/Kernel/Syscalls/jail.cpp b/Kernel/Syscalls/jail.cpp
index 68497bf515a..0f5ac4e16ce 100644
--- a/Kernel/Syscalls/jail.cpp
+++ b/Kernel/Syscalls/jail.cpp
@@ -45,6 +45,17 @@ ErrorOr<FlatPtr> Process::sys$jail_attach(Userspace<Syscall::SC_jail_attach_para
     VERIFY_NO_PROCESS_BIG_LOCK(this);
     TRY(require_promise(Pledge::jail));
 
+    // NOTE: Because the user might run a binary that is using this syscall and
+    // that binary was marked as SUID, then the user might be unaware of the
+    // fact that while no new setuid binaries might be executed, he is already
+    // running within such binary so for the sake of completeness and preventing
+    // naive sense of being secure, we should block that.
+    TRY(with_protected_data([&](auto& protected_data) -> ErrorOr<void> {
+        if (protected_data.executable_is_setid)
+            return EPERM;
+        return {};
+    }));
+
     auto params = TRY(copy_typed_from_user(user_params));
     return m_attached_jail.with([&](auto& my_jail) -> ErrorOr<FlatPtr> {
         // Note: If we are already in a jail, don't let the process escape it even if