client: prohibit unprivileged users from setting sgid/suid bits by tchaikov · Pull Request #64356 · ceph/ceph

tchaikov · 2025-07-05T14:47:28Z

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.

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins test classic perf Jenkins Job | Jenkins Job Definition
jenkins test crimson perf Jenkins Job | Jenkins Job Definition
jenkins test signed Jenkins Job | Jenkins Job Definition
jenkins test make check Jenkins Job | Jenkins Job Definition
jenkins test make check arm64 Jenkins Job | Jenkins Job Definition
jenkins test submodules Jenkins Job | Jenkins Job Definition
jenkins test dashboard Jenkins Job | Jenkins Job Definition
jenkins test dashboard cephadm Jenkins Job | Jenkins Job Definition
jenkins test api Jenkins Job | Jenkins Job Definition
jenkins test docs ReadTheDocs | Github Workflow Definition
jenkins test ceph-volume all Jenkins Jobs | Jenkins Jobs Definition
jenkins test windows Jenkins Job | Jenkins Job Definition
jenkins test rook e2e Jenkins Job | Jenkins Job Definition

ThomasLamprecht · 2025-07-05T15:52:39Z

src/client/Client.cc

+    bool clearing_suid_sgid = false;
+    static const uint16_t SUID_SGID_MASK = S_ISUID | S_ISGID;
+    if ((in->mode & ~SUID_SGID_MASK) == (stx->stx_mode & ~SUID_SGID_MASK) &&  // other bits unchanged
+        (in->mode & SUID_SGID_MASK) > (stx->stx_mode & SUID_SGID_MASK)) {  // only clearing


I thought about using a greater than comparison check with a combined mask initially, but AFAICT that still breaks as one can clear the higher significant bit while setting the lower one, Here is a simplified example of what I mean:

existing mode: b10 == 2 would be bigger than new mode: b01 == 1 while a bit got set for the new one.

I'm not sure what effect that would have in practice, but I think it might still lead to an attack if the group or user (whatever the lower significant bit maps to) of the file is one that has more privileges. And even if not, it would IMO be rather confusing and might lead to issues down the line if this gets copied somewhere else with the assumption it's safe.

While it's less clever and needs more code, it might be the best to check each bit separately, e.g. something like:

if ((in->mode & ~(S_ISUID | S_ISGID)) == (stx->stx_mode & ~(S_ISUID | S_ISGID)) && // other bits unchanged (in->mode & S_ISUID) > (stx->stx_mode & S_ISUID) && // only clearing SUID (in->mode & S_ISGID) > (stx->stx_mode & S_ISGID)) { // only clearing SGID clearing_suid_sgid = true; }

@ThomasLamprecht hi Thomas, i was reading the related linux kernel, and realized that there were chances that only the S_ISUID is cleared, so we might not want to prohibit this case. i updated my change accordingly.

tchaikov · 2025-07-06T01:17:06Z

changelog

prohibit S_ISGID -> S_ISUID change
add more tests

vshankar · 2025-07-07T06:11:47Z

changelog

prohibit S_ISGID -> S_ISUID change

add more tests

Thanks @tchaikov - I will have a look.

vshankar

Minor nit. Otherwise LGTM.

I'm trying to run the test locally, but running in issues.

src/test/libcephfs/suidsgid.cc

tchaikov · 2025-07-07T09:37:50Z

changelog

s/old_sid_suid/old_suid_sgid/

tchaikov · 2025-07-07T13:44:16Z

jenkins test windows

ThomasLamprecht

Your new variant makes the code easier to read, the change itself LGTM too:

Reviewed-by: Thomas Lamprecht t.lamprecht@proxmox.com

vshankar · 2025-07-08T04:20:58Z

jenkins test windows

vshankar · 2025-07-08T07:04:03Z

jenkins test windows

vshankar · 2025-07-08T13:32:32Z

@tchaikov I think the windows failure is legit.

[2025-07-08T08:47:04.000Z] [isolated][googletest] ceph_test_libcephfs_suidsgid failed. Error: Command returned non-zero code(1): "cmd /c 'C:\ceph\ceph_test_libcephfs_suidsgid.exe --gtest_output=xml:C:\workspace\test_results\out\ceph_test_libcephfs_suidsgid\ceph_test_libcephfs_suidsgid_results.xml --gtest_filter="-LibCephFS.Deleg*" >> C:\workspace\test_results\out\ceph_test_libcephfs_suidsgid\ceph_test_libcephfs_suidsgid_results.log 2>&1'".
[2025-07-08T08:47:04.000Z] [isolated][googletest] ceph_test_libcephfs_vxattr passed.
[2025-07-08T08:47:04.000Z] [isolated][googletest] unittest_admin_socket passed.
ceph_test_libcephfs_suidsgid.SuidsgidTest.WriteClearSetuid
[2025-07-08T08:47:07.000Z] subunit-trace reports failures: Command failed: type C:\workspace\test_results\subunit.out | subunit-trace
One or more test suites have failed
At C:\workspace\repos\ceph-win32-tests\test_host\run_tests.ps1:28 char:9
+         throw "One or more test suites have failed"
+         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (One or more test suites have failed:String) [], RuntimeException
    + FullyQualifiedErrorId : One or more test suites have failed

Mind having a look please?

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>

tchaikov · 2025-07-09T04:00:16Z

@vshankar hi Venky, i think that the failures were caused by the new negative tests which intended to chmod by clearing suid / sgid directly with an unprivileged user. these tests failed because of -EPERM. i think this is expected, as unprivileged user should not be allowed chown a directory to clearing its suid / sgid. chown(2) only puts that an executable's suid / sgid should be cleared when the executable is mutated by an unprivileged user. this does not apply to the added negative tests. so i am removing them in the latest revision.

tchaikov · 2025-07-09T04:00:44Z

changelog:

drop irrelevant negative tests added in previous revision.

vshankar · 2025-07-09T05:24:51Z

chown(2) only puts that an executable's suid / sgid should be cleared when the executable is mutated by an unprivileged user. this does not apply to the added negative tests. so i am removing them in the latest revision.

Makes sense. We could however add a test for this specific case, correct? I.e., dropping suid/sgid bits from an executable by an unprivileged user.

tchaikov · 2025-07-09T05:34:56Z

chown(2) only puts that an executable's suid / sgid should be cleared when the executable is mutated by an unprivileged user. this does not apply to the added negative tests. so i am removing them in the latest revision.

Makes sense. We could however add a test for this specific case, correct? I.e., dropping suid/sgid bits from an executable by an unprivileged user.

we already have tests for this in the same test case. see

ceph/src/test/libcephfs/suidsgid.cc

Line 203 in 7c523c4

run_fallocate_test_case(02777, 0); // a+rwx,g+rwxs
ceph/src/test/libcephfs/suidsgid.cc

Line 200 in 7c523c4

run_fallocate_test_case(02676, 0); // a+rw,g+rwxs
ceph/src/test/libcephfs/suidsgid.cc

Line 185 in 7c523c4

run_fallocate_test_case(06777, 0); // a+rwxs

vshankar · 2025-07-09T05:46:50Z

we already have tests for this in the same test case. see

Got it. Thx! This change is good to run through tests.

vshankar · 2025-07-09T06:49:05Z

jenkins test make check

tchaikov · 2025-07-10T01:50:25Z

jenkins test api

vshankar · 2025-07-10T13:07:08Z

This PR is under test in https://tracker.ceph.com/issues/72073.

tchaikov · 2025-07-21T03:53:11Z

@vshankar hi Venky, thank you for testing this change. now that this change has been verified at least at https://pulpito.ceph.com/vshankar-2025-07-14_15:05:46-fs-wip-vshankar-testing-20250714.111344-debug-testing-default-smithi/8385861/. and the test passed. is there anything else i can do to move this PR forward?

vshankar · 2025-07-21T04:00:47Z

@vshankar hi Venky, thank you for testing this change. now that this change has been verified at least at https://pulpito.ceph.com/vshankar-2025-07-14_15:05:46-fs-wip-vshankar-testing-20250714.111344-debug-testing-default-smithi/8385861/. and the test passed. is there anything else i can do to move this PR forward?

This is ready to merge. It's just waiting on me to prepare run wiki which I have been a bit slow due to other stuff needing my attention.

tchaikov · 2025-07-21T04:10:11Z

Ahh, I see. Thanks again!

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-vshankar-testing-20250714111344-debug

tchaikov requested a review from lxbsz July 5, 2025 14:47

github-actions bot added cephfs Ceph File System tests labels Jul 5, 2025

tchaikov mentioned this pull request Jul 5, 2025

quincy: client: disallow unprivileged users to escalate root privileges #60314

Merged

14 tasks

tchaikov requested a review from vshankar July 5, 2025 14:49

tchaikov force-pushed the wip-client-suid branch from 106587b to 68e2f01 Compare July 5, 2025 14:55

ThomasLamprecht suggested changes Jul 5, 2025

View reviewed changes

tchaikov force-pushed the wip-client-suid branch from 68e2f01 to 5314f26 Compare July 6, 2025 01:12

vshankar reviewed Jul 7, 2025

View reviewed changes

src/test/libcephfs/suidsgid.cc Outdated Show resolved Hide resolved

tchaikov force-pushed the wip-client-suid branch from 5314f26 to 8e970cb Compare July 7, 2025 09:34

vshankar added the wip-vshankar-testing4 label Jul 7, 2025

ThomasLamprecht approved these changes Jul 7, 2025

View reviewed changes

tchaikov force-pushed the wip-client-suid branch from 8e970cb to 7028ed2 Compare July 9, 2025 03:53

vshankar approved these changes Jul 21, 2025

View reviewed changes

vshankar merged commit 64f0d78 into ceph:main Jul 21, 2025
13 checks passed

vshankar removed the wip-vshankar-testing4 label Jul 21, 2025

Conversation

tchaikov commented Jul 5, 2025

Contribution Guidelines

Checklist

Uh oh!

ThomasLamprecht Jul 5, 2025

Choose a reason for hiding this comment

Uh oh!

tchaikov Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchaikov commented Jul 6, 2025

Uh oh!

vshankar commented Jul 7, 2025

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tchaikov commented Jul 7, 2025

Uh oh!

tchaikov commented Jul 7, 2025

Uh oh!

ThomasLamprecht left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented Jul 8, 2025

Uh oh!

vshankar commented Jul 8, 2025

Uh oh!

vshankar commented Jul 8, 2025

Uh oh!

tchaikov commented Jul 9, 2025

Uh oh!

tchaikov commented Jul 9, 2025

Uh oh!

vshankar commented Jul 9, 2025

Uh oh!

tchaikov commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar commented Jul 9, 2025

Uh oh!

vshankar commented Jul 9, 2025

Uh oh!

tchaikov commented Jul 10, 2025

Uh oh!

vshankar commented Jul 10, 2025

Uh oh!

tchaikov commented Jul 21, 2025

Uh oh!

vshankar commented Jul 21, 2025

Uh oh!

tchaikov commented Jul 21, 2025

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tchaikov Jul 6, 2025 •

edited

Loading

tchaikov commented Jul 9, 2025 •

edited

Loading