Re: [PATCH v7 10/11] SELinux: Add selfattr hooks

From: Paul Moore
Date: Thu Mar 30 2023 - 19:33:11 EST

On Thu, Mar 30, 2023 at 4:55 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> On 3/29/2023 6:13 PM, Paul Moore wrote:
> > On Wed, Mar 15, 2023 at 6:52 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> >> Cc: selinux@xxxxxxxxxxxxxxx
> >> Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
> >> ---
> >> 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.

Okay, I'll accept that argument, but I would ask that add some
additional handling in selinux_getprocattr() so that it returns
-EINVAL in this case just as it does today.

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

Also, same as above.

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

I'm still a big fan and I'm sticking to the 80 char limit for the LSM
layer as well as the SELinux, audit, and labeled networking
subsystems. Longer lines either predate me or I simply didn't catch
them during review/merge.

--
paul-moore.com