Re: [PATCH] libselinux: Use sestatus if open

[Mike Palmiotto <mike.palmiotto@crunchydata.com>]

On Tue, Jul 14, 2020 at 5:35 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 5:03 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
<snip>
>
> In its current form, your patch replaces one call in
> avc_has_perm_noaudit() to avc_netlink_check_nb() with a call to
> selinux_status_updated().  With that change alone, for existing
> callers of avc_has_perm_noaudit() that do not themselves call
> selinux_status_open(), selinux_status_updated() will find that the
> status page is NULL and will return immediately with an error. It
> won't fall back to probing the netlink socket.  So you either need to
> test for an error return and fall back yourself to
> avc_netlink_check_nb(), or change selinux_status_updated() to do that,
> or replace the use of avc_netlink_open/loop/close in avc.c with
> selinux_status_open/close.  Likewise for selinux_check_access().

For some reason I completely glossed over the selinux_status == NULL
case. I guess I didn't see errors when testing dbus because verbose
logging wasn't turned on.

I'll take a look at what it would take to move over sestatus entirely
and if it's too tricky/error-prone, _I think_ it should be fine to
move the selinux_status == NULL check down with the MAP_FAILED case so
they both fall through to avc_netlink_check_nb().

>
> > Do you think we should go ahead and completely swap in sestatus? I was
> > just worried about breaking userspace object managers that are
> > currently using netlink threads by default, for instance
> > systemd-dbusd.  I can spend some more time getting those to work with
> > the status page if you think that's worthwhile.
>
> I'd be interested in understanding the impact of such a change on
> existing userspace object managers.  If we can switch the default
> behavior for applications that are not explicitly using
> avc_netlink_*() interfaces themselves (e.g. they are only using
> selinux_check_access or avc_has_perm), then that would be beneficial
> since I think it fully removes the need for a system call on the AVC
> cache-hit code path.

I'll have to do a bit of digging to see how this will affect dbus, et
al. On first blush, it looks like they're just doing
avc_netlink_check_nb() in their watch thread. Presumably other object
managers are doing something similar so we would just need to make
sure there is a netlink fd available.

>
> > > Finally, I don't think you need to sanitize
> > > the enforcing value from the kernel; it takes care of that itself
> > > these days and no point in fixing it up for old kernels now.
> >
> > Very good to know, thanks! I'll update in the next version.



-- 
Mike Palmiotto
https://crunchydata.com