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 <[email protected]>

Author: tchaikov

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;

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);