Re: [PATCH v2 2/3] Teach SELinux about anonymous inodes

From: Daniel Colascione <dancol@google.com>
To: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Tim Murray <timmurray@google.com>,
	SElinux list <selinux@vger.kernel.org>,
	LSM List <linux-security-module@vger.kernel.org>,
	Linux FS Devel <linux-fsdevel@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	kvm@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>,
	Paul Moore <paul@paul-moore.com>, Nick Kralevich <nnk@google.com>,
	Lokesh Gidra <lokeshgidra@google.com>
Subject: Re: [PATCH v2 2/3] Teach SELinux about anonymous inodes
Date: Thu, 26 Mar 2020 10:59:41 -0700
Message-ID: <CAKOZueuztKTAVDKLBPePXfCzOcWXiTEJvC=-zH71mGZPi1YawQ@mail.gmail.com> (raw)
In-Reply-To: <b5999b89-6921-5667-9eb2-662b14d5f730@tycho.nsa.gov>

Thanks for taking a look!

On Thu, Mar 26, 2020 at 6:57 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 3/25/20 7:02 PM, Daniel Colascione wrote:
> > This change uses the anon_inodes and LSM infrastructure introduced in
> > the previous patch to give SELinux the ability to control
> > anonymous-inode files that are created using the new _secure()
> > anon_inodes functions.
> >
> > A SELinux policy author detects and controls these anonymous inodes by
> > adding a name-based type_transition rule that assigns a new security
> > type to anonymous-inode files created in some domain. The name used
> > for the name-based transition is the name associated with the
> > anonymous inode for file listings --- e.g., "[userfaultfd]" or
> > "[perf_event]".
> >
> > Example:
> >
> > type uffd_t;
> > type_transition sysadm_t sysadm_t : file uffd_t "[userfaultfd]";
> > allow sysadm_t uffd_t:file { create };

Oops. Will fix.

> > (The next patch in this series is necessary for making userfaultfd
> > support this new interface.  The example above is just
> > for exposition.)
> >
> > Signed-off-by: Daniel Colascione <dancol@google.com>
> > ---
> >   security/selinux/hooks.c            | 54 +++++++++++++++++++++++++++++
> >   security/selinux/include/classmap.h |  2 ++
> >   2 files changed, 56 insertions(+)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 1659b59fb5d7..b9eb45c2e4e5 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2915,6 +2915,59 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> >       return 0;
> >   }
> >
> > +static int selinux_inode_init_security_anon(struct inode *inode,
> > +                                         const struct qstr *name,
> > +                                         const struct file_operations *fops,
> > +                                         const struct inode *context_inode)
> > +{
> > +     const struct task_security_struct *tsec = selinux_cred(current_cred());
> > +     struct common_audit_data ad;
> > +     struct inode_security_struct *isec;
> > +     int rc;
> > +
> > +     if (unlikely(!selinux_state.initialized))
> > +             return 0;
>
> This leaves secure anon inodes created before first policy load with the
> unlabeled SID rather than defaulting to the SID of the creating task
> (kernel SID in that situation).  Is that what you want?  Alternatively
> you can just remove this test and let it proceed; nothing should be
> break and the anon inodes will get the kernel SID.

We talked about this decision on the last thread [1], and I think you
mentioned that either the unlabeled or the kernel SID approach would
be defensible. Using the unlabeled SID seems more "honest" to me than
using the kernel SID: the unlabeled SID says "we don't know", while
using kernel SID would be making an affirmative claim that the
anonymous inode belongs to the kernel, and claim wouldn't be true.
That's why I'm leaning toward the unlabeled approach right now.

[1] https://lore.kernel.org/lkml/9ca03838-8686-0007-0971-ee63bf5031da@tycho.nsa.gov/