From 44c3410cf519aa4a3ea5423d909e0b9a4d98efc8 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Sat, 5 Jul 2025 16:23:36 +0800 Subject: [PATCH] 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 (cherry picked from commit 7028ed21138522495df1e9f8b01195a3c43d47ff) --- src/client/Client.cc | 19 ++++++++++--------- src/test/libcephfs/suidsgid.cc | 22 ++++++++++++++++++++++ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/src/client/Client.cc b/src/client/Client.cc index e76cd9abc50..56a07af9577 100644 --- a/src/client/Client.cc +++ b/src/client/Client.cc @@ -6193,22 +6193,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 2ca38349a72..78369dabcf8 100644 --- 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 #include #include #include @@ -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); -- 2.39.5