Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Paul Moore <paul@paul-moore.com>]

On Thu, Jan 10, 2019 at 2:36 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 1/10/19 12:54 PM, Casey Schaufler wrote:
> > Resending after email configuration repair.
> >
> > On 1/10/2019 6:15 AM, Stephen Smalley wrote:
> >> On 1/9/19 5:03 PM, Casey Schaufler wrote:
> >>> On 1/9/2019 12:37 PM, Stephen Smalley wrote:
> >>>> On 1/9/19 12:19 PM, Casey Schaufler wrote:
> >>>>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
> >>>>>> Changes in v2:
> >>>>>> - add docstring for the new hook in union security_list_options
> >>>>>> - initialize *ctx to NULL and *ctxlen to 0 in case the hook is not
> >>>>>>      implemented
> >>>>>> v1: https://lore.kernel.org/selinux/20190109091028.24485-1-omosnace@redhat.com/T/
> >>>>>>
> >>>>>> This series adds a new security hook that allows to initialize the security
> >>>>>> context of kernfs properly, taking into account the parent context. Kernfs
> >>>>>> nodes require special handling here, since they are not bound to specific
> >>>>>> inodes/superblocks, but instead represent the backing tree structure that
> >>>>>> is used to build the VFS tree when the kernfs tree is mounted.
> >>>>>>
> >>>>>> The kernfs nodes initially do not store any security context and rely on
> >>>>>> the LSM to assign some default context to inodes created over them.
> >>>>>
> >>>>> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
> >>>>> and expected filesystem behavior?
> >>>>
> >>>> sysfs / kernfs didn't support xattrs at all when we first added support for setting security contexts to it, so originally all sysfs / kernfs inodes had a single security context, and we only required separate storage for the inodes that were explicitly labeled by userspace.
> >>>>
> >>>> Later kernfs grew support for trusted.* xattrs using simple_xattrs but the existing security.* support was left mostly unchanged.
> >>>
> >>> OK, so as I said, this seems like a bug in kernfs.
> >>>
> >>>>
> >>>>>
> >>>>>> Kernfs
> >>>>>> inodes, however, allow setting an explicit context via the *setxattr(2)
> >>>>>> syscalls, in which case the context is stored inside the kernfs node's
> >>>>>> metadata.
> >>>>>>
> >>>>>> SELinux (and possibly other LSMs) initialize the context of newly created
> >>>>>> FS objects based on the parent object's context (usually the child inherits
> >>>>>> the parent's context, unless the policy dictates otherwise).
> >>>>>
> >>>>> An LSM might use information about the parent other than the "context".
> >>>>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
> >>>>> to determine whether the Smack label of the new object should be taken
> >>>>> from the parent or the process. Passing the "context" of the parent is
> >>>>> insufficient for Smack.
> >>>>
> >>>> IIUC, this would involve switching the handling of security.* xattrs in kernfs over to use simple_xattrs too (so that we can store multiple such attributes), and then pass the entire simple_xattrs list or at least anything with a security.* prefix when initializing a new node or refreshing an existing inode.  Then the security module could extract any security.* attributes of interest for use in determining the label of new inodes and in refreshing the label of an inode.
> >>>
> >>> Right. But I'll point out that there is nothing to prevent an
> >>> LSM from using inode information outside of the xattrs (e.g. uids)
> >>> to determine the security state it wants to give a new object.
> >>
> >> If that's a real concern, the hook could pass the ia_iattr structure in addition to the simple_xattrs list and the security module could use any inode attributes it likes in making the decision.  Effectively it would be passing the entire kernfs_iattrs structure, but probably not directly since that definition is presently private to kernfs.
> >
> > Yes, it's a real concern. And no, just passing all of the kernfs internal data
> > out in j-random formats does not pass muster. Al Viro was commenting the other
> > day on how bad the LSM infrastructure interfaces are. The original proposal here
> > is already big, cluttered and inadequate. Adding more to it to make up for its
> > shortcomings should be sending up red flags
>
> I don't quite see how the original patch set or hook can be called big
> and cluttered.  Switching the handling of security xattrs in kernfs to
> use simple_xattrs (a natural and seemingly straightforward cleanup) and
> passing the entire simple_xattrs list to the hook interface would allow
> you to support SMACK64TRANSMUTE, which was the one actual inadequacy you
> identified.  You claim that someone might need/want the parent uid/gid
> too, but there are no in-tree security modules that do so nor any
> submitted AFAIK, and if that situation arises, all we need to do to
> support it is to add the iattrs.  Obviously they can all be wrapped up
> in some larger structure if desired. At that point the security modules
> would have access to all of the inode attributes supported by kernfs.

I'm with Stephen on this; if Ondrej changes it over to simple_xattrs
as described above so that Smack would have what it needs, I don't see
why we should hold off on this.

Everything we are talking about is a kernel internal issue, we can
change it as needed to take into account new LSMs or new functionality
in existing LSMs.

Ondrej, a gentle reminder that it would be nice to have a simple
selinux-testsuite test to make sure we are labeling
kernfs-based/cgroup files correctly.

-- 
paul moore
www.paul-moore.com