On Thu, Jan 10, 2019 at 2:36 PM Stephen Smalley <sds@xxxxxxxxxxxxx> 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@xxxxxxxxxx/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
