[RFC][PATCH] security: Make the selinux setxattr and removexattr hooks behave

[Casey Schaufler]

On 9/30/2017 1:40 PM, Eric W. Biederman wrote:
> Casey Schaufler <casey at schaufler-ca.com> writes:
>
>> On 9/30/2017 9:22 AM, Eric W. Biederman wrote:
>>> Casey Schaufler <casey at schaufler-ca.com> writes:
>>>
>>>> On 9/29/2017 7:18 AM, Stephen Smalley wrote:
>>>>> On Thu, 2017-09-28 at 18:16 -0700, Casey Schaufler wrote:
>>>>>> On 9/28/2017 3:34 PM, Eric W. Biederman wrote:
>>>>>>> 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?
>>>>>>
>>>>>>> To keep things working
>>>>>> Which "things"? How are they not "working"?
>>>>>>
>>>>>>>  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".
>>>>>>
>>>>>>> Signed-off-by: "Eric W. Biederman" <ebiederm at xmission.com>
>>>>>>> ---
>>>>>>>  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.
>>>>  
>>>>
>>>>> 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
>> Yup, that's pretty much what I'm thinking. It's unfortunate
>> that the magic_ API isn't fully implemented. There's going to
>> be a good deal of code surgery instead. Is there an observed
>> problem today? This is going to have to get addressed for stacking,
>> so if there isn't a behavioral issue that impacts something real
>> I would like to defer spending significant time on it. Do you have
>> a case where this is not working correctly?
> Merged as of 4.14-rc1 is the support for user namespace root to set
> sercurity.capable.  This fails when selinux is loaded.

OK. Is the failure unique to SELinux, or does it fail with
Smack as well?

> removexattr has the same problem and the code is a little less
> convoluted in that case.

Right. Because removexattr is a simpler situation.

> Not being able to set the capability when you should be able to is
> very noticable.  Like running into a brick wall noticable.

Ah, now you've identified the problem. Yes, I would agree that you've
uncovered an undesirable behavior.

> Which is where the minimal patch for selinux comes in.  I think it
> solves the exact case in question, even if it isn't the perfect long
> term solution.

If the problem is unique to SELinux I can see your logic. If it
isn't, that is, if it's also a problem with any other security
module, there either needs to be a fix for that/those module/s
as well or a "real" fix.

I'm not opposed to the SELinux short term fix if you can say
that that's the only module with the problem.

>
> 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
>


.
--
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