On Thu, Apr 20, 2017 at 11:31 AM, Stephen Smalley <[email protected]> wrote:
> SELinux uses CAP_MAC_ADMIN to control the ability to get or set a raw,
> uninterpreted security context unknown to the currently loaded security
> policy. When performing these checks, we only want to perform a base
> capabilities check and a SELinux permission check. If any other
> modules that implement a capable hook are stacked with SELinux, we do
> not want to require them to also have to authorize CAP_MAC_ADMIN,
> since it may have different implications for their security model.
> Rework the CAP_MAC_ADMIN checks within SELinux to only invoke the
> capabilities module and the SELinux permission checking.
>
> Signed-off-by: Stephen Smalley <[email protected]>
> ---
> security/selinux/hooks.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e67a526..1aef63c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3107,6 +3107,18 @@ static int selinux_inode_setotherxattr(struct dentry
> *dentry, const char *name)
> return dentry_has_perm(cred, dentry, FILE__SETATTR);
> }
>
> +static bool has_cap_mac_admin(bool audit)
> +{
> + const struct cred *cred = current_cred();
> + int cap_audit = audit ? SECURITY_CAP_AUDIT : SECURITY_CAP_NOAUDIT;
> +
> + if (cap_capable(cred, &init_user_ns, CAP_MAC_ADMIN, cap_audit))
> + return false;
> + if (cred_has_capability(cred, CAP_MAC_ADMIN, cap_audit, true))
> + return false;
> + return true;
> +}
Granted, I'm being nitpicky here, but that is one awful function name
IMHO. What's wrong with current_cap_mac_admin()?
Unless you're really bored don't worry about a respin, the rest of the
patch looks fine to me, I'll queue it up for after the next merge
window.
> static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
> const void *value, size_t size, int flags)
> {
> @@ -3138,7 +3150,7 @@ static int selinux_inode_setxattr(struct dentry
> *dentry, const char *name,
>
> rc = security_context_to_sid(value, size, &newsid, GFP_KERNEL);
> if (rc == -EINVAL) {
> - if (!capable(CAP_MAC_ADMIN)) {
> + if (!has_cap_mac_admin(true)) {
> struct audit_buffer *ab;
> size_t audit_size;
> const char *str;
> @@ -3264,13 +3276,8 @@ static int selinux_inode_getsecurity(struct inode
> *inode, const char *name, void
> * and lack of permission just means that we fall back to the
> * in-core context value, not a denial.
> */
> - error = cap_capable(current_cred(), &init_user_ns, CAP_MAC_ADMIN,
> - SECURITY_CAP_NOAUDIT);
> - if (!error)
> - error = cred_has_capability(current_cred(), CAP_MAC_ADMIN,
> - SECURITY_CAP_NOAUDIT, true);
> isec = inode_security(inode);
> - if (!error)
> + if (has_cap_mac_admin(false))
> error = security_sid_to_context_force(isec->sid, &context,
> &size);
> else
> @@ -5918,7 +5925,7 @@ static int selinux_setprocattr(const char *name, void
> *value, size_t size)
> }
> error = security_context_to_sid(value, size, &sid,
> GFP_KERNEL);
> if (error == -EINVAL && !strcmp(name, "fscreate")) {
> - if (!capable(CAP_MAC_ADMIN)) {
> + if (!has_cap_mac_admin(true)) {
> struct audit_buffer *ab;
> size_t audit_size;
>
> --
> 2.9.3
>
--
paul moore
www.paul-moore.com
