But this allows any change to SUID or SGID to happen for unprivileged users, not just clearing them like the comment would suggest? Which would then also allow escalating to the user the file is owned (like e.g., root), if it's already an executable.

@ThomasLamprecht hi Thomas, i concur with you. i am not a filesystem expert, but after reading through the related documents, i found that

When the owner or group of an executable file is changed by an unprivileged user, the S_ISUID and S_ISGID mode bits are cleared.

see chown(2). so the mode value we receive in the FUSE operations has already been processed by the kernel. If SUID/SGID bits should be cleared according to POSIX rules, they will already be cleared in the mode value passed to the fuse driver.

so there are two cases when it comes to modification to SUID/SGID:

1. SUID and/or SGID are the only bits cleared by kernel. we should allow this without even checking if current user is root/owner.
2. otherwise we enforce the regular checks.

before fb1b72d, we allowed an unprivileged user to add mode bits if these bits do not include S_ISUID or S_ISGID.

after fb1b72d, we only allow an unprivileged user to change these two mode bits as long as they don't mutate other bits in the same operation. as you pointed out, this allows an unprivileged to set the S_ISUID and/or S_ISGID bits if they are the only changed bits in this operation.

will come up with a fix shortly.

BTW, the original commit introducing this change was fb1b72d . we should have referenced it in the backport commit..

Hey @ThomasLamprecht - thanks for reporting the issue. This is a pretty serious bug and it is straightforward to reproduce.

ls -l xfile
----------. 1 root root 0 Jul  5 09:04 xfile

➜  tmp.y5yfT8L2Jf chmod 6766 xfile
chmod: changing permissions of 'xfile': Operation not permitted

➜  tmp.y5yfT8L2Jf chmod 6000 xfile
➜  tmp.y5yfT8L2Jf ls -l xfile
---S------. 1 root root 0 Jul  5 09:04 xfile

There isn't a test for this in our qa suite too :/

thanks @vshankar for confirming this issue. created #64356

BTW, the original commit introducing this change was fb1b72d . we should have referenced it in the backport commit..

@tchaikov - the reason for this is that the original issue (which this PR addressed) was raised in the ceph security list initially. The fixes were scrambled as email patches b/w the interested parties. At that point, there wasn't know how on how to test undisclosed security fixes (for which I spent a good amount of time since absolutely no-one seemed to know about it). Unfortunately, with centos8 support getting dropped, we had to do a final quincy point release and this fix had to be merged in the quincy branch first. Yes, this was a wild deviation from our backporting process, but it was something we did as a one time exception.

The fix itself was reviewed however, but, unfortunately, this bug (which @ThomasLamprecht reported) got overlooked during review.