From 43903aa9604e2ab42beb460bcbb112a83a40cb1e Mon Sep 17 00:00:00 2001
From: Liav A <liavalb@gmail.com>
Date: Wed, 24 May 2023 21:55:17 +0300
Subject: [PATCH] SystemServer: Ensure service drop privileges could fail only
 when root

If we try to launch a lazily-spawned service and the SystemServer as a
(running --user) session leader is running with root permissions, then
if it is instructed to drop the root permissions for a the new service
then it will make sense to abort the entire spawn procedure if dropping
of privileges failed.

For other users, trying to change UID/GID to something else doesn't make
sense (and will always actually fail) as we are already running in non
root permissions, hence we don't attempt to do this anymore.
It should be noted that if an explicit User configuration was actually
specified for a Service to be used with, we would still try to login
with the requested User option value, which would fail when running as
non-root user.

This is useful for example when trying to run the pro utility with pls
to elevate to root permissions, but the session leader is still the same
so trying to "drop" privileges to UID 0 doesn't make sense.
---
 Userland/Services/SystemServer/Service.cpp | 36 ++++++++++++++++------
 Userland/Services/SystemServer/Service.h   |  3 ++
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/Userland/Services/SystemServer/Service.cpp b/Userland/Services/SystemServer/Service.cpp
index bc3190c362..3893c6eaaa 100644
--- a/Userland/Services/SystemServer/Service.cpp
+++ b/Userland/Services/SystemServer/Service.cpp
@@ -121,6 +121,27 @@ ErrorOr<void> Service::activate()
     return {};
 }
 
+ErrorOr<void> Service::change_privileges()
+{
+    // NOTE: Dropping privileges makes sense when SystemServer is running
+    // for a root session.
+    // This could happen when we need to spawn a Service to serve a client with non-user UID/GID.
+    // However, in case the user explicitly specified a username via the User= option, then we must
+    // try to login as at that user, so we can't ignore the failure when it was requested to change
+    // privileges.
+    if (auto current_uid = getuid(); m_account.has_value() && m_account.value().uid() != current_uid) {
+        if (current_uid != 0 && !m_must_login)
+            return {};
+        auto& account = m_account.value();
+        if (auto error_or_void = account.login(); error_or_void.is_error()) {
+            dbgln("Failed to drop privileges (tried to change to GID={}, UID={}), due to {}\n", account.gid(), account.uid(), error_or_void.error());
+            exit(1);
+        }
+        TRY(Core::System::setenv("HOME"sv, account.home_directory(), true));
+    }
+    return {};
+}
+
 ErrorOr<void> Service::spawn(int socket_fd)
 {
     if (!FileSystem::exists(m_executable_path)) {
@@ -198,14 +219,7 @@ ErrorOr<void> Service::spawn(int socket_fd)
             TRY(Core::System::setenv("SOCKET_TAKEOVER"sv, socket_takeover_builder.string_view(), true));
         }
 
-        if (m_account.has_value() && m_account.value().uid() != getuid()) {
-            auto& account = m_account.value();
-            if (auto error_or_void = account.login(); error_or_void.is_error()) {
-                dbgln("Failed to drop privileges (GID={}, UID={}), due to {}\n", account.gid(), account.uid(), error_or_void.error());
-                exit(1);
-            }
-            TRY(Core::System::setenv("HOME"sv, account.home_directory(), true));
-        }
+        TRY(change_privileges());
 
         TRY(m_environment.view().for_each_split_view(' ', SplitBehavior::Nothing, [&](auto env) {
             return Core::System::putenv(env);
@@ -290,10 +304,12 @@ Service::Service(Core::ConfigFile const& config, StringView name)
     m_user = config.read_entry(name, "User");
     if (!m_user.is_null()) {
         auto result = Core::Account::from_name(m_user, Core::Account::Read::PasswdOnly);
-        if (result.is_error())
+        if (result.is_error()) {
             warnln("Failed to resolve user {}: {}", m_user, result.error());
-        else
+        } else {
+            m_must_login = true;
             m_account = result.value();
+        }
     }
 
     m_working_directory = config.read_entry(name, "WorkingDirectory");
diff --git a/Userland/Services/SystemServer/Service.h b/Userland/Services/SystemServer/Service.h
index f0aa9067cc..c82aabcea8 100644
--- a/Userland/Services/SystemServer/Service.h
+++ b/Userland/Services/SystemServer/Service.h
@@ -33,6 +33,8 @@ private:
 
     ErrorOr<void> determine_account(int fd);
 
+    ErrorOr<void> change_privileges();
+
     /// SocketDescriptor describes the details of a single socket that was
     /// requested by a service.
     struct SocketDescriptor {
@@ -74,6 +76,7 @@ private:
 
     // The resolved user account to run this service as.
     Optional<Core::Account> m_account;
+    bool m_must_login { false };
 
     // For single-instance services, PID of the running instance of this service.
     pid_t m_pid { -1 };