Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <61d21f68-8e84-ad85-ef20-fced8c8b916d@schaufler-ca.com>
Date:   Thu, 30 Mar 2023 13:55:35 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v7 10/11] SELinux: Add selfattr hooks
Content-Language: en-US
To:     Paul Moore <paul@paul-moore.com>
Cc:     linux-security-module@vger.kernel.org, jmorris@namei.org,
        keescook@chromium.org, john.johansen@canonical.com,
        penguin-kernel@i-love.sakura.ne.jp, stephen.smalley.work@gmail.com,
        linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
        mic@digikod.net, selinux@vger.kernel.org,
        Casey Schaufler <casey@schaufler-ca.com>
References: <20230315224704.2672-1-casey@schaufler-ca.com>
 <20230315224704.2672-11-casey@schaufler-ca.com>
 <CAHC9VhTyMmyB5Yr8Zp+Xg3R=J9VLp-oChxJPcAv+fL8czVzcYg@mail.gmail.com>
From:   Casey Schaufler <casey@schaufler-ca.com>
In-Reply-To: <CAHC9VhTyMmyB5Yr8Zp+Xg3R=J9VLp-oChxJPcAv+fL8czVzcYg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/29/2023 6:13 PM, Paul Moore wrote:
> On Wed, Mar 15, 2023 at 6:52 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Add hooks for setselfattr and getselfattr. These hooks are not very
>> different from their setprocattr and getprocattr equivalents, and
>> much of the code is shared.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Cc: selinux@vger.kernel.org
>> Cc: Paul Moore <paul@paul-moore.com>
>> ---
>>  security/selinux/hooks.c | 147 +++++++++++++++++++++++++++++++--------
>>  1 file changed, 117 insertions(+), 30 deletions(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 9403aee75981..8896edf80aa9 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -6348,8 +6348,7 @@ static void selinux_d_instantiate(struct dentry *dentry, struct inode *inode)
>>                 inode_doinit_with_dentry(inode, dentry);
>>  }
>>
>> -static int selinux_getprocattr(struct task_struct *p,
>> -                              const char *name, char **value)
>> +static int do_getattr(unsigned int attr, struct task_struct *p, char **value)
> Are you ready for more naming nitpicks? ;)

I would expect nothing less. :)

> Let's call this 'selinux_lsm_getattr()', and the matching setter
> should be 'selinux_lsm_setattr()'.

As you wish. It's your LSM.


>>  {
>>         const struct task_security_struct *__tsec;
>>         u32 sid;
>> @@ -6367,20 +6366,27 @@ static int selinux_getprocattr(struct task_struct *p,
>>                         goto bad;
>>         }
>>
>> -       if (!strcmp(name, "current"))
>> +       switch (attr) {
>> +       case LSM_ATTR_CURRENT:
>>                 sid = __tsec->sid;
>> -       else if (!strcmp(name, "prev"))
>> +               break;
>> +       case LSM_ATTR_PREV:
>>                 sid = __tsec->osid;
>> -       else if (!strcmp(name, "exec"))
>> +               break;
>> +       case LSM_ATTR_EXEC:
>>                 sid = __tsec->exec_sid;
>> -       else if (!strcmp(name, "fscreate"))
>> +               break;
>> +       case LSM_ATTR_FSCREATE:
>>                 sid = __tsec->create_sid;
>> -       else if (!strcmp(name, "keycreate"))
>> +               break;
>> +       case LSM_ATTR_KEYCREATE:
>>                 sid = __tsec->keycreate_sid;
>> -       else if (!strcmp(name, "sockcreate"))
>> +               break;
>> +       case LSM_ATTR_SOCKCREATE:
>>                 sid = __tsec->sockcreate_sid;
>> -       else {
>> -               error = -EINVAL;
>> +               break;
>> +       default:
>> +               error = -EOPNOTSUPP;
> The error should probably be -EINVAL.

It's possible that we may add an attribute that SELinux doesn't
support, say LSM_ATTR_CRYPTO_KEY, that another LSM does. This is
the same behavior the other LSMs exhibit in the face of a request
for attributes such as LSM_ATTR_KEYCREATE that they don't support.


>>                 goto bad;
>>         }
>>         rcu_read_unlock();
>> @@ -6398,7 +6404,7 @@ static int selinux_getprocattr(struct task_struct *p,
>>         return error;
>>  }
>>
>> -static int selinux_setprocattr(const char *name, void *value, size_t size)
>> +static int do_setattr(u64 attr, void *value, size_t size)
>>  {
>>         struct task_security_struct *tsec;
>>         struct cred *new;
>> @@ -6409,28 +6415,36 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>>         /*
>>          * Basic control over ability to set these attributes at all.
>>          */
>> -       if (!strcmp(name, "exec"))
>> +       switch (attr) {
>> +       case LSM_ATTR_CURRENT:
>> +               error = avc_has_perm(&selinux_state,
>> +                                    mysid, mysid, SECCLASS_PROCESS,
>> +                                    PROCESS__SETCURRENT, NULL);
>> +               break;
>> +       case LSM_ATTR_EXEC:
>>                 error = avc_has_perm(&selinux_state,
>>                                      mysid, mysid, SECCLASS_PROCESS,
>>                                      PROCESS__SETEXEC, NULL);
>> -       else if (!strcmp(name, "fscreate"))
>> +               break;
>> +       case LSM_ATTR_FSCREATE:
>>                 error = avc_has_perm(&selinux_state,
>>                                      mysid, mysid, SECCLASS_PROCESS,
>>                                      PROCESS__SETFSCREATE, NULL);
>> -       else if (!strcmp(name, "keycreate"))
>> +               break;
>> +       case LSM_ATTR_KEYCREATE:
>>                 error = avc_has_perm(&selinux_state,
>>                                      mysid, mysid, SECCLASS_PROCESS,
>>                                      PROCESS__SETKEYCREATE, NULL);
>> -       else if (!strcmp(name, "sockcreate"))
>> +               break;
>> +       case LSM_ATTR_SOCKCREATE:
>>                 error = avc_has_perm(&selinux_state,
>>                                      mysid, mysid, SECCLASS_PROCESS,
>>                                      PROCESS__SETSOCKCREATE, NULL);
>> -       else if (!strcmp(name, "current"))
>> -               error = avc_has_perm(&selinux_state,
>> -                                    mysid, mysid, SECCLASS_PROCESS,
>> -                                    PROCESS__SETCURRENT, NULL);
>> -       else
>> -               error = -EINVAL;
>> +               break;
>> +       default:
>> +               error = -EOPNOTSUPP;
> Same as above, should be -EINVAL.

Same as above, there may be attributes SELinux doesn't support.


>> +               break;
>> +       }
>>         if (error)
>>                 return error;
>>
>> @@ -6442,13 +6456,14 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>>                 }
>>                 error = security_context_to_sid(&selinux_state, value, size,
>>                                                 &sid, GFP_KERNEL);
>> -               if (error == -EINVAL && !strcmp(name, "fscreate")) {
>> +               if (error == -EINVAL && attr == LSM_ATTR_FSCREATE) {
>>                         if (!has_cap_mac_admin(true)) {
>>                                 struct audit_buffer *ab;
>>                                 size_t audit_size;
>>
>> -                               /* We strip a nul only if it is at the end, otherwise the
>> -                                * context contains a nul and we should audit that */
>> +                               /* We strip a nul only if it is at the end,
>> +                                * otherwise the context contains a nul and
>> +                                * we should audit that */
> You *do* get gold stars for fixing line lengths in close proximity ;)

I guess I'm the Last User of the 80 character terminal.

>>                                 if (str[size - 1] == '\0')
>>                                         audit_size = size - 1;
>>                                 else
>> @@ -6459,7 +6474,8 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>>                                 if (!ab)
>>                                         return error;
>>                                 audit_log_format(ab, "op=fscreate invalid_context=");
>> -                               audit_log_n_untrustedstring(ab, value, audit_size);
>> +                               audit_log_n_untrustedstring(ab, value,
>> +                                                           audit_size);
>>                                 audit_log_end(ab);
>>
>>                                 return error;
>> @@ -6483,11 +6499,11 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>>            checks and may_create for the file creation checks. The
>>            operation will then fail if the context is not permitted. */
>>         tsec = selinux_cred(new);
>> -       if (!strcmp(name, "exec")) {
>> +       if (attr == LSM_ATTR_EXEC) {
>>                 tsec->exec_sid = sid;
>> -       } else if (!strcmp(name, "fscreate")) {
>> +       } else if (attr == LSM_ATTR_FSCREATE) {
>>                 tsec->create_sid = sid;
>> -       } else if (!strcmp(name, "keycreate")) {
>> +       } else if (attr == LSM_ATTR_KEYCREATE) {
>>                 if (sid) {
>>                         error = avc_has_perm(&selinux_state, mysid, sid,
>>                                              SECCLASS_KEY, KEY__CREATE, NULL);
>> @@ -6495,9 +6511,9 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>>                                 goto abort_change;
>>                 }
>>                 tsec->keycreate_sid = sid;
>> -       } else if (!strcmp(name, "sockcreate")) {
>> +       } else if (attr == LSM_ATTR_SOCKCREATE) {
>>                 tsec->sockcreate_sid = sid;
>> -       } else if (!strcmp(name, "current")) {
>> +       } else if (attr == LSM_ATTR_CURRENT) {
>>                 error = -EINVAL;
>>                 if (sid == 0)
>>                         goto abort_change;
>> @@ -6542,6 +6558,75 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>>         return error;
>>  }
>>
>> +static int selinux_getselfattr(unsigned int __user attr,
>> +                              struct lsm_ctx __user *ctx, size_t *size,
>> +                              u32 __user flags)
>> +{
>> +       char *value;
>> +       size_t total_len;
>> +       int len;
>> +       int rc = 0;
>> +
>> +       len = do_getattr(attr, current, &value);
>> +       if (len < 0)
>> +               return len;
>> +
>> +       total_len = len + sizeof(*ctx);
>> +
>> +       if (total_len > *size)
>> +               rc = -E2BIG;
>> +       else
>> +               lsm_fill_user_ctx(ctx, value, len, LSM_ID_SELINUX, 0);
>> +
>> +       *size = total_len;
>> +       return rc;
>> +}
>> +
>> +static int selinux_setselfattr(unsigned int __user attr,
>> +                              struct lsm_ctx __user *ctx, size_t __user size,
>> +                              u32 __user flags)
>> +{
>> +       struct lsm_ctx *lctx;
>> +       void *context;
>> +       int rc;
>> +
>> +       context = kmalloc(size, GFP_KERNEL);
>> +       if (context == NULL)
>> +               return -ENOMEM;
>> +
>> +       lctx = (struct lsm_ctx *)context;
>> +       if (copy_from_user(context, ctx, size))
>> +               rc = -EFAULT;
>> +       else if (lctx->ctx_len > size)
>> +               rc = -EINVAL;
>> +       else
>> +               rc = do_setattr(attr, lctx + 1, lctx->ctx_len);
>> +
>> +       kfree(context);
>> +       if (rc > 0)
>> +               return 0;
>> +       return rc;
>> +}
>> +
>> +static int selinux_getprocattr(struct task_struct *p,
>> +                              const char *name, char **value)
>> +{
>> +       unsigned int attr = lsm_name_to_attr(name);
>> +
>> +       if (attr)
>> +               return do_getattr(attr, p, value);
>> +       return -EINVAL;
>> +}
>> +
>> +static int selinux_setprocattr(const char *name, void *value, size_t size)
>> +{
>> +       int attr = lsm_name_to_attr(name);
>> +
>> +       if (attr)
>> +               return do_setattr(attr, value, size);
>> +       return -EINVAL;
>> +}
>> +
>>  static int selinux_ismaclabel(const char *name)
>>  {
>>         return (strcmp(name, XATTR_SELINUX_SUFFIX) == 0);
>> @@ -7183,6 +7268,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>>
>>         LSM_HOOK_INIT(d_instantiate, selinux_d_instantiate),
>>
>> +       LSM_HOOK_INIT(getselfattr, selinux_getselfattr),
>> +       LSM_HOOK_INIT(setselfattr, selinux_setselfattr),
>>         LSM_HOOK_INIT(getprocattr, selinux_getprocattr),
>>         LSM_HOOK_INIT(setprocattr, selinux_setprocattr),
>>
>> --
>> 2.39.2
> --
> paul-moore.com