Re: [PATCH] lsm: security_task_getsecid_subj() -> security_current_getsecid_subj()

[Paul Moore <paul@paul-moore.com>]

On Mon, Nov 22, 2021 at 7:40 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 11/22/2021 3:12 PM, Paul Moore wrote:
> > On Fri, Nov 19, 2021 at 5:52 PM Paul Moore <paul@paul-moore.com> wrote:
> >> On Wed, Sep 29, 2021 at 3:17 PM Paul Moore <paul@paul-moore.com> wrote:
> >>> The security_task_getsecid_subj() LSM hook invites misuse by allowing
> >>> callers to specify a task even though the hook is only safe when the
> >>> current task is referenced.  Fix this by removing the task_struct
> >>> argument to the hook, requiring LSM implementations to use the
> >>> current task.  While we are changing the hook declaration we also
> >>> rename the function to security_current_getsecid_subj() in an effort
> >>> to reinforce that the hook captures the subjective credentials of the
> >>> current task and not an arbitrary task on the system.
> >>>
> >>> Signed-off-by: Paul Moore <paul@paul-moore.com>
> >>> ---
> >>>   include/linux/lsm_hook_defs.h         |    3 +--
> >>>   include/linux/lsm_hooks.h             |    8 +++-----
> >>>   include/linux/security.h              |    4 ++--
> >>>   kernel/audit.c                        |    4 ++--
> >>>   kernel/auditfilter.c                  |    3 +--
> >>>   kernel/auditsc.c                      |   10 +++++++++-
> >>>   net/netlabel/netlabel_unlabeled.c     |    2 +-
> >>>   net/netlabel/netlabel_user.h          |    2 +-
> >>>   security/apparmor/lsm.c               |   13 ++++++++++---
> >>>   security/integrity/ima/ima_appraise.c |    2 +-
> >>>   security/integrity/ima/ima_main.c     |   14 +++++++-------
> >>>   security/security.c                   |    6 +++---
> >>>   security/selinux/hooks.c              |   19 +++----------------
> >>>   security/smack/smack.h                |   16 ----------------
> >>>   security/smack/smack_lsm.c            |    9 ++++-----
> >>>   15 files changed, 48 insertions(+), 67 deletions(-)
> >> I never saw any comments, positive or negative, on this patch so I'll
> >> plan on merging it early next week.  If you've got objections, now is
> >> the time to speak up.
> >
> > I just merged this patch, with the AppArmor tweak suggested by Serge,
> > into selinux/next.  Thanks everyone.
>
> Has the security tree been abandoned as a path for general LSM
> changes? Except for the initial Landlock pull and a couple touch-ups
> to capabilities nothing has gone in via security this year. This
> change should have gone in through security, not selinux. I'm glad
> that this change is going in, don't get me wrong on that. I am
> somewhat concerned about the LSM infrastructure work I'm doing,
> and how it's going to get upstream. The diffstats from that look
> a lot like the one here. I seriously doubt that taking the full
> set of changes for stacking through the Smack tree is going to fly. ;)

I don't think there is ever a clear answer when you have a patch(set)
that crosses subsystem boundaries, there is always going to be a bit
of awkwardness and merge-fun for those affected.  If something touches
SELinux, and this patch qualifies as far as I'm concerned, I tend to
offer to take it via the SELinux tree (mostly for selfish reasons as
it helps with merge conflicts) unless James wants to pull it via the
LSM tree; in almost all of those cases James has deferred the changes
to the SELinux tree.  In this particular case I guess I didn't
explicitly ask about the LSM tree vs the SELinux tree, but the patch
has been on-list for weeks and it wasn't snatched up so I felt like
pulling it into the SELinux tree was justified.  If nothing else,
between the SELinux, audit, and netlabel changes I guess I can claim
some level of merge authority ;)

However, regardless of this particular patchset, I agree that the LSM
stacking patches should almost surely go in via the LSM tree.

-- 
paul moore
www.paul-moore.com