[RFC][PATCH] security: Make the selinux setxattr and removexattr hooks behave
Eric W. Biederman
ebiederm at xmission.com
Sun Oct 1 01:02:01 UTC 2017
Casey Schaufler <casey at schaufler-ca.com> writes:
> 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?
I don't have a smack configuration handy, but reading through
the code smack setxattr the permission checks for all xattrs
that are not smack xattrs to cap_inode_setxattr.
So smack and commoncap combined will not fail.
smack and selinux will result in people who should be able to set
selinux xattrs not being able to. That however is less of an immediate
problem.
>> 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.
Apologies for not being clearer earlier, but I was still in shock from
running into a brick wall.
>> 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.
So far there are exactly two implementation of
LSM_HOOK_INIT(inode_setxattr, ...)
So as a practical case it looks like combination with selinux is the
only case where the problem will be observed right now. And it makes
the code at least somewhat match the comments in
security_inode_setxattr.
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
More information about the Linux-security-module-archive
mailing list