Re: [PATCH v4 10/30] selinux: implement get, set and remove acl hook

From: Christian Brauner <brauner@kernel.org>
To: Paul Moore <paul@paul-moore.com>
Cc: linux-fsdevel@vger.kernel.org, Seth Forshee <sforshee@kernel.org>,
	Christoph Hellwig <hch@lst.de>, Al Viro <viro@zeniv.linux.org.uk>,
	linux-integrity@vger.kernel.org,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	Eric Paris <eparis@parisplace.org>,
	selinux@vger.kernel.org, linux-security-module@vger.kernel.org
Subject: Re: [PATCH v4 10/30] selinux: implement get, set and remove acl hook
Date: Fri, 30 Sep 2022 10:38:04 +0200	[thread overview]
Message-ID: <20220930083804.eiar274qhclpo5uw@wittgenstein> (raw)
In-Reply-To: <CAHC9VhSHSk9MNK+FmydGTZDzDOuwF0b1A3SqYhG+X0NSCwoUEg@mail.gmail.com>

On Thu, Sep 29, 2022 at 03:15:17PM -0400, Paul Moore wrote:
> On Thu, Sep 29, 2022 at 11:31 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > The current way of setting and getting posix acls through the generic
> > xattr interface is error prone and type unsafe. The vfs needs to
> > interpret and fixup posix acls before storing or reporting it to
> > userspace. Various hacks exist to make this work. The code is hard to
> > understand and difficult to maintain in it's current form. Instead of
> > making this work by hacking posix acls through xattr handlers we are
> > building a dedicated posix acl api around the get and set inode
> > operations. This removes a lot of hackiness and makes the codepaths
> > easier to maintain. A lot of background can be found in [1].
> >
> > So far posix acls were passed as a void blob to the security and
> > integrity modules. Some of them like evm then proceed to interpret the
> > void pointer and convert it into the kernel internal struct posix acl
> > representation to perform their integrity checking magic. This is
> > obviously pretty problematic as that requires knowledge that only the
> > vfs is guaranteed to have and has lead to various bugs. Add a proper
> > security hook for setting posix acls and pass down the posix acls in
> > their appropriate vfs format instead of hacking it through a void
> > pointer stored in the uapi format.
> >
> > I spent considerate time in the security module infrastructure and
> > audited all codepaths. SELinux has no restrictions based on the posix
> > acl values passed through it. The capability hook doesn't need to be
> > called either because it only has restrictions on security.* xattrs. So
> > these are all fairly simply hooks for SELinux.
> >
> > Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1]
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > ---
> >
> > Notes:
> >     /* v2 */
> >     unchanged
> >
> >     /* v3 */
> >     Paul Moore <paul@paul-moore.com>:
> >     - Add get, and remove acl hook
> >
> >     /* v4 */
> >     unchanged
> >
> >  security/selinux/hooks.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> 
> One small nitpick below, but looks good regardless.
> 
> Acked-by: Paul Moore <paul@paul-moore.com>
> 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 79573504783b..0e3cd67e5e92 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -3239,6 +3239,27 @@ static int selinux_inode_setxattr(struct user_namespace *mnt_userns,
> >                             &ad);
> >  }
> >
> > +static int selinux_inode_set_acl(struct user_namespace *mnt_userns,
> > +                                struct dentry *dentry, const char *acl_name,
> > +                                struct posix_acl *kacl)
> > +{
> > +       return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
> > +}
> > +
> > +static int selinux_inode_get_acl(struct user_namespace *mnt_userns,
> > +                                struct dentry *dentry, const char *acl_name)
> > +{
> > +       const struct cred *cred = current_cred();
> > +
> > +       return dentry_has_perm(cred, dentry, FILE__GETATTR);
> > +}
> 
> Both the set and remove hooks use current_cred() directly in the call
> to dentry_has_perm(), you might as well do the same in the get hook.

Done.