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;
MIME-Version: 1.0
References: <20230315224704.2672-1-casey@schaufler-ca.com> <20230315224704.2672-11-casey@schaufler-ca.com>
 <CAHC9VhTyMmyB5Yr8Zp+Xg3R=J9VLp-oChxJPcAv+fL8czVzcYg@mail.gmail.com> <61d21f68-8e84-ad85-ef20-fced8c8b916d@schaufler-ca.com>
In-Reply-To: <61d21f68-8e84-ad85-ef20-fced8c8b916d@schaufler-ca.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Thu, 30 Mar 2023 19:32:54 -0400
Message-ID: <CAHC9VhS+rh-pRshAQcJPGYBx98=NW17XPKSAM8d7ahPBTv3_2A@mail.gmail.com>
Subject: Re: [PATCH v7 10/11] SELinux: Add selfattr hooks
To:     Casey Schaufler <casey@schaufler-ca.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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Mar 30, 2023 at 4:55=E2=80=AFPM Casey Schaufler <casey@schaufler-ca=
.com> wrote:
> On 3/29/2023 6:13 PM, Paul Moore wrote:
> > On Wed, Mar 15, 2023 at 6:52=E2=80=AFPM Casey Schaufler <casey@schaufle=
r-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_str=
uct *p,
> >>                         goto bad;
> >>         }
> >>
> >> -       if (!strcmp(name, "current"))
> >> +       switch (attr) {
> >> +       case LSM_ATTR_CURRENT:
> >>                 sid =3D __tsec->sid;
> >> -       else if (!strcmp(name, "prev"))
> >> +               break;
> >> +       case LSM_ATTR_PREV:
> >>                 sid =3D __tsec->osid;
> >> -       else if (!strcmp(name, "exec"))
> >> +               break;
> >> +       case LSM_ATTR_EXEC:
> >>                 sid =3D __tsec->exec_sid;
> >> -       else if (!strcmp(name, "fscreate"))
> >> +               break;
> >> +       case LSM_ATTR_FSCREATE:
> >>                 sid =3D __tsec->create_sid;
> >> -       else if (!strcmp(name, "keycreate"))
> >> +               break;
> >> +       case LSM_ATTR_KEYCREATE:
> >>                 sid =3D __tsec->keycreate_sid;
> >> -       else if (!strcmp(name, "sockcreate"))
> >> +               break;
> >> +       case LSM_ATTR_SOCKCREATE:
> >>                 sid =3D __tsec->sockcreate_sid;
> >> -       else {
> >> -               error =3D -EINVAL;
> >> +               break;
> >> +       default:
> >> +               error =3D -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_struc=
t *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 *nam=
e, 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 =3D avc_has_perm(&selinux_state,
> >> +                                    mysid, mysid, SECCLASS_PROCESS,
> >> +                                    PROCESS__SETCURRENT, NULL);
> >> +               break;
> >> +       case LSM_ATTR_EXEC:
> >>                 error =3D avc_has_perm(&selinux_state,
> >>                                      mysid, mysid, SECCLASS_PROCESS,
> >>                                      PROCESS__SETEXEC, NULL);
> >> -       else if (!strcmp(name, "fscreate"))
> >> +               break;
> >> +       case LSM_ATTR_FSCREATE:
> >>                 error =3D avc_has_perm(&selinux_state,
> >>                                      mysid, mysid, SECCLASS_PROCESS,
> >>                                      PROCESS__SETFSCREATE, NULL);
> >> -       else if (!strcmp(name, "keycreate"))
> >> +               break;
> >> +       case LSM_ATTR_KEYCREATE:
> >>                 error =3D avc_has_perm(&selinux_state,
> >>                                      mysid, mysid, SECCLASS_PROCESS,
> >>                                      PROCESS__SETKEYCREATE, NULL);
> >> -       else if (!strcmp(name, "sockcreate"))
> >> +               break;
> >> +       case LSM_ATTR_SOCKCREATE:
> >>                 error =3D avc_has_perm(&selinux_state,
> >>                                      mysid, mysid, SECCLASS_PROCESS,
> >>                                      PROCESS__SETSOCKCREATE, NULL);
> >> -       else if (!strcmp(name, "current"))
> >> -               error =3D avc_has_perm(&selinux_state,
> >> -                                    mysid, mysid, SECCLASS_PROCESS,
> >> -                                    PROCESS__SETCURRENT, NULL);
> >> -       else
> >> -               error =3D -EINVAL;
> >> +               break;
> >> +       default:
> >> +               error =3D -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 *nam=
e, void *value, size_t size)
> >>                 }
> >>                 error =3D security_context_to_sid(&selinux_state, valu=
e, size,
> >>                                                 &sid, GFP_KERNEL);
> >> -               if (error =3D=3D -EINVAL && !strcmp(name, "fscreate"))=
 {
> >> +               if (error =3D=3D -EINVAL && attr =3D=3D LSM_ATTR_FSCRE=
ATE) {
> >>                         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 shoul=
d audit that */
> >> +                               /* We strip a nul only if it is at the=
 end,
> >> +                                * otherwise the context contains a nu=
l 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.

--=20
paul-moore.com