Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210421171446.785507-1-omosnace@redhat.com> <20210421171446.785507-3-omosnace@redhat.com>
 <CAEjxPJ5ksqrafO8uaf3jR=cjU5JnyQYmn_57skp=WXz7-RcbVQ@mail.gmail.com> <CAFqZXNv4gKFN5FV_Z8U82cOzauBggaqPE0WZZUdnNRxCQ3PVPw@mail.gmail.com>
In-Reply-To: <CAFqZXNv4gKFN5FV_Z8U82cOzauBggaqPE0WZZUdnNRxCQ3PVPw@mail.gmail.com>
From:   Stephen Smalley <stephen.smalley.work@gmail.com>
Date:   Fri, 23 Apr 2021 10:22:42 -0400
Message-ID: <CAEjxPJ5iWjcQGzfJy-5CLa+e95C+OmeQ_GAU44s+8ripuMJg9g@mail.gmail.com>
Subject: Re: [RFC PATCH 2/2] selinux: add capability to map anon inode types
 to separate classes
To:     Ondrej Mosnacek <omosnace@redhat.com>
Cc:     SElinux list <selinux@vger.kernel.org>,
        Paul Moore <paul@paul-moore.com>,
        LSM List <linux-security-module@vger.kernel.org>,
        "open list:MEMORY MANAGEMENT" <linux-mm@kvack.org>,
        Linux FS Devel <linux-fsdevel@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Lokesh Gidra <lokeshgidra@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Apr 23, 2021 at 9:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Thu, Apr 22, 2021 at 3:21 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > On Wed, Apr 21, 2021 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > Unfortunately, the approach chosen in commit 29cd6591ab6f ("selinux:
> > > teach SELinux about anonymous inodes") to use a single class for all
> > > anon inodes and let the policy distinguish between them using named
> > > transitions turned out to have a rather unfortunate drawback.
> > >
> > > For example, suppose we have two types of anon inodes, "A" and "B", and
> > > we want to allow a set of domains (represented by an attribute "attr_x")
> > > certain set of permissions on anon inodes of type "A" that were created
> > > by the same domain, but at the same time disallow this set to access
> > > anon inodes of type "B" entirely. Since all inodes share the same class
> > > and we want to distinguish both the inode types and the domains that
> > > created them, we have no choice than to create separate types for the
> > > cartesian product of (domains that belong to attr_x) x ("A", "B") and
> > > add all the necessary allow and transition rules for each domain
> > > individually.
> > >
> > > This makes it very impractical to write sane policies for anon inodes in
> > > the future, as more anon inode types are added. Therefore, this patch
> > > implements an alternative approach that assigns a separate class to each
> > > type of anon inode. This allows the example above to be implemented
> > > without any transition rules and with just a single allow rule:
> > >
> > > allow attr_x self:A { ... };
> > >
> > > In order to not break possible existing users of the already merged
> > > original approach, this patch also adds a new policy capability
> > > "extended_anon_inode_class" that needs to be set by the policy to enable
> > > the new behavior.
> > >
> > > I decided to keep the named transition mechanism in the new variant,
> > > since there might eventually be some extra information in the anon inode
> > > name that could be used in transitions.
> > >
> > > One minor annoyance is that the kernel still expects the policy to
> > > provide both classes (anon_inode and userfaultfd) regardless of the
> > > capability setting and if one of them is not defined in the policy, the
> > > kernel will print a warning when loading the policy. However, it doesn't
> > > seem worth to work around that in the kernel, as the policy can provide
> > > just the definition of the unused class(es) (and permissions) to avoid
> > > this warning. Keeping the legacy anon_inode class with some fallback
> > > rules may also be desirable to keep the policy compatible with kernels
> > > that only support anon_inode.
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> >
> > NAK.  We do not want to introduce a new security class for every user
> > of anon inodes - that isn't what security classes are for.
> > For things like kvm device inodes, those should ultimately use the
> > inherited context from the related inode (the /dev/kvm inode itself).
> > That was the original intent of supporting the related inode.
>
> Hmm, so are you implying that anon inodes should be thought of the
> same as control /dev nodes? I.e. that even though there may be many
> one-time actual inodes created by different processes, they should be
> thought of as a single "static interface" to the respective kernel
> functionality? That would justify having a common type/label for all
> of them, but I'm not sure if it doesn't open some gap due to the
> possibility to pass the associated file descriptors between processes
> (as AFAIK, these can hold some context)...

That was the original design (and the original patchset that we posted
in parallel with Google's independently developed one). We even had
example policy/controls for /dev/kvm ioctls.
Imagine trying to write policy over /dev/kvm ioctls where you have to
deal with N different classes and/or types and remember which ioctl
commands are exercised on which class or type even though from the
users' perspective they all occurred through the /dev/kvm interface.
It seemed super fragile and difficult to maintain/analyze that way.
Versus writing a single allow rule for all /dev/kvm ioctls.

I guess we could discuss the alternatives but please have a look at
those original patches and examples.  If we go down this road, we need
some way to deal with scaling because we only have a limited number of
discrete classes available to us and potentially unbounded set of
distinct anon inode users (although hopefully in practice only a few
that we care about distinguishing).

> I thought this was supposed to resemble more the way BPF, perf_event,
> etc. support was implemented - the BPF and perf_event fds are also
> anon inodes under the hood, BTW - where each file descriptor is
> considered a separate object that inherits the label of its creator
> and there is some class separation (e.g. bpf vs. perf_event).