client: prohibit unprivileged users from setting sgid/suid bits

Prior to fb1b72d, unprivileged users could add mode bits as long as
S_ISUID and S_ISGID were not included in the change.

After fb1b72d, unprivileged users were allowed to modify S_ISUID and
S_ISGID bits only when no other mode bits were changed in the same
operation. This inadvertently permitted unprivileged users to set
S_ISUID and/or S_ISGID bits when they were the sole bits being modified.

This behavior should not be allowed. Unprivileged users should be
prohibited from setting S_ISUID and/or S_ISGID bits under any
circumstances.

This change tightens the permission check to prevent unprivileged
users from setting these privileged bits in all cases.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
(cherry picked from commit 7028ed21138522495df1e9f8b01195a3c43d47ff)

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 8f6393f6a8333b90bcd19cda8e38ac0027eace9f..1b71c771d0c514908ae175b7001c4b779d4e7eeb 100644 (file)
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -6204,22 +6204,23 @@ int Client::may_setattr(const InodeRef& in, struct ceph_statx *stx, int mask,
   }
 
   if (mask & CEPH_SETATTR_MODE) {
-    bool allowed = false;
     /*
      * Currently the kernel fuse and libfuse code is buggy and
      * won't pass the ATTR_KILL_SUID/ATTR_KILL_SGID to ceph-fuse.
      * But will just set the ATTR_MODE and at the same time by
      * clearing the suid/sgid bits.
      *
-     * Only allow unprivileged users to clear S_ISUID and S_ISUID.
+     * Only allow unprivileged users to clear S_ISUID and S_ISGID.
      */
-    if ((in->mode & (S_ISUID | S_ISGID)) != (stx->stx_mode & (S_ISUID | S_ISGID)) &&
-        (in->mode & ~(S_ISUID | S_ISGID)) == (stx->stx_mode & ~(S_ISUID | S_ISGID))) {
-      allowed = true;
-    }
-    uint32_t m = ~stx->stx_mode & in->mode; // mode bits removed
-    ldout(cct, 20) << __func__ << " " << *in << " = " << hex << m << dec <<  dendl;
-    if (perms.uid() != 0 && perms.uid() != in->uid && !allowed)
+    uint32_t removed_bits = ~stx->stx_mode & in->mode;
+    uint32_t added_bits = ~in->mode & stx->stx_mode;
+    bool clearing_suid_sgid = (
+      // no new bits added
+      added_bits == 0 &&
+      // only suid/suid bits removed
+      (removed_bits & ~(S_ISUID | S_ISGID)) == 0);
+    ldout(cct, 20) << __func__ << " " << *in << " = " << hex << removed_bits << dec <<  dendl;
+    if (perms.uid() != 0 && perms.uid() != in->uid && !clearing_suid_sgid)
       goto out;
 
     gid_t i_gid = (mask & CEPH_SETATTR_GID) ? stx->stx_gid : in->gid;

diff --git a/src/test/libcephfs/suidsgid.cc b/src/test/libcephfs/suidsgid.cc
index 2ca38349a720a155ee405aa29ecda1c998a266b3..78369dabcf84648bc78144a7f1d3441a96cb92ed 100644 (file)
--- a/src/test/libcephfs/suidsgid.cc
+++ b/src/test/libcephfs/suidsgid.cc
@@ -19,6 +19,7 @@
 #include "include/stringify.h"
 #include "include/cephfs/libcephfs.h"
 #include "include/rados/librados.h"
+#include <cerrno>
 #include <errno.h>
 #include <fcntl.h>
 #include <unistd.h>
@@ -142,6 +143,17 @@ void run_change_mode_test_case()
   ASSERT_EQ(ceph_chmod(cmount, c_dir, 0777), -EPERM);
 }
 
+static void run_set_sgid_suid_test_case(int old_suid_sgid,
+                                       int new_suid_sgid,
+                                       int expected_result)
+{
+  char c_dir[1024];
+  sprintf(c_dir, "/mode_test_%d", getpid());
+  const int mode = 0766;
+  ASSERT_EQ(ceph_mkdirs(admin, c_dir, mode | old_suid_sgid), 0);
+  ASSERT_EQ(ceph_chmod(cmount, c_dir, mode | new_suid_sgid), expected_result);
+}
+
 TEST(SuidsgidTest, WriteClearSetuid) {
   ASSERT_EQ(0, ceph_create(&admin, NULL));
   ASSERT_EQ(0, ceph_conf_read_file(admin, NULL));
@@ -214,8 +226,18 @@ TEST(SuidsgidTest, WriteClearSetuid) {
   // 14, Truncate by unprivileged user clears the suid and sgid
   run_truncate_test_case(06766, 0, 100);
 
+  // 15, Prohibit unpriviledged user from changing non-sgid/suid
+  //     mode bits
   run_change_mode_test_case();
 
+  // 16, Prohibit unpriviledged user from setting suid/sgid
+  run_set_sgid_suid_test_case(0, S_ISUID, -EPERM);
+  run_set_sgid_suid_test_case(0, S_ISGID, -EPERM);
+  run_set_sgid_suid_test_case(0, S_ISUID | S_ISGID, -EPERM);
+  run_set_sgid_suid_test_case(S_ISGID, S_ISUID, -EPERM);
+  run_set_sgid_suid_test_case(S_ISUID, S_ISGID, -EPERM);
+  run_set_sgid_suid_test_case(S_ISGID, S_ISUID | S_ISGID, -EPERM);
+
   // clean up
   ceph_shutdown(cmount);
   ceph_shutdown(admin);