From 5a649d0fd5cbb42af31988fd4121adbfabf83696 Mon Sep 17 00:00:00 2001
From: Liav A <liavalb@gmail.com>
Date: Sun, 19 Dec 2021 15:10:45 +0200
Subject: [PATCH] Kernel: Return EINVAL when specifying -1 for setuid and
 similar syscalls

For setreuid and setresuid syscalls, -1 means to set the current
uid/euid/gid/egid value, to be more convenient for programming.
However, for other syscalls where we pass only one argument, there's no
justification to specify -1.

This behavior is identical to how Linux handles the value -1, and is
influenced by the fact that the manual pages for the group of one
argument syscalls that handle ID operations is ambiguous about this
topic.
---
 Base/usr/share/man/man2/seteuid.md |  1 +
 Base/usr/share/man/man2/setuid.md  |  1 +
 Kernel/Syscalls/setuid.cpp         | 12 ++++++++++++
 Tests/Kernel/CMakeLists.txt        |  1 +
 Tests/Kernel/TestInvalidUIDSet.cpp | 28 ++++++++++++++++++++++++++++
 5 files changed, 43 insertions(+)
 create mode 100644 Tests/Kernel/TestInvalidUIDSet.cpp

diff --git a/Base/usr/share/man/man2/seteuid.md b/Base/usr/share/man/man2/seteuid.md
index e47347d32c..5941771eb5 100644
--- a/Base/usr/share/man/man2/seteuid.md
+++ b/Base/usr/share/man/man2/seteuid.md
@@ -27,6 +27,7 @@ Otherwise, returns -1 and sets `errno` to describe the error.
 ## Errors
 
 * `EPERM`: The new ID is not equal to the real ID or saved ID, and the user is not superuser.
+* `EINVAL`: The new ID is set to invalid value (-1).
 
 ## See also
 
diff --git a/Base/usr/share/man/man2/setuid.md b/Base/usr/share/man/man2/setuid.md
index 7743d23355..5b9eeb171d 100644
--- a/Base/usr/share/man/man2/setuid.md
+++ b/Base/usr/share/man/man2/setuid.md
@@ -25,6 +25,7 @@ Otherwise, returns -1 and sets `errno` to describe the error.
 ## Errors
 
 * `EPERM`: The new ID is not equal to the real ID or effective ID, and the user is not superuser.
+* `EINVAL`: The new ID is set to invalid value (-1).
 
 ## See also
 
diff --git a/Kernel/Syscalls/setuid.cpp b/Kernel/Syscalls/setuid.cpp
index 3b07c7aeda..7b6c058428 100644
--- a/Kernel/Syscalls/setuid.cpp
+++ b/Kernel/Syscalls/setuid.cpp
@@ -13,6 +13,9 @@ ErrorOr<FlatPtr> Process::sys$seteuid(UserID new_euid)
     VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this)
     REQUIRE_PROMISE(id);
 
+    if (new_euid == (uid_t)-1)
+        return EINVAL;
+
     if (new_euid != uid() && new_euid != suid() && !is_superuser())
         return EPERM;
 
@@ -30,6 +33,9 @@ ErrorOr<FlatPtr> Process::sys$setegid(GroupID new_egid)
     VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this)
     REQUIRE_PROMISE(id);
 
+    if (new_egid == (uid_t)-1)
+        return EINVAL;
+
     if (new_egid != gid() && new_egid != sgid() && !is_superuser())
         return EPERM;
 
@@ -46,6 +52,9 @@ ErrorOr<FlatPtr> Process::sys$setuid(UserID new_uid)
     VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this)
     REQUIRE_PROMISE(id);
 
+    if (new_uid == (uid_t)-1)
+        return EINVAL;
+
     if (new_uid != uid() && new_uid != euid() && !is_superuser())
         return EPERM;
 
@@ -64,6 +73,9 @@ ErrorOr<FlatPtr> Process::sys$setgid(GroupID new_gid)
     VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this)
     REQUIRE_PROMISE(id);
 
+    if (new_gid == (uid_t)-1)
+        return EINVAL;
+
     if (new_gid != gid() && new_gid != egid() && !is_superuser())
         return EPERM;
 
diff --git a/Tests/Kernel/CMakeLists.txt b/Tests/Kernel/CMakeLists.txt
index 4cec637dc3..5744a74cc6 100644
--- a/Tests/Kernel/CMakeLists.txt
+++ b/Tests/Kernel/CMakeLists.txt
@@ -33,6 +33,7 @@ serenity_test("crash.cpp" Kernel MAIN_ALREADY_DEFINED)
 
 set(LIBTEST_BASED_SOURCES
     TestEFault.cpp
+    TestInvalidUIDSet.cpp
     TestKernelAlarm.cpp
     TestKernelFilePermissions.cpp
     TestKernelPledge.cpp
diff --git a/Tests/Kernel/TestInvalidUIDSet.cpp b/Tests/Kernel/TestInvalidUIDSet.cpp
new file mode 100644
index 0000000000..19ba9d5585
--- /dev/null
+++ b/Tests/Kernel/TestInvalidUIDSet.cpp
@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2021, Liav A. <liavalb@hotmail.co.il>
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#include <LibTest/TestCase.h>
+#include <errno.h>
+#include <unistd.h>
+
+TEST_CASE(test_invalid_set_uid_parameters)
+{
+    auto res = setuid(-1);
+    EXPECT_EQ(res, -1);
+    EXPECT_EQ(errno, EINVAL);
+
+    res = seteuid(-1);
+    EXPECT_EQ(res, -1);
+    EXPECT_EQ(errno, EINVAL);
+
+    res = setgid(-1);
+    EXPECT_EQ(res, -1);
+    EXPECT_EQ(errno, EINVAL);
+
+    res = setegid(-1);
+    EXPECT_EQ(res, -1);
+    EXPECT_EQ(errno, EINVAL);
+}