Re: [PATCH] selinux: allow reading labels before policy is loaded

[Stephen Smalley <stephen.smalley.work@gmail.com>]

On Wed, May 27, 2020 at 4:23 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Tue, May 26, 2020 at 9:12 PM Jonathan Lebon <jlebon@redhat.com> wrote:
> > On Mon, May 25, 2020 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > I might be missing something, but couldn't you achieve the same by
> > > simply adding something like this in here:
> > >
> > > if (!selinux_initialized(&selinux_state))
> > >         return -EOPNOTSUPP;
> > >
> > > (Or by adding it to the condition above.)
> > >
> > > Then you should hit this condition here and be all set:
> > > https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337
> >
> > Hi Ondrej,
> >
> > Yes, that looks promising. Two questions with that approach:
> >
> > 1. Is there a concern here with transiently returning -EOPNOTSUPP even
> > if the SELinux LSM does technically support the inode_getsecurity
> > hook? I'm thinking of potential corner-cases down the road where
> > somehow this knowledge is cached.
>
> I would hope not. I don't think it's likely this would be cached,
> since it would require a guarantee from all LSMs that they won't flip
> from -EOPNOTSUPP to something else... That would be error-prone IMHO.
>
> >
> > 2. The selinux_inode_getsecurity hook today does somewhat handle the
> > uninitialized case. It ends up here:
> >
> > https://elixir.bootlin.com/linux/v5.7-rc7/source/security/selinux/ss/services.c#L1322.
> >
> > Specifically, it has support for initial SIDs. The patch I wrote
> > purposely tries to allow falling back to that logic. Is there a
> > concern with short-circuiting this logic even if the inode SID somehow
> > isn't SECINITSID_UNLABELED?
>
> Oh, right, so that's what I missed :) I'll have to defer to Stephen on
> whether this is a concern... Obviously we lose the previous behavior
> of returning the initial SID strings via getxattr(), but I'm not sure
> if that's significant. Since those strings obviously aren't real
> contexts, it seems they only serve an informational purpose.
>
> Anyway, I looked at the original patch again and it generally looks
> sane. I don't like the fact that we need to call back to
> __vfs_getxattr() in yet another place, but it makes sense since
> security_inode_getsecurity() is basically overriding it. So I leave it
> on Stephen or Paul to decide which is better.

I think Ondrej's suggested approach is better.  I don't think it is a concern.