All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] devpts xattr support
@ 2004-10-26 14:14 Darrel Goeddel
  2004-10-26 14:42 ` Stephen Smalley
  0 siblings, 1 reply; 13+ messages in thread
From: Darrel Goeddel @ 2004-10-26 14:14 UTC (permalink / raw)
  To: selinux

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: dgoeddel@trustedcs.com
121 West Goose Alley                   V: 217.384.0028 x19
Urbana, IL  61801                      F: 217.384.0288


--- inode.c     12 Oct 2004 18:07:40 -0000      1.1.1.3
+++ inode.c     25 Oct 2004 17:41:43 -0000      1.2
@@ -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 majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2004-10-26 14:14 [PATCH] devpts xattr support Darrel Goeddel
@ 2004-10-26 14:42 ` Stephen Smalley
  2004-10-26 18:00   ` Darrel Goeddel
  2004-10-26 18:04   ` Luke Kenneth Casson Leighton
  0 siblings, 2 replies; 13+ messages in thread
From: Stephen Smalley @ 2004-10-26 14:42 UTC (permalink / raw)
  To: Darrel Goeddel; +Cc: selinux, James Morris

On Tue, 2004-10-26 at 10:14, Darrel Goeddel wrote:
> 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.

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 <sds@epoch.ncsc.mil>
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2004-10-26 14:42 ` Stephen Smalley
@ 2004-10-26 18:00   ` Darrel Goeddel
  2004-10-27 14:15     ` Stephen Smalley
  2004-10-26 18:04   ` Luke Kenneth Casson Leighton
  1 sibling, 1 reply; 13+ messages in thread
From: Darrel Goeddel @ 2004-10-26 18:00 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: selinux, James Morris, Chad Hanson

Stephen Smalley wrote:
> 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.
> 

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: dgoeddel@trustedcs.com
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 majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2004-10-26 14:42 ` Stephen Smalley
  2004-10-26 18:00   ` Darrel Goeddel
@ 2004-10-26 18:04   ` Luke Kenneth Casson Leighton
  1 sibling, 0 replies; 13+ messages in thread
From: Luke Kenneth Casson Leighton @ 2004-10-26 18:04 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: Darrel Goeddel, selinux, James Morris

On Tue, Oct 26, 2004 at 10:42:46AM -0400, Stephen Smalley wrote:

> 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.
 
 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 majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2004-10-26 18:00   ` Darrel Goeddel
@ 2004-10-27 14:15     ` Stephen Smalley
  2004-10-27 14:33       ` James Morris
  2004-10-27 17:18       ` Colin Walters
  0 siblings, 2 replies; 13+ messages in thread
From: Stephen Smalley @ 2004-10-27 14:15 UTC (permalink / raw)
  To: Darrel Goeddel; +Cc: selinux, James Morris, Chad Hanson

On Tue, 2004-10-26 at 14:00, Darrel Goeddel wrote:
> 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.

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 <sds@epoch.ncsc.mil>
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2004-10-27 14:15     ` Stephen Smalley
@ 2004-10-27 14:33       ` James Morris
  2004-10-27 18:10         ` Darrel Goeddel
  2004-10-27 17:18       ` Colin Walters
  1 sibling, 1 reply; 13+ messages in thread
From: James Morris @ 2004-10-27 14:33 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: Darrel Goeddel, selinux, Chad Hanson

On Wed, 27 Oct 2004, Stephen Smalley wrote:

> 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.

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
<jmorris@redhat.com>



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2004-10-27 14:15     ` Stephen Smalley
  2004-10-27 14:33       ` James Morris
@ 2004-10-27 17:18       ` Colin Walters
  1 sibling, 0 replies; 13+ messages in thread
From: Colin Walters @ 2004-10-27 17:18 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: Darrel Goeddel, selinux, James Morris, Chad Hanson

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On Wed, 2004-10-27 at 10:15 -0400, Stephen Smalley wrote:

> 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.

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.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2004-10-27 14:33       ` James Morris
@ 2004-10-27 18:10         ` Darrel Goeddel
  2004-10-27 18:31           ` James Morris
  0 siblings, 1 reply; 13+ messages in thread
From: Darrel Goeddel @ 2004-10-27 18:10 UTC (permalink / raw)
  To: James Morris; +Cc: Stephen Smalley, selinux, Chad Hanson

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

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 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.

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: dgoeddel@trustedcs.com
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 majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2004-10-27 18:10         ` Darrel Goeddel
@ 2004-10-27 18:31           ` James Morris
  2005-06-13 19:32             ` Stephen Smalley
  0 siblings, 1 reply; 13+ messages in thread
From: James Morris @ 2004-10-27 18:31 UTC (permalink / raw)
  To: Darrel Goeddel; +Cc: Stephen Smalley, selinux, Chad Hanson

On Wed, 27 Oct 2004, Darrel Goeddel wrote:

> 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...

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
<jmorris@redhat.com>



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2004-10-27 18:31           ` James Morris
@ 2005-06-13 19:32             ` Stephen Smalley
  2005-06-13 21:01               ` Darrel Goeddel
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Smalley @ 2005-06-13 19:32 UTC (permalink / raw)
  To: James Morris
  Cc: Darrel Goeddel, selinux, Chad Hanson,
	Lorenzo Hernández García-Hierro

On Wed, 2004-10-27 at 14:31 -0400, James Morris wrote:
> On Wed, 27 Oct 2004, Darrel Goeddel wrote:
> 
> > 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...
> 
> 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?).

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
--- linux-2.6/fs/xattr.c	27 Dec 2004 15:09:17 -0000	1.9
+++ linux-2.6/fs/xattr.c	31 May 2005 12:56:27 -0000
@@ -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 majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2005-06-13 19:32             ` Stephen Smalley
@ 2005-06-13 21:01               ` Darrel Goeddel
  2005-06-14 13:15                 ` Stephen Smalley
  0 siblings, 1 reply; 13+ messages in thread
From: Darrel Goeddel @ 2005-06-13 21:01 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: James Morris, selinux, Chad Hanson,
	Lorenzo Hernández García-Hierro

Stephen Smalley wrote:
> 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
> --- linux-2.6/fs/xattr.c	27 Dec 2004 15:09:17 -0000	1.9
> +++ linux-2.6/fs/xattr.c	31 May 2005 12:56:27 -0000
> @@ -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)) {

What about the case of an xattr handle being present that does not support security
attributes?  Or the case where an appropriate handler exists but there is no data stored
on the filesystem (I think this still happens for directories that are mounted over at
install time on Fedora Core).  Changing the above to:

	}
	if ((error == -EOPNOTSUPP || error == -ENODATA) &&
	    !strncmp(kname, XATTR_SECURITY_PREFIX, sizeof XATTR_SECURITY_PREFIX - 1)) {

would handle those cases as well (I think that gets em all).

> +		const char *suffix = kname + sizeof XATTR_SECURITY_PREFIX - 1;
> +		error = security_inode_getsecurity(d->d_inode, suffix, kvalue, size);

Shouldn't we also check security_inode_getxattr before going ahead with the
security_ionde_getxattr_checkin the "fallback" case?  

>  	}
> +	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);
> 
> 

-- 

Darrel

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2005-06-13 21:01               ` Darrel Goeddel
@ 2005-06-14 13:15                 ` Stephen Smalley
  2005-06-16 14:45                   ` Darrel Goeddel
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Smalley @ 2005-06-14 13:15 UTC (permalink / raw)
  To: Darrel Goeddel
  Cc: James Morris, selinux, Chad Hanson,
	Lorenzo Hernández García-Hierro

On Mon, 2005-06-13 at 16:01 -0500, Darrel Goeddel wrote:
> What about the case of an xattr handle being present that does not support security
> attributes?  Or the case where an appropriate handler exists but there is no data stored
> on the filesystem (I think this still happens for directories that are mounted over at
> install time on Fedora Core).  Changing the above to:
> 
> 	}
> 	if ((error == -EOPNOTSUPP || error == -ENODATA) &&
> 	    !strncmp(kname, XATTR_SECURITY_PREFIX, sizeof XATTR_SECURITY_PREFIX - 1)) {
> 
> would handle those cases as well (I think that gets em all).

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).

> > +		const char *suffix = kname + sizeof XATTR_SECURITY_PREFIX - 1;
> > +		error = security_inode_getsecurity(d->d_inode, suffix, kvalue, size);
> 
> Shouldn't we also check security_inode_getxattr before going ahead with the
> security_ionde_getxattr_checkin the "fallback" case?

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 majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] devpts xattr support
  2005-06-14 13:15                 ` Stephen Smalley
@ 2005-06-16 14:45                   ` Darrel Goeddel
  0 siblings, 0 replies; 13+ messages in thread
From: Darrel Goeddel @ 2005-06-16 14:45 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: James Morris, selinux, Chad Hanson,
	Lorenzo Hernández García-Hierro

Stephen Smalley wrote:
> On Mon, 2005-06-13 at 16:01 -0500, Darrel Goeddel wrote:
> 
>>What about the case of an xattr handle being present that does not support security
>>attributes?  Or the case where an appropriate handler exists but there is no data stored
>>on the filesystem (I think this still happens for directories that are mounted over at
>>install time on Fedora Core).  Changing the above to:
>>
>>	}
>>	if ((error == -EOPNOTSUPP || error == -ENODATA) &&
>>	    !strncmp(kname, XATTR_SECURITY_PREFIX, sizeof XATTR_SECURITY_PREFIX - 1)) {
>>
>>would handle those cases as well (I think that gets em all).
> 
> 
> 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).

That seems reasonable.

> 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).

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 :)

> 
>>>+		const char *suffix = kname + sizeof XATTR_SECURITY_PREFIX - 1;
>>>+		error = security_inode_getsecurity(d->d_inode, suffix, kvalue, size);
>>
>>Shouldn't we also check security_inode_getxattr before going ahead with the
>>security_ionde_getxattr_checkin the "fallback" case?
> 
> 
> 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.

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

> 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?

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.

> - 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.
> 

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 majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2005-06-16 14:52 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-10-26 14:14 [PATCH] devpts xattr support Darrel Goeddel
2004-10-26 14:42 ` Stephen Smalley
2004-10-26 18:00   ` Darrel Goeddel
2004-10-27 14:15     ` Stephen Smalley
2004-10-27 14:33       ` James Morris
2004-10-27 18:10         ` Darrel Goeddel
2004-10-27 18:31           ` James Morris
2005-06-13 19:32             ` Stephen Smalley
2005-06-13 21:01               ` Darrel Goeddel
2005-06-14 13:15                 ` Stephen Smalley
2005-06-16 14:45                   ` Darrel Goeddel
2004-10-27 17:18       ` Colin Walters
2004-10-26 18:04   ` Luke Kenneth Casson Leighton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.