fedora-selinux / selinux-policy

selinux-policy for Fedora is a large patch off the mainline
GNU General Public License v2.0
168 stars 168 forks source link

Allow samba to have dac_override capability #1990

Closed anoopcs9 closed 8 months ago

anoopcs9 commented 10 months ago

Previously commit cc5d0d7c98d06866f26dd1f54b34f70fd3b531f9 removed dac_override capability from many SELinux modules. But with recent changes to Samba upstream it has become necessary to have this capability to work under some special common configurations.

One among those configurations require smbd to read ACLs stored in extended attributes from security namespace which further calls for additional privileges where dac_override would be the bare minimum and least expensive capability to be acquired without becoming root. You may find slighlty more details from the discussion around the merge request upstream.

Therefore resurrect the dac_override capability for smbd_t to avoid the following AVC denial.

type=AVC msg=audit(1700643404.314:3644): avc: denied { dac_override } for pid=83444 comm="smbd[192.168.12" capability=1 scontext=system_u:system_r:smbd_t:s0 tcontext=system_u:system_r:smb d_t:s0 tclass=capability permissive=0 type=SYSCALL msg=audit(1700643404.314:3644): arch=c000003e syscall=191 success=no exit=-61 a0=7ffea4096890 a1=7f5809ce635a a2=561c6dfe9d90 a3=1000 items=1 ppid=76437 pid=83444 auid=429496 7295 uid=2001 gid=0 euid=2001 suid=0 fsuid=2001 egid=2001 sgid=0 fsgid=2001 tty=(none) ses=4294967295 comm="smbd[192.168.12" exe="/usr/sbin/smbd" subj=system_u:system_r:smbd_t:s0 key=(null)AR CH=x86_64 SYSCALL=getxattr AUID="unset" UID="test1" GID="root" EUID="test1" SUID="root" FSUID="test1" EGID="test1" SGID="root" FSGID="test1"

anoopcs9 commented 10 months ago

Am I missing something to start with reviews?

anoopcs9 commented 9 months ago

@zpytela Should I create an issue before raising a pull request?

zpytela commented 9 months ago

I am sorry for the delay, it is actually better to create a PR. Still thinking about it and how can we avoid allowing the permission. How frequent such a configuration really is? Would a boolean needed to turn on be sufficient? Or having it completely on the admin's decision to create a local module?

anoopcs9 commented 9 months ago

I am sorry for the delay, it is actually better to create a PR.

Fine.

Still thinking about it and how can we avoid allowing the permission.

At least with the said configuration smbd might require extra privileges at times to overcome file system ACLs to correctly implement Windows ACL(NT ACL) semantics. This is completely client driven and not something server can predict before hand. Given the protective nature of ACLs in general they are chosen to be stored in xattr from security namespace on those files and directories on demand.

How frequent such a configuration really is?

It is more frequently used where ACL management details are important such that proper NT ACL behaviour is presented as expected by Windows clients.

Would a boolean needed to turn on be sufficient? Or having it completely on the admin's decision to create a local module?

Hm..may be that's debatable. Once setup such a configuration is expected to work out of the box without further admin intervention. Even if we go down that road coming up with a boolean should be the last resort.

Let me ask around and come back with a more decisive request.

anoopcs9 commented 8 months ago

Still thinking about it and how can we avoid allowing the permission.

At least with the said configuration smbd might require extra privileges at times to overcome file system ACLs to correctly implement Windows ACL(NT ACL) semantics. This is completely client driven and not something server can predict before hand. Given the protective nature of ACLs in general they are chosen to be stored in xattr from security namespace on those files and directories on demand.

How frequent such a configuration really is?

It is more frequently used where ACL management details are important such that proper NT ACL behaviour is presented as expected by Windows clients.

Would a boolean needed to turn on be sufficient? Or having it completely on the admin's decision to create a local module?

Hm..may be that's debatable. Once setup such a configuration is expected to work out of the box without further admin intervention. Even if we go down that road coming up with a boolean should be the last resort.

Let me ask around and come back with a more decisive request.

It seems that this is also a requirement for special cases in default/basic server setup. I could also find few other useful configuration parameters relying on CAP_DAC_OVERRIDE to manipulate permissions as and when required. As mentioned earlier these cannot be anticipated well before such that permissions are favourable to perform certain operations. Switching to use minimum required capability instead of _becomeroot() was found to be less expensive and perform better.

All these facts converge at a point where smbd is expected/allowed to operate with CAP_DAC_OVERRIDE to seamlessly provide the very same behaviour as before without an additional step on updated samba installations.

@zpytela Therefore I recommend to have dac_override added to the list of capabilities allowed for _smbdt.

zpytela commented 8 months ago

Merging then, thank you.