devpts xattr support

Below is a patch (against the SourceForge CVS) that will give xattr support to directories in a devpts filesystem. This will allow the context of the root node to be retrieved and modified. Note that there are other filesystems using the simple_dir_inode_operations that may need to be changed later as xattr support is spread to more filesystems (devpts is the only one causing a problem right now), so a more general fix may be wanted/needed later. Any comments are appreciated.

Darrel Goeddel
Senior Secure Systems Engineer

Trusted Computer Solutions             E: [email protected]
121 West Goose Alley                   V: 217.384.0028 x19
Urbana, IL  61801                      F: 217.384.0288

@@ -40,6 +40,16 @@
 #endif
 };
 
+struct inode_operations devpts_dir_inode_operations = {
+       .lookup         = simple_lookup,
+#ifdef CONFIG_DEVPTS_FS_XATTR
+       .setxattr       = generic_setxattr,
+       .getxattr       = generic_getxattr,
+       .listxattr      = generic_listxattr,
+       .removexattr    = generic_removexattr,
+#endif /* CONFIG_DEVPTS_FS_XATTR */
+};
+
 static struct vfsmount *devpts_mnt;
 static struct dentry *devpts_root;
 

@@ -113,7 +123,7 @@
        inode->i_blksize = 1024;
        inode->i_uid = inode->i_gid = 0;
        inode->i_mode = S_IFDIR | S_IRUGO | S_IXUGO | S_IWUSR;
-       inode->i_op = &simple_dir_inode_operations;
+       inode->i_op = &devpts_dir_inode_operations;
        inode->i_fop = &simple_dir_operations;
        inode->i_nlink = 2;
 

--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.

Thanks. Let's step back for a moment from the implementation and talk about the general issue IIUC: you want userspace to be able to get and set the file context of any file (as seen internally by the SELinux module), regardless of the underlying filesystem type, using a single API. That is what we had with the original SELinux API; stat_secure and friends always returned the incore inode SID directly. In contrast, using the xattr API and framework, we must add xattr handlers to each filesystem implementation (and for each kind of inode, as in your patch) in order to export file contexts to userspace, even when those handlers do nothing more than call into the security module to map an incore inode SID to a context (as with devpts and tmpfs). IIRC, TrustedBSD/FreeBSD approached this issue differently, with separate and orthogonal APIs for xattrs vs. MAC labels so that you could get the MAC label of a file independently of whether or not the filesystem supported extended attributes (e.g. singe-level filesystems). So now would be a good time to confirm that we want to proceed down this path of adding more fake xattr handlers wherever the filesystem does not natively support xattrs so that userspace can have a single consistent interface for getting file contexts.

Stephen Smalley <[email protected]>
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

especially as the author of squashfs is adding xattrs to his code [yes, i want to make a live-cd selinux system]

especially as i have had to do some horrible things to fuse (userspace filesystem) to get it to cooperate with selinux.

l.

--
you don't have to BE MAD   | this space    | my brother wanted to join mensa,
  to work, but   IT HELPS  |   for rent    | for an ego trip - and get kicked 
 you feel better!  I AM    | can pay cash  | out for a even bigger one.
--

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Your understanding is correct. We want to provide an interface that
will allow userspace the ability to get/set the security attributes on
any file (with the proper checks, of course), simialr to the old SELinux API. We basically need to be able to see the security attributes that are being enforced. Since there are files that receive their attributes from sources other than xattrs (such as genfscon, transition SIDS, and straight process SIDS), an interface which directly looks at the incore inode for those files would be great.

IMHO the "nicest" way to achieve this would be to abandon the use of the
xattr interface between libselinux and the kernel. The get/set SID
operations would then be achieved through a new interface (we had toyed
with the idea of a transaction node in selinuxfs). This interface would not be a duplicate of the xattr interfaces, rather it would be more of a specialized wrapper using the xattrs internally. The standard xattr system would still be used internally for permanent storage on supported filesystems. The get operation would look up the file and return the SID that was placed on the incore inode through the current means - no modifications needed in the SID association. The set operation would also do a lookup, set the SID on the incore inode, and then propagate that information to the xattr system if it is supported by that filesystem very much like the post_create function. These new interfaces would alleviate the need for enhancing all of the pseudo filesystems to include xattr support, and it would be a quick road to full attribute support for all files under SELinux.

I will try to get a chance to look at the BSD interfaces to see if there are close to what I am envisioning.

Darrel Goeddel
Senior Secure Systems Engineer

Trusted Computer Solutions             E: [email protected]
121 West Goose Alley                   V: 217.384.0028 x19
Urbana, IL  61801                      F: 217.384.0288

--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.

While I'm sympathetic to the notion (as it is more in line with the original stat_secure/chsid API), I have a hard time seeing how this will fly upstream. What about every other attribute, e.g. POSIX ACLs; will these too ultimately get their own API separate and orthogonal to the xattr API? Even the xattr API itself is controversial (vs. attributes-as-files). I also shudder a bit at the thought of this API via selinuxfs, particularly f[gs]etfilecon.

On the other hand, adding a fake xattr handler to every filesystem that doesn't natively support them also seems difficult to accept; we've only done this to date on a demand basis; devpts needed the support for pty relabeling, tmpfs needed support for udev.

Stephen Smalley <[email protected]>
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

What other filesystems are likely to need xattr handlers? It is (now especially) very simple to add them as needed.

I think we should stick with the xattr API, it is "the" way to manage extra-inode data under Linux. Perhaps what we should do instead is fix the core xattr code so that it can do what we need it do to.

- James

James Morris
<[email protected]>



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

All filesystems (and file types) will need that support. Basically, getfilecon should never be unable to retrieve a context for a valid file because there actually *is* a context associated with every file, and it is being used for access decisions. Just think if every file had the standard DAC info, but you could only see/modify that info on certain filesystems - you may scratch your head for a while trying to figure out what is causing access denials.

I'm all for that. I will play around with a few ideas and see if I can come with anything useful. I'm thinking along the lines of the permission call which uses i_op->permission if it exists and falls back to vfs_permission if it does not. Maybe we can create a fallback for *xattr...

Thanks everyone for the feedback.

Darrel Goeddel
Senior Secure Systems Engineer

Trusted Computer Solutions             E: [email protected]
121 West Goose Alley                   V: 217.384.0028 x19
Urbana, IL  61801                      F: 217.384.0288

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Yes, it seems logical that we could have a generic fallback in the VFS code for getxattr and listxattr, although I guess this would have been considered before and rejected for some reason (possibly advice from upstream?).

- James

James Morris
<[email protected]>



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Below is a simple patch that does this for getxattr. ls -Z then works for e.g. /proc /dev/pts /selinux.

Index: linux-2.6/fs/xattr.c
===================================================================
RCS file: /nfshome/pal/CVS/linux-2.6/fs/xattr.c,v
retrieving revision 1.9

diff -u -p -r1.9 xattr.c
@@ -142,15 +142,19 @@ getxattr(struct dentry *d, char __user *
 		if (error)
 			goto out;
 		error = d->d_inode->i_op->getxattr(d, kname, kvalue, size);
-		if (error > 0) {
-			if (size && copy_to_user(value, kvalue, error))
-				error = -EFAULT;
-		} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
-			/* The file system tried to returned a value bigger
-			   than XATTR_SIZE_MAX bytes. Not possible. */
-			error = -E2BIG;
-		}
+	} else if (!strncmp(kname, XATTR_SECURITY_PREFIX, sizeof XATTR_SECURITY_PREFIX - 1)) {
+		const char *suffix = kname + sizeof XATTR_SECURITY_PREFIX - 1;
+		error = security_inode_getsecurity(d->d_inode, suffix, kvalue, size);
 	}
+	if (error > 0) {
+		if (size && copy_to_user(value, kvalue, error))
+			error = -EFAULT;
+	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
+		/* The file system tried to returned a value bigger
+		   than XATTR_SIZE_MAX bytes. Not possible. */
+		error = -E2BIG;
+	} 
+
 out:
 	if (kvalue)
 		kfree(kvalue);

Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

If the filesystem has an xattr handler at all, I think that the right solution is to extend that handler to deal with security attributes, and this has already been done for the usual suspects (ext[23], reiserfs, xfs, jfs). Not sure about whether we want to hide the ENODATA case from userspace; setfiles, restorecon, and chcon all have explicit checks for that case presently (e.g. you want setfiles to assign a label in the ENODATA case even if the default incore inode label happens to match file_contexts).

We should apply a permission check, but having two hook calls at the same location is rather ugly. Possibly we could duplicate the check in the inode_getsecurity hook as well. Only case where that would be a problem is if the kernel internally calls it for some reason and doesn't want to apply a check based on the current process.

Other questions:
- Do we want the same kind of fallbacks for listxattr and setxattr? For the latter, we definitely don't want to allow setxattr of /proc inodes, but that could be prevented by policy, whereas we do want to allow setxattr of devpts and tmpfs inodes (as we presently do by their own xattr handlers). Once we have that support, we should be able to drop the xattr handlers in devpts and tmpfs entirely, right?
- How do we want to handle removexattr? We don't presently allow it ever in SELinux, but other security modules might be willing to allow removal of their security attributes.

Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

That seems reasonable.

It certainly seems that we would want to preserve that error case. I do remember some confusion here when mounting a newly created filesystem and having ls return NULL for the security context. Using the fallback after getting ENODATA gave the "seemingly desired" functionality of returning the context that is being used for enforcement. That context could be inferred from the policy - I just wonder if that will be good enough for a CC eval.

Another potential area of suestion may be that selinux_inode_getxattr() will return -EOPNOTSUPP for filesystems with the behavior of SECURITY_FS_USE_MNTPOINT. This will make it impossible to get the context being used for enforcement, although it is better than getting a completely unrelated context that be returned from the filesystem's getxattr. Sure would be nice to have the old security APIs back... Maybe we could add a new SELINUX_ENFORCEABLE xattr that would always call security_inode_getsecurity without consulting the filesystem :)

If the audit subsystem will make use of security_inode_getsecurity hook, that will be the case.

I am not sure how useful a listxattr fallback would be, but I can see no problems with it. It seems that we would also want to then extend the existing listxattr functions of the pseudo filesystems to include a consultation to the lsm as well so they can spit out security info just as if they had not implemented the listxattr function.

I have a bit more concern about the setxattr fallback. Since this operation is a bit more important, I like the extra thought that is required for adding the proper support.

If we do without a setxattr fallback, we wouldn't want a removexattr fallback. If there is a setxattr fallback, we would most likely want a removexattr fallback as well - just not with an SELinux implementation.

Darrel

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to [email protected] with
the words "unsubscribe selinux" without quotes as the message.

Having xattr support for more filesystems is useful even outside SELinux. For example there is some discussion about storing MIME types in say a user.mimetype xattr. If that were to happen, we'd want it to work even for people who make /tmp a tmpfs.

Once filesystems like tmpfs gain real xattr support, the "fake" hooks wouldn't be so anymore.