linux-security-module.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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@paul-moore.com, Nick Kralevich <nnk@google.com>,
	Lokesh Gidra <lokeshgidra@google.com>
Subject: Re: [PATCH 2/3] Teach SELinux about anonymous inodes
Date: Fri, 14 Feb 2020 09:21:04 -0800	[thread overview]
Message-ID: <CAKOZuev-=7Lgu35E3tzpHQn0m_KAvvrqi+ZJr1dpqRjHERRSqg@mail.gmail.com> (raw)
In-Reply-To: <9ca03838-8686-0007-0971-ee63bf5031da@tycho.nsa.gov>

On Fri, Feb 14, 2020 at 8:38 AM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 2/13/20 10:26 PM, Daniel Colascione wrote:
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 1659b59fb5d7..6de0892620b3 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2915,6 +2915,62 @@ 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(IS_PRIVATE(inode)))
> > +             return 0;
>
> This is not possible since the caller clears S_PRIVATE before calling
> and it would be a bug to call the hook on an inode that was intended to
> be private, so we shouldn't check it here.
>
> > +
> > +     if (unlikely(!selinux_state.initialized))
> > +             return 0;
>
> Are we doing this to defer initialization until selinux_complete_init()
> - that's normally why we bail in the !initialized case?  Not entirely
> sure what will happen in such a situation since we won't have the
> context_inode or the allocating task information at that time, so we
> certainly won't get the same result - probably they would all be labeled
> with whatever anon_inodefs is assigned via genfscon or
> SECINITSID_UNLABELED by default.
> If we instead just drop this test and
> proceed, we'll inherit the context inode SID if specified or we'll call
> security_transition_sid(), which in the !initialized case will just
> return the tsid i.e. tsec->sid, so it will be labeled with the creating
> task SID (SECINITSID_KERNEL prior to initialization).  Then the
> avc_has_perm() call will pass because everything gets allowed until
> initialization. So you could drop this check and userfaultfds created
> before policy load would get the kernel SID, or you can keep it and they
> will get the unlabeled SID.  Preference?

The kernel SID seems safer. Thanks for the explanation!

> > +
> > +     isec = selinux_inode(inode);
> > +
> > +     /*
> > +      * We only get here once per ephemeral inode.  The inode has
> > +      * been initialized via inode_alloc_security but is otherwise
> > +      * untouched.
> > +      */
> > +
> > +     if (context_inode) {
> > +             struct inode_security_struct *context_isec =
> > +                     selinux_inode(context_inode);
> > +             if (IS_ERR(context_isec))
> > +                     return PTR_ERR(context_isec);
>
> This isn't possible AFAICT so you don't need to test for it or handle
> it.  In fact, even the test for NULL in selinux_inode() is bogus and
> should get dropped AFAICT; we always allocate an inode security blob
> even before policy load so it would be a bug if we ever had a NULL there.

Thanks. Will fix.

> > +             isec->sid = context_isec->sid;
> > +     } else {
> > +             rc = security_transition_sid(
> > +                     &selinux_state, tsec->sid, tsec->sid,
> > +                     SECCLASS_FILE, name, &isec->sid);
> > +             if (rc)
> > +                     return rc;
> > +     }
>
> Since you switched to using security_transition_sid(), you are not using
> the fops parameter anymore nor comparing with userfaultfd_fops, so you
> could drop the parameter from the hook and leave the latter static in
> the first patch.

That's true, but I figured different LSMs might want different rules
that depend on the fops. I'm also okay with removing this parameter
for now, since we're not using it.

> That's assuming you are ok with having to define these type_transition
> rules for the userfaultfd case instead of having your own separate
> security class.  Wondering how many different anon inode names/classes
> there are in the kernel today and how much they change over time; for a
> small, relatively stable set, separate classes might be ok; for a large,
> dynamic set, type transitions should scale better.

I think we can get away without a class per anonymous-inode-type. I do
wonder whether we need a class for all anonymous inodes, though: if we
just give them the file class and use the anon inode type name for the
type_transition rule, isn't it possible that the type_transition rule
might also fire for plain files with the same names in the last path
component and the same originating sid? (Maybe I'm not understanding
type_transition rules properly.) Using a class to encompass all
anonymous inodes would address this problem (assuming the problem
exists in the first place).

> We might still need
> to create a mapping table in SELinux from the names to some stable
> identifier for the policy lookup if we can't rely on the names being stable.

Sure. The anonymous inode type names have historically been stable,
though, so maybe we can just use the names from anon_inodes directly
for now and then add some kind of remapping if we want to change those
names in the core, remaping to the old name for SELinux
type_transition purposes.

  reply	other threads:[~2020-02-14 17:21 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20200211225547.235083-1-dancol@google.com>
     [not found] ` <9ae20f6e-c5c0-4fd7-5b61-77218d19480b@schaufler-ca.com>
2020-02-11 23:27   ` [PATCH v2 0/6] Harden userfaultfd Daniel Colascione
2020-02-12 16:09     ` Stephen Smalley
2020-02-21 17:56     ` James Morris
2020-02-12  7:50 ` Kees Cook
2020-02-12 16:54   ` Jann Horn
2020-02-12 17:14     ` Peter Xu
2020-02-12 19:41       ` Andrea Arcangeli
2020-02-12 20:04         ` Daniel Colascione
2020-02-12 23:41           ` Andrea Arcangeli
2020-02-12 17:12   ` Daniel Colascione
2020-02-14  3:26 ` [PATCH 0/3] SELinux support for anonymous inodes and UFFD Daniel Colascione
2020-02-14  3:26   ` [PATCH 1/3] Add a new LSM-supporting anonymous inode interface Daniel Colascione
2020-02-14  3:26   ` [PATCH 2/3] Teach SELinux about anonymous inodes Daniel Colascione
2020-02-14 16:39     ` Stephen Smalley
2020-02-14 17:21       ` Daniel Colascione [this message]
2020-02-14 18:02         ` Stephen Smalley
2020-02-14 18:08           ` Stephen Smalley
2020-02-14 20:24             ` Stephen Smalley
2020-02-14  3:26   ` [PATCH 3/3] Wire UFFD up to SELinux Daniel Colascione
2020-03-25 23:02   ` [PATCH v2 0/3] SELinux support for anonymous inodes and UFFD Daniel Colascione
2020-03-25 23:02   ` [PATCH v2 1/3] Add a new LSM-supporting anonymous inode interface Daniel Colascione
2020-03-26 13:53     ` Stephen Smalley
2020-03-25 23:02   ` [PATCH v2 2/3] Teach SELinux about anonymous inodes Daniel Colascione
2020-03-26 13:58     ` Stephen Smalley
2020-03-26 17:59       ` Daniel Colascione
2020-03-26 17:37     ` Stephen Smalley
2020-03-25 23:02   ` [PATCH v2 3/3] Wire UFFD up to SELinux Daniel Colascione
2020-03-25 23:49     ` Casey Schaufler
2020-03-26 18:14   ` [PATCH v3 0/3] SELinux support for anonymous inodes and UFFD Daniel Colascione
2020-03-26 18:14     ` [PATCH v3 1/3] Add a new LSM-supporting anonymous inode interface Daniel Colascione
2020-03-26 19:00       ` Stephen Smalley
2020-03-26 18:14     ` [PATCH v3 2/3] Teach SELinux about anonymous inodes Daniel Colascione
2020-03-26 19:02       ` Stephen Smalley
2020-03-26 18:14     ` [PATCH v3 3/3] Wire UFFD up to SELinux Daniel Colascione
2020-03-26 20:06     ` [PATCH v4 0/3] SELinux support for anonymous inodes and UFFD Daniel Colascione
2020-03-26 20:06       ` [PATCH v4 1/3] Add a new LSM-supporting anonymous inode interface Daniel Colascione
2020-03-27 13:40         ` Stephen Smalley
2020-03-26 20:06       ` [PATCH v4 2/3] Teach SELinux about anonymous inodes Daniel Colascione
2020-03-27 13:41         ` Stephen Smalley
2020-03-26 20:06       ` [PATCH v4 3/3] Wire UFFD up to SELinux Daniel Colascione
2020-04-01 21:39       ` [PATCH v5 0/3] SELinux support for anonymous inodes and UFFD Daniel Colascione
2020-04-01 21:39         ` [PATCH v5 1/3] Add a new LSM-supporting anonymous inode interface Daniel Colascione
2020-05-07 16:02           ` James Morris
2020-08-04 21:22           ` Eric Biggers
2020-04-01 21:39         ` [PATCH v5 2/3] Teach SELinux about anonymous inodes Daniel Colascione
2020-04-01 21:39         ` [PATCH v5 3/3] Wire UFFD up to SELinux Daniel Colascione
2020-08-04 21:16           ` Eric Biggers
2020-04-13 13:29         ` [PATCH v5 0/3] SELinux support for anonymous inodes and UFFD Daniel Colascione
2020-04-22 16:55           ` James Morris
2020-04-22 17:12             ` Casey Schaufler
2020-04-23 22:24               ` Casey Schaufler
2020-04-27 16:18                 ` Casey Schaufler
2020-04-27 16:48                   ` Stephen Smalley
2020-04-27 17:12                     ` Casey Schaufler
2020-04-29 17:02                     ` Stephen Smalley
2020-04-27 17:15             ` Casey Schaufler
2020-04-27 19:40               ` Stephen Smalley
2020-06-04  3:56         ` James Morris
2020-06-04 18:51           ` Stephen Smalley
2020-06-04 19:24             ` Lokesh Gidra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKOZuev-=7Lgu35E3tzpHQn0m_KAvvrqi+ZJr1dpqRjHERRSqg@mail.gmail.com' \
    --to=dancol@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=lokeshgidra@google.com \
    --cc=nnk@google.com \
    --cc=paul@paul-moore.com \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@vger.kernel.org \
    --cc=timmurray@google.com \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).