Casey Schaufler [off-list ref] writes:

On 9/29/2017 7:18 AM, Stephen Smalley wrote:

quoted

On Thu, 2017-09-28 at 18:16 -0700, Casey Schaufler wrote:

quoted

On 9/28/2017 3:34 PM, Eric W. Biederman wrote:

quoted

It looks like once upon a time a long time ago selinux copied code
from cap_inode_removexattr and cap_inode_setxattr into
selinux_inode_setotherxattr.??However the code has now diverged and
selinux is implementing a policy that is quite different than
cap_inode_setxattr and cap_inode_removexattr especially when it
comes
to the security.capable xattr.

What leads you to believe that this isn't intentional?
It's most likely the case that this change occurred as
part of the first round module stacking change. What behavior
do you see that you're unhappy with?

quoted

To keep things working

Which "things"? How are they not "working"?

quoted

?and to make the comments in security/security.c
correct when the xattr is securit.capable, call cap_inode_setxattr
or cap_inode_removexattr as appropriate.

I suspect there is a larger conversation to be had here but this
is enough to keep selinux from implementing a non-sense hard coded
policy that breaks other parts of the kernel.

Specifics, please. Since I can't guess what problem you've
encountered I can't tell if it's here, in the infrastructure,
or in your perception of what constitutes "broken".

quoted

Signed-off-by: "Eric W. Biederman" <redacted>
---
?security/selinux/hooks.c | 6 ++++++
?1 file changed, 6 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f5d304736852..edf4bd292dc7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c

@@ -3167,6 +3167,9 @@ static int selinux_inode_setxattr(struct

dentry *dentry, const char *name,
?	u32 newsid, sid = current_sid();
?	int rc = 0;
?
+	if (strcmp(name, XATTR_NAME_CAPS) == 0)
+		return cap_inode_setxattr(dentry, name, value,
size, flags);
+

No. Don't even think of contemplating considering embedding the cap
attribute check in the SELinux code. cap_inode_setxattr() is called
in
the infrastructure.

Except that it isn't, not if any other security module is enabled and
implements those hooks, to prevent imposing CAP_SYS_ADMIN checks when
setting security.selinux or security.SMACK*.

OK. Yes, this bit of the infrastructure is some of the
worst I've done in a long time. This is a case where we
already need special case stacking infrastructure. It looks
like we'll have to separate setting the cap attribute from
checking the cap state in order to make this work. In any
case, the security_inode_setxattr() code is where the change
belongs. There will likely be fallout changes in the modules,
including the cap module.
?

quoted

An alternative approach to fixing this would be to change the cap
functions to only apply their checks if setting the capability
attribute and defer any checks on other security.* attributes to either
the security framework or the other security modules.  Then the
framework could always call all the modules on the inode_setxattr and
inode_removexattr hooks as with other hooks.  The security framework
would then need to ensure that a check is still applied when setting
security.* attributes if it isn't already handled by one of the enabled
security modules, as you don't want unprivileged userspace to be able
to set arbitrary security.foo attributes or to set up security.selinux
or security.SMACK* attributes if those modules happen to be disabled.

Agreed. This isn't a two line change. Grumble.

I can guess at what the problem might be, but I hate making
assumptions when I go to fix a problem. I will start looking
at a patch, but it would really help if I could say for sure
what I'm out to accomplish. It may be obvious to the casual
observer, but that description has not been applied to me very
often.

Apologies for the delayed reply.

I am looking at security_inode_setxattr.

For setting attributes in the security.* the generic code in fs/xattr.c
applies no permission checks.

Each security module that implements an xattr in security.* then imposes
it's own policy on it's own attribute.

For smack the basic rule is smack_privileged(CAP_MAC_ADMIN).
For selinux the basic rule is inode_or_owner_capable(inode).
For commoncap the basic rule is capable_wrt_inode_uidgid(inode, CAP_SETFCAP).

commoncap also applies a default policity to setting security.* xattrs.
ns_capable(dentry->d_sb->s_userns, CAP_SYS_ADMIN).

smack reuses that default policy by calling cap_inode_setxattr if it
isn't a smack security.* xattr.

selinux has what looks like an old copy of the commoncap checks for
the security.* in selinux_inode_setotherxattr.  Testing for
capable(CAP_SETFCAP) for security.capable and capable(CAP_SYS_ADMIN)
for the others.

With the added complication that selinux calls
selinux_inode_setotherxattr also for the remove_xattr case.  So fixing
this in selinux_inode_setotherxattr is not appropriate.

I believe selinux also has general policy hooks it applies to all
invocations of setxattr.

So I think to really fix this we need to separate the cases of is this
your security modules attribute from general policy checks added by the
security modules.  Perhaps something like this for
security_inode_setxattr:

Hmm.  Looking at least ima also has the distinction between protecting
it's own xattr writes and running generaly security module policy on
xattr writes.

int security_inode_setxattr(struct dentry *dentry, const char *name,
			    const void *value, size_t size, int flags)
{
	int ret = 0;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;

	if (strncmp(name, XATTR_SECURITY_PREFIX,
			sizeof(XATTR_SECURITY_PREFIX) - 1) == 0) {
		/* Call the security modules and see if they all return
                 * -EOPNOTSUPP if so apply the default permission
                 * check of ns_capable(dentry->d_sb->s_user_ns, CAP_SYS_ADMIN)
                 * otherwise if one of the security modules supports
		 * this attribute (signaled by returning something other
		 * -EOPNOTSUPP) then set ret to that result.
                 *
                 * The security modules include at least smack, selinux,
		 * commoncap, ima, and evm.
                 */
                 ret = magic_inode_protect_setxattr(dentry, name, value, size);
        }
	if (ret)
		return ret;

        /* Run all of the security module policy against this setxattr call */
        return magic_inode_policy_setxattr(dentry, name, value, size);
}

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html