netdev - Re: [PATCH v4 27/42] selinux: introduce task_obj

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEjxPJ5cZB1zz3dCUz7ix-WW4P6Q7QaeE02TjcU6zg+SOg8KLA@mail.gmail.com>
Date: Fri, 13 Jun 2025 14:18:54 -0400
From: Stephen Smalley <stephen.smalley.work@...il.com>
To: Simon Horman <horms@...nel.org>
Cc: selinux@...r.kernel.org, paul@...l-moore.com, omosnace@...hat.com, 
	netdev@...r.kernel.org
Subject: Re: [PATCH v4 27/42] selinux: introduce task_obj_perm()

On Fri, Jun 13, 2025 at 10:15 AM Simon Horman <horms@...nel.org> wrote:
>
> On Tue, Jun 10, 2025 at 01:21:58PM -0400, Stephen Smalley wrote:
> > Introduce task_obj_perm() for namespace-aware permission checking
> > between two tasks using the objective SID for both tasks and
> > without assuming that either task is current.
> >
> > Convert the permission checks of this form in the hook functions
> > to use this new helper.
> >
> > Signed-off-by: Stephen Smalley <stephen.smalley.work@...il.com>
>
> ...
>
> > +int task_obj_has_perm(const struct task_struct *s,
> > +                   const struct task_struct *t,
> > +                   u16 tclass, u32 requested,
> > +                   struct common_audit_data *ad)
> > +{
> > +     const struct cred *cred;
> > +     const struct task_security_struct *tsec;
> > +     struct selinux_state *state;
> > +     u32 ssid;
> > +     u32 tsid;
> > +     int rc;
> > +
> > +     state = current_selinux_state;
> > +     rcu_read_lock();
> > +     tsec = task_security(s);
> > +     if (tsec)
> > +             ssid = tsec->sid;
> > +     else
> > +             ssid = SECINITSID_UNLABELED;
>
> Hi Stephen,
>
> Above it is assumed that tsec may be NULL...
>
> > +     rcu_read_unlock();
> > +
> > +     do {
> > +             tsid = task_sid_obj_for_state(t, state);
> > +
> > +             rc = avc_has_perm(state, ssid, tsid, tclass, requested, ad);
> > +             if (rc)
> > +                     return rc;
> > +
> > +             cred = tsec->parent_cred;
>
> ... but here tsec is dereferenced without explicitly checking if it is not
> NULL. I'm wondering if this is safe, e.g. due to the call to avc_has_perm().

No, you are correct - this is a bug. Thank you, fixed via
https://github.com/stephensmalley/selinux-kernel/commit/85e72ed549d01a2da407feef6493cbdeca324f82
and will likely squash into this patch on next submission.

>
> Flagged by Smatch.
>
> > +             if (!cred)
> > +                     break;
> > +
> > +             rcu_read_lock();
> > +             tsec = selinux_cred(cred);
> > +             ssid = tsec->sid;
> > +             state = tsec->state;
> > +             rcu_read_unlock();
> > +     } while (cred);
> > +
> > +     return 0;
> > +}
>
> ...