Description of problem: A downstream patch removing the CAP_DAC_OVERRIDE from the CapabilityBoundingSet line in the sssd.service completely broke FleetCommander integration. Version-Release number of selected component (if applicable): Tested with up-to-date Fedora 26 and Fedora 27. Most likely the issue also happens on rawhide. How reproducible: 100% Steps to Reproduce: 1. git clone https://github.com/fidencio/fleet-commander-vagans 2. In the project's folder do: cd fleet-commander; vagrant up 3. Once the VMs are provisioned, do, from the very same fleet-commander dir: vagrant ssh ipamaster 4. When logged into the ipamaster machine, do: 4.1. kinit admin (admin's password is: "Secret123") 4.2. ipa deskprofileconfig-mod --priority=22 4.3. wget -c https://raw.githubusercontent.com/fleet-commander/fc-client/master/tests/data/sampleprofiledata/0050-0050-0000-0000-0000-Test1.profile 4.4. base64 0050-0050-0000-0000-0000-Test1.profile > finance.json 4.5. ipa deskprofile-add finance --data=finance.json --desc="Finance Department Desktop" 4.6. ipa deskprofilerule-add finance --profile=finance --prio=100 4.7. ipa deskprofilerule-add-user finance --users=admin 4.8. ipa deskprofilerule-add-host finance --hosts=client.ipa.example 4.9. Leave the ipamaster machine 5. From the very same dir from step 3, do: vagrant ssh ipaclient 6. When logged into the ipaclient machine, do: 6.1. Increase ipa.example's domain debug_level in sssd.conf to 10 6.2. Restart sssd: systemctl restart sssd 6.3. ssh to the machine using admin's user: ssh -l admin localhost 6.4. Close the admin's session 6.5. As root, check /var/log/sssd_ipa.example for: (Sun Jan 21 16:56:59 2018) [sssd[be[ipa.example]]] [sss_create_dir] (0x0020): Error reading '/var/lib/sss/deskprofile/ipa.example': Permission denied (Sun Jan 21 16:56:59 2018) [sssd[be[ipa.example]]] [ipa_deskprofile_rules_create_user_dir] (0x0020): Failed to create the directory "/var/lib/sss/deskprofile/ipa.example/admin" that would be used to store the Desktop Profile rules for the user "admin" [13]: Permission denied (Sun Jan 21 16:56:59 2018) [sssd[be[ipa.example]]] [ipa_pam_session_handler_save_deskprofile_rules] (0x0020): Cannot create the user directory [13]: Permission denied Actual results: The file "/var/lib/sss/deskprofile/ipa.example/admin/0050-0050-0000-0000-0000-Test1.profile" is not created. Expected results: The file "/var/lib/sss/deskprofile/ipa.example/admin/0050-0050-0000-0000-0000-Test1.profile" should be created. Additional info: The faulty change was introduced in the commits 0982e5e83 (f26 branch), ea632499f (f27 branch) and 4f58854911 (master) where the CAP_DAC_OVERRIDE capability has been removed without any explanation. Adding the capability back to the sssd.service makes it work as expected. According to http://man7.org/linux/man-pages/man7/capabilities.7.html CAP_DAC_OVERRIDE "Bypass file read, write, and execute permission checks. (DAC is an abbreviation of "discretionary access control".)" Would be really good for the future if the patches pushed downstream could have a commit message explaining the reasons of the changes, why each one of the capabilities have been added or removed there and why they're needed by the project. Workaround for those trying to take advantage of the FleetCommander integration: As root do: - systemctl edit sssd.service - Add the following content to the file, save and close: [Service] CapabilityBoundingSet=CAP_IPC_LOCK CAP_CHOWN CAP_DAC_READ_SEARCH CAP_DAC_OVERRIDE CAP_KILL CAP_NET_ADMIN CAP_SYS_NICE CAP_FOWNER CAP_SETGID CAP_SETUID CAP_SYS_ADMIN CAP_SYS_RESOURCE CAP_BLOCK_SUSPEND - systemctl daemon-reload - systemctl restart sssd.service
Created attachment 1384126 [details] Proposed fix for this bugzilla (for master) I've attached a patch for master that adds back the CAP_DAC_OVERRIDE to the CapabilityBoundingSet line of sssd.service. Either the same or a very similar patch can be used/provided to f26 and f27 branches, just let me know if those are desired.
Since CAP_DAC_OVERRIDE is a quite heavy capability, you can do everything on the file system, I wonder if there is an alternative solution to fix this issue? If I understand it correctly /var/lib/sss/deskprofile/ and its sub-directories are manages exclusively by SSSD. Maybe there is just some umask setting missing when creating one of the sub-directories? About changing capabilities in general. I agree that it would be nice if every change has an explanation why the change is good/needed. Since capabilities will give you more privileges and than you have by default, removing one is imo always a good idea. But of course if this breaks functionality and it cannot be mitigated otherwise is should be reverted.
(In reply to Sumit Bose from comment #2) > Since CAP_DAC_OVERRIDE is a quite heavy capability, you can do everything on > the file system, I wonder if there is an alternative solution to fix this > issue? There's an alternative solution, that would be changing SSSD code to: When creating the files: - Create the dirs needed as root; - Create the files needed as root; - Only after that change the owner/permissions of those dirs and files; When removing the files: - Change the owner/permissions of the dirs; - Change the owner/permissions of the files; - Only after that, try to remove those dirs files; In any case, if we decide to take this path would be better to have a ticket about it, having the change reverted (as it introduced regressions) and add it back whenever our code is ready to run without CAP_DAC_OVERRIDE. > If I understand it correctly /var/lib/sss/deskprofile/ and its > sub-directories are manages exclusively by SSSD. Maybe there is just some > umask setting missing when creating one of the sub-directories? > No, it's not only a matter of umask setting due to the way the code is done. We end up creating a dir which has as owner user:group and permissions 600. Without this cap, to have the code working as it is without any changes, this dir should have 607 permission so we could create/delete files there, which doesn't sound like a good idea. One possible solution would be the one I suggested above, but it would take time and the best to do would be to revert the changes that introduced the regressions till the code is running without issues without CAP_DAC_OVERRIDE ... and just then remove the capability.
(In reply to Fabiano Fidêncio from comment #1) > Created attachment 1384126 [details] > Proposed fix for this bugzilla (for master) > > I've attached a patch for master that adds back the CAP_DAC_OVERRIDE to the > CapabilityBoundingSet line of sssd.service. > > Either the same or a very similar patch can be used/provided to f26 and f27 > branches, just let me know if those are desired. This patch would not help because it would be blocked by SELinux policy as well. Current set of capability "mirror" allowed capabilities by SELinux-policy in fedora
(In reply to Fabiano Fidêncio from comment #3) > (In reply to Sumit Bose from comment #2) > > Since CAP_DAC_OVERRIDE is a quite heavy capability, you can do everything on > > the file system, I wonder if there is an alternative solution to fix this > > issue? > > There's an alternative solution, that would be changing SSSD code to: > When creating the files: > - Create the dirs needed as root; > - Create the files needed as root; > - Only after that change the owner/permissions of those dirs and files; > When removing the files: > - Change the owner/permissions of the dirs; > - Change the owner/permissions of the files; > - Only after that, try to remove those dirs files; > Usually there is enough to keep an owner and change group to root. Unfortunately design page for this feature does not contain such very important information. Please update design page with required info and then we can discuss about finding a way how to solve this.
Anyway, we should not add back back capability nentioned in subject therefore closing as wontfix. Please move the discussion to newly created PR for related design page.
(In reply to Lukas Slebodnik from comment #6) > Anyway, we should not add back back capability nentioned in subject > therefore closing as wontfix. Please move the discussion to newly created PR > for related design page. Here is an upstream ticket for further discussion. https://pagure.io/SSSD/sssd/issue/3621 Meanwhile, fedora users could use less secure systemd service file for sssd in /etc/systemd/system (or use just a drop-in files to modify/remove CapabilityBoundingSet)
But isn't it better to keep this BZ open rather than closing and let it track that the feature currently doesn't work regardless of what the technical solution would be?
Just adding a last comment in this already closed bug, just in case someone hits this issue! CAP_DAC_OVERRIDE has been dropped in order to follow SELinux-policy in Fedora and it changed on Fedora Rawhide. I've done a bunch of tests on Fedora 26 and Fedora 27, where this capability was also dropped, and it doesn't cause any issues with SELinux (thus, I'd really prefer to have it back there, but I do respect the maintainer's decision to have the bug closed ... even if it introduced regressions on Fedora 26 and Fedora 27). So, as mentioned in comment #7, whoever hits this issue, please, just use a less secure systemd by adding back CAP_DAC_OVERRIDE and Fleet Commander integration will be working as expected again. Patches on SSSD side are being discussed and hopefully they'll be backported to all affected versions of Fedora, mainly because dropping CAP_DAC_OVERRIDE *is* a good thing and a *proper* solution around it is more welcome than having back the CAP_DAC_OVERRIDE (although, I'd prefer to have it added back on Fedora 26 and Fedora 27 till the proper solution is lands on those two).
FWIW, I see nothing wrong in keeping CAP_DAC_OVERRIDE, the process is running as root and can impersonate users at any time so removing this capability does not change the security stance. However it may make some errors less sever if people use seteuid() to change the effective id of the user, because then the process cannot alter data or access compartmentilized data by mistake. Opening permissions on the files negates the benefit of using seteuid() so it is the least desirable way to handle this "issue". My 2c, HTH
(In reply to Simo Sorce from comment #10) > FWIW, > I see nothing wrong in keeping CAP_DAC_OVERRIDE, the process is running as > root and can impersonate users at any time so removing this capability does > not change the security stance. > SSSD does not run as a root with SELinux in enforcing mode. And I have a (long term) plan to limit even root user with seccomp. + also hardening SELinux policy because responder and backend are running with the same SELinux context and therefore responder can execute many viles from /usr/bin/ (bin_t) sh# sesearch -A -s sssd_t -c file | grep exec allow sssd_t base_ro_file_type:file { execute execute_no_trans getattr ioctl lock map open read }; allow sssd_t bin_t:file { execute execute_no_trans getattr ioctl lock map open read }; allow sssd_t chkpwd_exec_t:file { execute getattr open read }; allow sssd_t sssd_exec_t:file { entrypoint execute execute_no_trans getattr ioctl lock map open read }; allow sssd_t sssd_selinux_manager_exec_t:file { execute getattr open read }; allow sssd_t updpwd_exec_t:file { execute getattr open read }; (In reply to Fabiano Fidêncio from comment #9) > CAP_DAC_OVERRIDE has been dropped in order to follow SELinux-policy in > Fedora and it changed on Fedora Rawhide. > I use only rawhide. So thank you very much for reminder. I will send PR to backport related patches into f26+ selinux-policy.
(In reply to Lukas Slebodnik from comment #11) > (In reply to Simo Sorce from comment #10) > > FWIW, > > I see nothing wrong in keeping CAP_DAC_OVERRIDE, the process is running as > > root and can impersonate users at any time so removing this capability does > > not change the security stance. > > > > SSSD does not run as a root with SELinux in enforcing mode. And I have a > (long term) plan to limit even root user with seccomp. + also hardening > SELinux policy > because responder and backend are running with the same SELinux context and > therefore responder can execute many viles from /usr/bin/ (bin_t) > > sh# sesearch -A -s sssd_t -c file | grep exec > allow sssd_t base_ro_file_type:file { execute execute_no_trans getattr ioctl > lock map open read }; > allow sssd_t bin_t:file { execute execute_no_trans getattr ioctl lock map > open read }; > allow sssd_t chkpwd_exec_t:file { execute getattr open read }; > allow sssd_t sssd_exec_t:file { entrypoint execute execute_no_trans getattr > ioctl lock map open read }; > allow sssd_t sssd_selinux_manager_exec_t:file { execute getattr open read }; > allow sssd_t updpwd_exec_t:file { execute getattr open read }; > > > (In reply to Fabiano Fidêncio from comment #9) > > CAP_DAC_OVERRIDE has been dropped in order to follow SELinux-policy in > > Fedora and it changed on Fedora Rawhide. > > > > I use only rawhide. So, please, don't close bugs that are specific to F26 and F27 before double-checking them. And, please, do not do blind backports to F26 and F27 as you're not using those. > So thank you very much for reminder. I will send PR to > backport related patches into f26+ selinux-policy. Okay. Please, if possible, post the link of the PR here, so we can follow what's going on and then decide whether the SSSD patches will have to be backported to F26 and F27 as well due to the selinux-policy change.
(In reply to Fabiano Fidêncio from comment #12) > So, please, don't close bugs that are specific to F26 and F27 before > double-checking them. And, please, do not do blind backports to F26 and F27 > as you're not using those. > This ticket was not closed because of rawhide/SELinux policy but because adding back capability DAC_OVERRIDE is not a good idea from security POV and mainly because it would cause more issues when running sssd in non privileged mode by default BZ1212503
(In reply to Lukas Slebodnik from comment #13) > (In reply to Fabiano Fidêncio from comment #12) > > So, please, don't close bugs that are specific to F26 and F27 before > > double-checking them. And, please, do not do blind backports to F26 and F27 > > as you're not using those. > > > > This ticket was not closed because of rawhide/SELinux policy but because > adding back capability DAC_OVERRIDE is not a good idea from security POV and > mainly because it would cause more issues when running sssd in non > privileged mode by default BZ1212503 Adding back the capability is indeed not the best idea, thus I'm providing patches for SSSD. Blindly removing the capability and introducing regressions is also a not good idea (thus I've suggested to have it added back until the patches are not merged). The BZ1212503 was never ever mentioned before and, AFAIU, it's not something that happens in Fedora nowadays. So, introducing a regression and not being willing to revert this for a week or two till we have the patches merged in favour of a future-plan doesn't sound like the neither the best nor the most community friendly approach. But you're the maintainer and I do respect your decision. So, please, if it's not a lot of trouble for you, would you mind adding the links for the PRs you're going to open for f26+ selinux-policy?
(In reply to Fabiano Fidêncio from comment #14) > (In reply to Lukas Slebodnik from comment #13) > > (In reply to Fabiano Fidêncio from comment #12) > > > So, please, don't close bugs that are specific to F26 and F27 before > > > double-checking them. And, please, do not do blind backports to F26 and F27 > > > as you're not using those. > > > > > > > This ticket was not closed because of rawhide/SELinux policy but because > > adding back capability DAC_OVERRIDE is not a good idea from security POV and > > mainly because it would cause more issues when running sssd in non > > privileged mode by default BZ1212503 > > Adding back the capability is indeed not the best idea, thus I'm providing > patches for SSSD. Blindly removing the capability and introducing > regressions is also a not good idea (thus I've suggested to have it added > back until the patches are not merged). > It was not blindly. I ran tests and I could not see any regressions. Unfortunately, there are not ANY tests for that part of code but that it is a little bit off topic for this BZ. Thank you very much for agreement that adding capability back is not a good idea.
(In reply to Lukas Slebodnik from comment #15) > (In reply to Fabiano Fidêncio from comment #14) > > (In reply to Lukas Slebodnik from comment #13) > > > (In reply to Fabiano Fidêncio from comment #12) > > > > So, please, don't close bugs that are specific to F26 and F27 before > > > > double-checking them. And, please, do not do blind backports to F26 and F27 > > > > as you're not using those. > > > > > > > > > > This ticket was not closed because of rawhide/SELinux policy but because > > > adding back capability DAC_OVERRIDE is not a good idea from security POV and > > > mainly because it would cause more issues when running sssd in non > > > privileged mode by default BZ1212503 > > > > Adding back the capability is indeed not the best idea, thus I'm providing > > patches for SSSD. Blindly removing the capability and introducing > > regressions is also a not good idea (thus I've suggested to have it added > > back until the patches are not merged). > > > > It was not blindly. I ran tests and I could not see any regressions. > Unfortunately, there are not ANY tests for that part of code but that it is > a little bit off topic for this BZ. As a few other parts of the project. However, a regression was found and I'd prefer to keep the ticket open at least to track that the fix was properly backported to Fedora and by closing the ticket you simply remove the chance of having it properly tracked. Please, keep in mind that I understand your point and I'm following your suggestions and trying to have the problem solved and tracked in the best possible way.
(In reply to Fabiano Fidêncio from comment #14) > The BZ1212503 was never ever mentioned before and, AFAIU, It was not directly mentioned but non-privileged use-case is mentioned in the description of upstream ticket. Which was created 9 days ago. https://pagure.io/SSSD/sssd/issue/3621 Sorry for splitting the paragraph. > it's not something > that happens in Fedora nowadays. So, introducing a regression and not being > willing to revert this for a week or two till we have the patches merged in > favour of a future-plan doesn't sound like the neither the best nor the most > community friendly approach. > I think we will need to agree to disagree that it is a regression. It was a bug in sssd that it relied on CAP_DAC_OVERRIDE and nobody noticed that due to various reason. Comment7 contains workaround which I agree is not super friendly for community but it does not block (low percentage of) users who would like to test that. It is a really good feature but nowadays most of sssd users in fedora have a much different focus.
(In reply to Fabiano Fidêncio from comment #16) > (In reply to Lukas Slebodnik from comment #15) > > It was not blindly. I ran tests and I could not see any regressions. > > Unfortunately, there are not ANY tests for that part of code but that it is > > a little bit off topic for this BZ. > > As a few other parts of the project. Would you mind to file upstream tickets for such parts which do not have any upstream/downstream code coverage? I would like to track them even in "Future milestone".
(In reply to Lukas Slebodnik from comment #18) > (In reply to Fabiano Fidêncio from comment #16) > > (In reply to Lukas Slebodnik from comment #15) > > > It was not blindly. I ran tests and I could not see any regressions. > > > Unfortunately, there are not ANY tests for that part of code but that it is > > > a little bit off topic for this BZ. > > > > As a few other parts of the project. > > Would you mind to file upstream tickets for such parts which do not have > any upstream/downstream code coverage? I would like to track them even in > "Future milestone". This is not related to this bugzilla at all, but I'll do once I face them. Thanks for your suggestion and your time spent on this bug report.
(In reply to Fabiano Fidêncio from comment #19) > (In reply to Lukas Slebodnik from comment #18) > > (In reply to Fabiano Fidêncio from comment #16) > > > (In reply to Lukas Slebodnik from comment #15) > > > > It was not blindly. I ran tests and I could not see any regressions. > > > > Unfortunately, there are not ANY tests for that part of code but that it is > > > > a little bit off topic for this BZ. > > > > > > As a few other parts of the project. > > > > Would you mind to file upstream tickets for such parts which do not have > > any upstream/downstream code coverage? I would like to track them even in > > "Future milestone". > > This is not related to this bugzilla at all, but I'll do once I face them. > I agree there were many (partially related/unrelated) comments mentioned in this BZ. But this is how discussion usually works :-)
(In reply to Lukas Slebodnik from comment #20) > (In reply to Fabiano Fidêncio from comment #19) > > (In reply to Lukas Slebodnik from comment #18) > > > (In reply to Fabiano Fidêncio from comment #16) > > > > (In reply to Lukas Slebodnik from comment #15) > > > > > It was not blindly. I ran tests and I could not see any regressions. > > > > > Unfortunately, there are not ANY tests for that part of code but that it is > > > > > a little bit off topic for this BZ. > > > > > > > > As a few other parts of the project. > > > > > > Would you mind to file upstream tickets for such parts which do not have > > > any upstream/downstream code coverage? I would like to track them even in > > > "Future milestone". > > > > This is not related to this bugzilla at all, but I'll do once I face them. > > > > I agree there were many (partially related/unrelated) comments mentioned in > this BZ. But this is how discussion usually works :-) Although is quite hard to keep a discussion when a bug is prematurely closed as WONTFIX and the maintainer is quite busy trying to prove himself right instead of taking a step back and listening to the others. Anyways, you have the power, you used the power you have, that are the rules. :-) Anyways, thanks for the discussion, we're all learning here!
This *is* a bug, and *is* a regression. So closing Wontfix is absolutely incorrect. It *will* be fixed, either by changing the restiction back or by changing code in SSSD. You will close the bug when the fix lands in Fedora, whatever that is.
Just for the record, the fix has landed upstream[0] and has been backported to F26[1], F27[2] and rawhide. [0]: https://pagure.io/SSSD/sssd/issue/3621 [1]: https://bodhi.fedoraproject.org/updates/sssd-1.16.0-6.fc26 [2]: https://bodhi.fedoraproject.org/updates/sssd-1.16.0-7.fc27