On 1/22/19 9:17 AM, Stephen Smalley wrote:

On 1/22/19 3:49 AM, Ondrej Mosnacek wrote:

quoted

On Mon, Jan 14, 2019 at 10:01 AM Ondrej Mosnacek [off-list ref] 
wrote:

quoted

On Thu, Jan 10, 2019 at 6:55 PM Casey Schaufler 
[off-list ref] wrote:

quoted

Resending after email configuration repair.

On 1/10/2019 6:15 AM, Stephen Smalley wrote:

quoted

On 1/9/19 5:03 PM, Casey Schaufler wrote:

quoted

On 1/9/2019 12:37 PM, Stephen Smalley wrote:

quoted

On 1/9/19 12:19 PM, Casey Schaufler wrote:

quoted

On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:

quoted

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/ (local) 


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.

quoted

quoted

quoted

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.

I actually had a patch to do just that at one point because I thought
for a while that it would be required to call
security_inode_init_security() (which I had tried to somehow force
into the kernfs node creation at some point), but then I realized it
is not actually needed (although would make thing a bit nicer) and put
it away... I will try to dig it out and reuse here.

Okay, now that I tried to do this with full xattr support I ran into a
problem. Along with converting kernfs to use simple_xattrs for
security attributes, I removed the call to
security_inode_notifysecctx() from kernfs_refresh_inode(), as it no
longer makes sense (kernfs doesn't know which attribute contains the
context; the LSM should now be able to pull it out via
vfs_getxattr()). However, SELinux now doesn't set the right security
context in the selinux_d_instantiate() hook, because the policy tells
it to use genfs, not xattr.

So... I'm not sure how to fix this. Setting fs_use_xattr for cgroupfs
in the policy won't work, because then all nodes will be unlabeled_t
by default. Maybe we could patch the genfs case in
inode_doinit_with_dentry() to try fetching the xattr first? I'm not
very confident about touching that part of the code, so I would
welcome some advice here.

This is the code I have so far, in case it helps:
https://gitlab.com/omos/linux-public/compare/selinux-next...selinux-fix-cgroupfs-v8 

I would have left security_inode_notifysecctx() or an equivalent that 
passes all of the xattrs to push the security attributes to the security 
module.

Blindly calling __vfs_getxattr() on genfs could be a problem; IIRC, 
doing so on fuse filesytems can create a deadlock during mount.  Or at 
least that was the issue with switching fuse to fs_use_xattr in the past.

See commits 4d546f81717d253ab67643bf072c6d8821a9249c, 
102aefdda4d8275ce7d7100bc16c88c74272b260, 
089be43e403a78cd6889cde2fba164fefe9dfd89, 
811f3799279e567aa354c649ce22688d949ac7a9 and
https://bugzilla.redhat.com/show_bug.cgi?id=1256635#c34 for some prior 
work and discussions in this area.