[PATCH] Filesystem capabilities support

[PATCH] Filesystem capabilities support

[Nicholas Hans Simmonds]

This is a simple attempt at providing capability support through extended
attributes. Setting security.cap_set to contain a struct cap_xattr_data which
defines the desired capabilities will switch on the new behaviour otherwise
there is no change. When a file is written to then the xattr (if it exists) is
removed to prevent tampering with priveleged executables. Whilst I'm not sure
this provides a secure implementation, I can't see any problem with it myself.
The patch should apply cleanly against the latest git tree and has been running
on my machine for about a week now without any noticeable problems.

Signed-off-by: Nicholas Simmonds <nhstux@gmail.com>

diff --git a/fs/read_write.c b/fs/read_write.c
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -14,6 +14,7 @@
 #include <linux/security.h>
 #include <linux/module.h>
 #include <linux/syscalls.h>
+#include <linux/xattr.h>
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
@@ -303,6 +304,16 @@ ssize_t vfs_write(struct file *file, con
 			else
 				ret = do_sync_write(file, buf, count, pos);
 			if (ret > 0) {
+#ifdef CONFIG_SECURITY_FS_CAPABILITIES
+				struct dentry *d = file->f_dentry;
+				if(d->d_inode->i_op && d->d_inode->i_op->
+								removexattr) {
+					down(&d->d_inode->i_sem);
+					d->d_inode->i_op->removexattr(d,
+								XATTR_CAP_SET);
+					up(&d->d_inode->i_sem);
+				}
+#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
 				dnotify_parent(file->f_dentry, DN_MODIFY);
 				current->wchar += ret;
 			}
diff --git a/include/linux/capability.h b/include/linux/capability.h
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -39,7 +39,19 @@ typedef struct __user_cap_data_struct {
         __u32 permitted;
         __u32 inheritable;
 } __user *cap_user_data_t;
-  
+
+struct cap_xattr_data {
+	__u32 version;
+	__u32 mask_effective;
+	__u32 effective;
+	__u32 mask_permitted;
+	__u32 permitted;
+	__u32 mask_inheritable;
+	__u32 inheritable;
+};
+
+#define XATTR_CAP_SET XATTR_SECURITY_PREFIX "cap_set"
+
 #ifdef __KERNEL__
 
 #include <linux/spinlock.h>
diff --git a/security/Kconfig b/security/Kconfig
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -60,6 +60,13 @@ config SECURITY_CAPABILITIES
 	  This enables the "default" Linux capabilities functionality.
 	  If you are unsure how to answer this question, answer Y.
 
+config SECURITY_FS_CAPABILITIES
+	bool "Filesystem Capabilities (EXPERIMENTAL)"
+	depends on SECURITY && EXPERIMENTAL
+	help
+	  This permits a process' capabilities to be set by an extended
+	  attribute in the security namespace (security.cap_set).
+
 config SECURITY_ROOTPLUG
 	tristate "Root Plug Support"
 	depends on USB && SECURITY
diff --git a/security/commoncap.c b/security/commoncap.c
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -111,9 +111,13 @@ void cap_capset_set (struct task_struct 
 
 int cap_bprm_set_security (struct linux_binprm *bprm)
 {
+	ssize_t (*bprm_getxattr)(struct dentry *,const char *,void *,size_t);
+	struct dentry *bprm_dentry;
+	ssize_t ret;
+	struct cap_xattr_data caps;
+	
 	/* Copied from fs/exec.c:prepare_binprm. */
 
-	/* We don't have VFS support for capabilities yet */
 	cap_clear (bprm->cap_inheritable);
 	cap_clear (bprm->cap_permitted);
 	cap_clear (bprm->cap_effective);
@@ -134,6 +138,34 @@ int cap_bprm_set_security (struct linux_
 		if (bprm->e_uid == 0)
 			cap_set_full (bprm->cap_effective);
 	}
+	
+#ifdef CONFIG_SECURITY_FS_CAPABILITIES
+	/* Locate any VFS capabilities: */
+
+	bprm_dentry = bprm->file->f_dentry;
+	if(!(bprm_dentry->d_inode->i_op &&
+				bprm_dentry->d_inode->i_op->getxattr))
+		return 0;
+	bprm_getxattr = bprm_dentry->d_inode->i_op->getxattr;
+	
+	down(&bprm_dentry->d_inode->i_sem);
+	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
+	if(ret == sizeof(caps)) {
+		if(caps.version == _LINUX_CAPABILITY_VERSION) {
+			cap_t(bprm->cap_effective) &= caps.mask_effective;
+			cap_t(bprm->cap_effective) |= caps.effective;
+			
+			cap_t(bprm->cap_permitted) &= caps.mask_permitted;
+			cap_t(bprm->cap_permitted) |= caps.permitted;
+			
+			cap_t(bprm->cap_inheritable) &= caps.mask_inheritable;
+			cap_t(bprm->cap_inheritable) |= caps.inheritable;
+		} else
+			printk(KERN_WARNING "Warning: %s capability set has "
+				"incorrect version\n",bprm->filename);
+	}
+	up(&bprm_dentry->d_inode->i_sem);
+#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
 	return 0;
 }
 

Re: [PATCH] Filesystem capabilities support

[Alexey Dobriyan]

On Sunday 03 July 2005 01:41, Nicholas Hans Simmonds wrote:
> This is a simple attempt at providing capability support through extended
> attributes. Setting security.cap_set to contain a struct cap_xattr_data which
> defines the desired capabilities will switch on the new behaviour otherwise
> there is no change. When a file is written to then the xattr (if it exists) is
> removed to prevent tampering with priveleged executables. Whilst I'm not sure
> this provides a secure implementation, I can't see any problem with it myself.

> --- a/security/commoncap.c
> +++ b/security/commoncap.c

>  int cap_bprm_set_security (struct linux_binprm *bprm)
>  {
> +	ssize_t (*bprm_getxattr)(struct dentry *,const char *,void *,size_t);
> +	struct dentry *bprm_dentry;
> +	ssize_t ret;
> +	struct cap_xattr_data caps;
> +	

$ make security/commoncap.o
  CC      security/commoncap.o
security/commoncap.c: In function `cap_bprm_set_security':
security/commoncap.c:114: warning: unused variable `bprm_getxattr'
security/commoncap.c:115: warning: unused variable `bprm_dentry'
security/commoncap.c:116: warning: unused variable `ret'
security/commoncap.c:117: warning: unused variable `caps'

with an obvious change in .config

> +#ifdef CONFIG_SECURITY_FS_CAPABILITIES
> +	/* Locate any VFS capabilities: */
> +
> +	bprm_dentry = bprm->file->f_dentry;
> +	if(!(bprm_dentry->d_inode->i_op &&
> +				bprm_dentry->d_inode->i_op->getxattr))
> +		return 0;
> +	bprm_getxattr = bprm_dentry->d_inode->i_op->getxattr;
> +	
> +	down(&bprm_dentry->d_inode->i_sem);
> +	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
> +	if(ret == sizeof(caps)) {
> +		if(caps.version == _LINUX_CAPABILITY_VERSION) {
> +			cap_t(bprm->cap_effective) &= caps.mask_effective;
> +			cap_t(bprm->cap_effective) |= caps.effective;
> +			
> +			cap_t(bprm->cap_permitted) &= caps.mask_permitted;
> +			cap_t(bprm->cap_permitted) |= caps.permitted;
> +			
> +			cap_t(bprm->cap_inheritable) &= caps.mask_inheritable;
> +			cap_t(bprm->cap_inheritable) |= caps.inheritable;
> +		} else
> +			printk(KERN_WARNING "Warning: %s capability set has "
> +				"incorrect version\n",bprm->filename);

You may want to print this incorrect version.

> +	}
> +	up(&bprm_dentry->d_inode->i_sem);
> +#endif /* CONFIG_SECURITY_FS_CAPABILITIES */

Re: [PATCH] Filesystem capabilities support

[Nicholas Hans Simmonds]

On Sunday 03 July 2005 00:01, Alexey Dobriyan wrote:
> On Sunday 03 July 2005 01:41, Nicholas Hans Simmonds wrote:
> > This is a simple attempt at providing capability support through
> > extended
> > attributes. Setting security.cap_set to contain a struct
> > cap_xattr_data which
> > defines the desired capabilities will switch on the new behaviour
> > otherwise
> > there is no change. When a file is written to then the xattr (if it
> > exists) is
> > removed to prevent tampering with priveleged executables. Whilst I'm
> > not sure
> > this provides a secure implementation, I can't see any problem with it
> > myself.
> > 
> > <code snipped/>
> 
> $ make security/commoncap.o
> CC      security/commoncap.o
> security/commoncap.c: In function `cap_bprm_set_security':
> security/commoncap.c:114: warning: unused variable `bprm_getxattr'
> security/commoncap.c:115: warning: unused variable `bprm_dentry'
> security/commoncap.c:116: warning: unused variable `ret'
> security/commoncap.c:117: warning: unused variable `caps'
> 
> with an obvious change in .config

Sorry, a missing #ifdef/#endif pair there.

> > <code snipped/>
> > +			printk(KERN_WARNING "Warning: %s capability set has "
> > +				"incorrect version\n.",bprm->filename);
> 
> You may want to print this incorrect version.

Good point. Both these problems are fixed in the following patch. Thank's for
testing.

diff --git a/fs/read_write.c b/fs/read_write.c
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -14,6 +14,7 @@
 #include <linux/security.h>
 #include <linux/module.h>
 #include <linux/syscalls.h>
+#include <linux/xattr.h>
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
@@ -303,6 +304,16 @@ ssize_t vfs_write(struct file *file, con
 			else
 				ret = do_sync_write(file, buf, count, pos);
 			if (ret > 0) {
+#ifdef CONFIG_SECURITY_FS_CAPABILITIES
+				struct dentry *d = file->f_dentry;
+				if(d->d_inode->i_op && d->d_inode->i_op->
+								removexattr) {
+					down(&d->d_inode->i_sem);
+					d->d_inode->i_op->removexattr(d,
+								XATTR_CAP_SET);
+					up(&d->d_inode->i_sem);
+				}
+#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
 				dnotify_parent(file->f_dentry, DN_MODIFY);
 				current->wchar += ret;
 			}
diff --git a/include/linux/capability.h b/include/linux/capability.h
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -39,7 +39,19 @@ typedef struct __user_cap_data_struct {
         __u32 permitted;
         __u32 inheritable;
 } __user *cap_user_data_t;
-  
+
+struct cap_xattr_data {
+	__u32 version;
+	__u32 mask_effective;
+	__u32 effective;
+	__u32 mask_permitted;
+	__u32 permitted;
+	__u32 mask_inheritable;
+	__u32 inheritable;
+};
+
+#define XATTR_CAP_SET XATTR_SECURITY_PREFIX "cap_set"
+
 #ifdef __KERNEL__
 
 #include <linux/spinlock.h>
diff --git a/security/Kconfig b/security/Kconfig
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -60,6 +60,13 @@ config SECURITY_CAPABILITIES
 	  This enables the "default" Linux capabilities functionality.
 	  If you are unsure how to answer this question, answer Y.
 
+config SECURITY_FS_CAPABILITIES
+	bool "Filesystem Capabilities (EXPERIMENTAL)"
+	depends on SECURITY && EXPERIMENTAL
+	help
+	  This permits a process' capabilities to be set by an extended
+	  attribute in the security namespace (security.cap_set).
+
 config SECURITY_ROOTPLUG
 	tristate "Root Plug Support"
 	depends on USB && SECURITY
diff --git a/security/commoncap.c b/security/commoncap.c
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -111,9 +111,15 @@ void cap_capset_set (struct task_struct 
 
 int cap_bprm_set_security (struct linux_binprm *bprm)
 {
+#ifdef CONFIG_SECURITY_FS_CAPABILITIES
+	ssize_t (*bprm_getxattr)(struct dentry *,const char *,void *,size_t);
+	struct dentry *bprm_dentry;
+	ssize_t ret;
+	struct cap_xattr_data caps;
+#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
+	
 	/* Copied from fs/exec.c:prepare_binprm. */
 
-	/* We don't have VFS support for capabilities yet */
 	cap_clear (bprm->cap_inheritable);
 	cap_clear (bprm->cap_permitted);
 	cap_clear (bprm->cap_effective);
@@ -134,6 +140,37 @@ int cap_bprm_set_security (struct linux_
 		if (bprm->e_uid == 0)
 			cap_set_full (bprm->cap_effective);
 	}
+	
+#ifdef CONFIG_SECURITY_FS_CAPABILITIES
+	/* Locate any VFS capabilities: */
+
+	bprm_dentry = bprm->file->f_dentry;
+	if(!(bprm_dentry->d_inode->i_op &&
+				bprm_dentry->d_inode->i_op->getxattr))
+		return 0;
+	bprm_getxattr = bprm_dentry->d_inode->i_op->getxattr;
+	
+	down(&bprm_dentry->d_inode->i_sem);
+	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
+	if(ret == sizeof(caps)) {
+		if(caps.version == _LINUX_CAPABILITY_VERSION) {
+			cap_t(bprm->cap_effective) &= caps.mask_effective;
+			cap_t(bprm->cap_effective) |= caps.effective;
+			
+			cap_t(bprm->cap_permitted) &= caps.mask_permitted;
+			cap_t(bprm->cap_permitted) |= caps.permitted;
+			
+			cap_t(bprm->cap_inheritable) &= caps.mask_inheritable;
+			cap_t(bprm->cap_inheritable) |= caps.inheritable;
+		} else
+			printk(KERN_WARNING "Warning: %s capability set has "
+				"incorrect version %08X. Correct version "
+				"is %08X\n",bprm->filename,caps.version,
+				_LINUX_CAPABILITY_VERSION);
+	}
+	up(&bprm_dentry->d_inode->i_sem);
+#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
+
 	return 0;
 }
 

Re: [PATCH] Filesystem capabilities support

[Nicholas Hans Simmonds]

On Wed, Jul 06, 2005 at 02:56:52PM +1000, Nathan Scott wrote:
> Hi Nicholas,
> 
> On Sat, Jul 02, 2005 at 10:41:08PM +0100, Nicholas Hans Simmonds wrote:
> > This is a simple attempt at providing capability support through extended
> > attributes.
> > ...
> > +#define XATTR_CAP_SET XATTR_SECURITY_PREFIX "cap_set"
> > ...
> > +	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
> > +	if(ret == sizeof(caps)) {
> > +		if(caps.version == _LINUX_CAPABILITY_VERSION) {
> > +			cap_t(bprm->cap_effective) &= caps.mask_effective;
> > ...
> 
> Since this is being stored on-disk, you may want to consider
> endianness issues.  I guess for binaries this isn't really a
> problem (since they're unlikely to be run on other platforms),
> though perhaps it is for shell scripts and the like.  Storing
> values in native endianness poses problems for backup/restore
> programs, NFS, etc.
> 
> IIRC, the other LSM security attribute values are stored as
> ASCII strings on-disk to avoid this sort of issue.
> 
> cheers.
> 
> -- 
> Nathan

Indeed. I've been thinking about this for some time, trying to determine
whether it's worth worrying about (if it's byte swapped then the version
code won't match) however you rightly point out the issue off shell
scripts. To that end I've cooked up a tiny patch which applies on top of
the previous one and will detect a byte swapped version code. My
preference for this over just using (for example) a big endian structure
is that it keeps the userspace side of things simple (no need to worry
about endianness there) however as I say I'm still not sure whether this
is the right way to deal with the problem.

diff --git a/security/commoncap.c b/security/commoncap.c
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -153,6 +153,15 @@ int cap_bprm_set_security (struct linux_
 	down(&bprm_dentry->d_inode->i_sem);
 	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
 	if(ret == sizeof(caps)) {
+		if(caps.version = swab32(_LINUX_CAPABILITY_VERSION)) {
+			swab32s(&caps.version);
+			swab32s(&caps.effective);
+			swab32s(&caps.mask_effective);
+			swab32s(&caps.permitted);
+			swab32s(&caps.mask_permitted);
+			swab32s(&caps.inheritable);
+			swab32s(&caps.mask_inheritable);
+		}
 		if(caps.version == _LINUX_CAPABILITY_VERSION) {
 			cap_t(bprm->cap_effective) &= caps.mask_effective;
 			cap_t(bprm->cap_effective) |= caps.effective;

Re: [PATCH] Filesystem capabilities support

[Nathan Scott]

Hi Nicholas,

On Sat, Jul 02, 2005 at 10:41:08PM +0100, Nicholas Hans Simmonds wrote:
> This is a simple attempt at providing capability support through extended
> attributes.
> ...
> +#define XATTR_CAP_SET XATTR_SECURITY_PREFIX "cap_set"
> ...
> +	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
> +	if(ret == sizeof(caps)) {
> +		if(caps.version == _LINUX_CAPABILITY_VERSION) {
> +			cap_t(bprm->cap_effective) &= caps.mask_effective;
> ...

Since this is being stored on-disk, you may want to consider
endianness issues.  I guess for binaries this isn't really a
problem (since they're unlikely to be run on other platforms),
though perhaps it is for shell scripts and the like.  Storing
values in native endianness poses problems for backup/restore
programs, NFS, etc.

IIRC, the other LSM security attribute values are stored as
ASCII strings on-disk to avoid this sort of issue.

cheers.

-- 
Nathan

Re: [PATCH] Filesystem capabilities support

[Nicholas Hans Simmonds]

On Wed, Jul 06, 2005 at 02:56:52PM +1000, Nathan Scott wrote:
> Hi Nicholas,
> 
> On Sat, Jul 02, 2005 at 10:41:08PM +0100, Nicholas Hans Simmonds wrote:
> > This is a simple attempt at providing capability support through extended
> > attributes.
> > ...
> > +#define XATTR_CAP_SET XATTR_SECURITY_PREFIX "cap_set"
> > ...
> > +	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
> > +	if(ret == sizeof(caps)) {
> > +		if(caps.version == _LINUX_CAPABILITY_VERSION) {
> > +			cap_t(bprm->cap_effective) &= caps.mask_effective;
> > ...
> 
> Since this is being stored on-disk, you may want to consider
> endianness issues.  I guess for binaries this isn't really a
> problem (since they're unlikely to be run on other platforms),
> though perhaps it is for shell scripts and the like.  Storing
> values in native endianness poses problems for backup/restore
> programs, NFS, etc.
> 
> IIRC, the other LSM security attribute values are stored as
> ASCII strings on-disk to avoid this sort of issue.
> 
> cheers.
> 

Sorry, my earlier reply seems to have gotten lost somewhere. I've been
pondering this issue for some time and am still not sure what's the best
answer. I've attached a small patch which handles this by detecting byte
swapping of the version code. I'm not convinced it's necessary but
shouldn't hurt.

diff --git a/security/commoncap.c b/security/commoncap.c
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -153,6 +153,15 @@ int cap_bprm_set_security (struct linux_
 	down(&bprm_dentry->d_inode->i_sem);
 	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
 	if(ret == sizeof(caps)) {
+		if(caps.version = swab32(_LINUX_CAPABILITY_VERSION)) {
+			swab32s(&caps.version);
+			swab32s(&caps.effective);
+			swab32s(&caps.mask_effective);
+			swab32s(&caps.permitted);
+			swab32s(&caps.mask_permitted);
+			swab32s(&caps.inheritable);
+			swab32s(&caps.mask_inheritable);
+		}
 		if(caps.version == _LINUX_CAPABILITY_VERSION) {
 			cap_t(bprm->cap_effective) &= caps.mask_effective;
 			cap_t(bprm->cap_effective) |= caps.effective;

Re: [PATCH] Filesystem capabilities support

[Horst von Brand]

Nicholas Hans Simmonds <nhstux@gmail.com> wrote:
> Sorry, my earlier reply seems to have gotten lost somewhere. I've been
> pondering this issue for some time and am still not sure what's the best
> answer. I've attached a small patch which handles this by detecting byte
> swapping of the version code. I'm not convinced it's necessary but
> shouldn't hurt.
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -153,6 +153,15 @@ int cap_bprm_set_security (struct linux_
>  	down(&bprm_dentry->d_inode->i_sem);
>  	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
>  	if(ret == sizeof(caps)) {
> +		if(caps.version = swab32(_LINUX_CAPABILITY_VERSION)) {
                                ^
				|
				+-- Surely wrong?!

> +			swab32s(&caps.version);
> +			swab32s(&caps.effective);
> +			swab32s(&caps.mask_effective);
> +			swab32s(&caps.permitted);
> +			swab32s(&caps.mask_permitted);
> +			swab32s(&caps.inheritable);
> +			swab32s(&caps.mask_inheritable);
> +		}
>  		if(caps.version == _LINUX_CAPABILITY_VERSION) {
>  			cap_t(bprm->cap_effective) &= caps.mask_effective;
>  			cap_t(bprm->cap_effective) |= caps.effective;
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH] Filesystem capabilities support

[Nicholas Hans Simmonds]

On Wed, Jul 13, 2005 at 01:51:46PM -0400, Horst von Brand wrote:
> Nicholas Hans Simmonds <nhstux@gmail.com> wrote:
> > Sorry, my earlier reply seems to have gotten lost somewhere. I've been
> > pondering this issue for some time and am still not sure what's the best
> > answer. I've attached a small patch which handles this by detecting byte
> > swapping of the version code. I'm not convinced it's necessary but
> > shouldn't hurt.
> > 
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -153,6 +153,15 @@ int cap_bprm_set_security (struct linux_
> >  	down(&bprm_dentry->d_inode->i_sem);
> >  	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
> >  	if(ret == sizeof(caps)) {
> > +		if(caps.version = swab32(_LINUX_CAPABILITY_VERSION)) {
>                               ^
> 				|
> 				+-- Surely wrong?!
> 

True, just noticed that. Amazing how even the simplest patches provide
such ample opportunity to shoot oneself in the foot.

> > +			swab32s(&caps.version);
> > +			swab32s(&caps.effective);
> > +			swab32s(&caps.mask_effective);
> > +			swab32s(&caps.permitted);
> > +			swab32s(&caps.mask_permitted);
> > +			swab32s(&caps.inheritable);
> > +			swab32s(&caps.mask_inheritable);
> > +		}
> >  		if(caps.version == _LINUX_CAPABILITY_VERSION) {
> >  			cap_t(bprm->cap_effective) &= caps.mask_effective;
> >  			cap_t(bprm->cap_effective) |= caps.effective;
> -- 
> Dr. Horst H. von Brand                   User #22616 counter.li.org
> Departamento de Informatica                     Fono: +56 32 654431
> Universidad Tecnica Federico Santa Maria              +56 32 654239
> Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Other than this, what are the general thoughts about this method as
opposed to just using a well defined byte order?

Thanks,

Nicholas

Re: [PATCH] Filesystem capabilities support

[Horst von Brand]

Nicholas Hans Simmonds <nhstux@gmail.com> wrote:

[...]

> Other than this, what are the general thoughts about this method as
> opposed to just using a well defined byte order?

I'd prefer a defined byte order. That way it won't bite too hard if I
happen to move a filesystem (image) from PC to SPARC or whatever.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH] Filesystem capabilities support

[Jesper Juhl]

On 7/16/05, Nicholas Hans Simmonds <nhstux@gmail.com> wrote:

While I'm not qualified to comment on the implementation I do have a
few small codingstyle comments :-)


> diff --git a/fs/read_write.c b/fs/read_write.c
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -14,6 +14,7 @@
>  #include <linux/security.h>
>  #include <linux/module.h>
>  #include <linux/syscalls.h>
> +#include <linux/xattr.h>
> 
>  #include <asm/uaccess.h>
>  #include <asm/unistd.h>
> @@ -303,6 +304,16 @@ ssize_t vfs_write(struct file *file, con
>                         else
>                                 ret = do_sync_write(file, buf, count, pos);
>                         if (ret > 0) {
> +#ifdef CONFIG_SECURITY_FS_CAPABILITIES
> +                               struct dentry *d = file->f_dentry;
> +                               if(d->d_inode->i_op && d->d_inode->i_op->

                                      if (d->d_inode->i_op ...

> +                                                               removexattr) {
> +                                       down(&d->d_inode->i_sem);
> +                                       d->d_inode->i_op->removexattr(d,
> +                                                               XATTR_CAP_SET);
> +                                       up(&d->d_inode->i_sem);
> +                               }
> +#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
>                                 fsnotify_modify(file->f_dentry);
>                                 current->wchar += ret;
>                         }
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -39,7 +39,19 @@ typedef struct __user_cap_data_struct {
>          __u32 permitted;
>          __u32 inheritable;
>  } __user *cap_user_data_t;
> -
> +
> +struct cap_xattr_data {
> +       __u32 version;
> +       __u32 mask_effective;
> +       __u32 effective;
> +       __u32 mask_permitted;
> +       __u32 permitted;
> +       __u32 mask_inheritable;
> +       __u32 inheritable;
> +};
> +
> +#define XATTR_CAP_SET XATTR_SECURITY_PREFIX "cap_set"
> +
>  #ifdef __KERNEL__
> 
>  #include <linux/spinlock.h>
> diff --git a/security/Kconfig b/security/Kconfig
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -60,6 +60,13 @@ config SECURITY_CAPABILITIES
>           This enables the "default" Linux capabilities functionality.
>           If you are unsure how to answer this question, answer Y.
> 
> +config SECURITY_FS_CAPABILITIES
> +       bool "Filesystem Capabilities (EXPERIMENTAL)"
> +       depends on SECURITY && EXPERIMENTAL
> +       help
> +         This permits a process' capabilities to be set by an extended
> +         attribute in the security namespace (security.cap_set).
> +
>  config SECURITY_ROOTPLUG
>         tristate "Root Plug Support"
>         depends on USB && SECURITY
> diff --git a/security/commoncap.c b/security/commoncap.c
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -111,9 +111,15 @@ void cap_capset_set (struct task_struct
> 
>  int cap_bprm_set_security (struct linux_binprm *bprm)
>  {
> +#ifdef CONFIG_SECURITY_FS_CAPABILITIES
> +       ssize_t (*bprm_getxattr)(struct dentry *,const char *,void *,size_t);
> +       struct dentry *bprm_dentry;
> +       ssize_t ret;
> +       struct cap_xattr_data caps;
> +#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
> +
>         /* Copied from fs/exec.c:prepare_binprm. */
> 
> -       /* We don't have VFS support for capabilities yet */
>         cap_clear (bprm->cap_inheritable);
>         cap_clear (bprm->cap_permitted);
>         cap_clear (bprm->cap_effective);
> @@ -134,6 +140,44 @@ int cap_bprm_set_security (struct linux_
>                 if (bprm->e_uid == 0)
>                         cap_set_full (bprm->cap_effective);
>         }
> +
> +#ifdef CONFIG_SECURITY_FS_CAPABILITIES
> +       /* Locate any VFS capabilities: */
> +
> +       bprm_dentry = bprm->file->f_dentry;
> +       if(!(bprm_dentry->d_inode->i_op &&

              if (!(bprm_dentry->d_inode->i_op ...

> +                               bprm_dentry->d_inode->i_op->getxattr))
> +               return 0;
> +       bprm_getxattr = bprm_dentry->d_inode->i_op->getxattr;
> +
> +       down(&bprm_dentry->d_inode->i_sem);
> +       ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
> +       up(&bprm_dentry->d_inode->i_sem);
> +       if(ret == sizeof(caps)) {

              if (ret == sizeof(caps)) {

> +               be32_to_cpus(&caps.version);
> +               be32_to_cpus(&caps.effective);
> +               be32_to_cpus(&caps.mask_effective);
> +               be32_to_cpus(&caps.permitted);
> +               be32_to_cpus(&caps.mask_permitted);
> +               be32_to_cpus(&caps.inheritable);
> +               be32_to_cpus(&caps.mask_inheritable);
> +               if(caps.version == _LINUX_CAPABILITY_VERSION) {

                      if (caps.version ...

> +                       cap_t(bprm->cap_effective) &= caps.mask_effective;
> +                       cap_t(bprm->cap_effective) |= caps.effective;
> +
> +                       cap_t(bprm->cap_permitted) &= caps.mask_permitted;
> +                       cap_t(bprm->cap_permitted) |= caps.permitted;
> +
> +                       cap_t(bprm->cap_inheritable) &= caps.mask_inheritable;
> +                       cap_t(bprm->cap_inheritable) |= caps.inheritable;
> +               } else
> +                       printk(KERN_WARNING "Warning: %s capability set has "
> +                               "incorrect version %08X. Correct version "
> +                               "is %08X.\n",bprm->filename,caps.version,
> +                               _LINUX_CAPABILITY_VERSION);
> +       }
> +#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
> +
>         return 0;
>  }
> 

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Filesystem capabilities support

[Nicholas Hans Simmonds]

On Thu, Jul 14, 2005 at 04:05:17PM -0400, Horst von Brand wrote:
> Nicholas Hans Simmonds <nhstux@gmail.com> wrote:
> 
> [...]
> 
> > Other than this, what are the general thoughts about this method as
> > opposed to just using a well defined byte order?
> 
> I'd prefer a defined byte order. That way it won't bite too hard if I
> happen to move a filesystem (image) from PC to SPARC or whatever.
> -- 
> Dr. Horst H. von Brand                   User #22616 counter.li.org
> Departamento de Informatica                     Fono: +56 32 654431
> Universidad Tecnica Federico Santa Maria              +56 32 654239
> Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Fair enough. With inotify now in Linus' tree, my patch will conflict
so I've fixed this in the following new patch which also stores the
xattr data in big-endian format. I've tested it this time and it
seems to work. Also if anyone can think of a neater method of byte-
swapping the structure (some sort of string operation?) I'd be glad
to hear it as the current code looks a bit ugly.

Thanks as ever,

Nicholas

diff --git a/fs/read_write.c b/fs/read_write.c
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -14,6 +14,7 @@
 #include <linux/security.h>
 #include <linux/module.h>
 #include <linux/syscalls.h>
+#include <linux/xattr.h>
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
@@ -303,6 +304,16 @@ ssize_t vfs_write(struct file *file, con
 			else
 				ret = do_sync_write(file, buf, count, pos);
 			if (ret > 0) {
+#ifdef CONFIG_SECURITY_FS_CAPABILITIES
+				struct dentry *d = file->f_dentry;
+				if(d->d_inode->i_op && d->d_inode->i_op->
+								removexattr) {
+					down(&d->d_inode->i_sem);
+					d->d_inode->i_op->removexattr(d,
+								XATTR_CAP_SET);
+					up(&d->d_inode->i_sem);
+				}
+#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
 				fsnotify_modify(file->f_dentry);
 				current->wchar += ret;
 			}
diff --git a/include/linux/capability.h b/include/linux/capability.h
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -39,7 +39,19 @@ typedef struct __user_cap_data_struct {
         __u32 permitted;
         __u32 inheritable;
 } __user *cap_user_data_t;
-  
+
+struct cap_xattr_data {
+	__u32 version;
+	__u32 mask_effective;
+	__u32 effective;
+	__u32 mask_permitted;
+	__u32 permitted;
+	__u32 mask_inheritable;
+	__u32 inheritable;
+};
+
+#define XATTR_CAP_SET XATTR_SECURITY_PREFIX "cap_set"
+
 #ifdef __KERNEL__
 
 #include <linux/spinlock.h>
diff --git a/security/Kconfig b/security/Kconfig
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -60,6 +60,13 @@ config SECURITY_CAPABILITIES
 	  This enables the "default" Linux capabilities functionality.
 	  If you are unsure how to answer this question, answer Y.
 
+config SECURITY_FS_CAPABILITIES
+	bool "Filesystem Capabilities (EXPERIMENTAL)"
+	depends on SECURITY && EXPERIMENTAL
+	help
+	  This permits a process' capabilities to be set by an extended
+	  attribute in the security namespace (security.cap_set).
+
 config SECURITY_ROOTPLUG
 	tristate "Root Plug Support"
 	depends on USB && SECURITY
diff --git a/security/commoncap.c b/security/commoncap.c
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -111,9 +111,15 @@ void cap_capset_set (struct task_struct 
 
 int cap_bprm_set_security (struct linux_binprm *bprm)
 {
+#ifdef CONFIG_SECURITY_FS_CAPABILITIES
+	ssize_t (*bprm_getxattr)(struct dentry *,const char *,void *,size_t);
+	struct dentry *bprm_dentry;
+	ssize_t ret;
+	struct cap_xattr_data caps;
+#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
+
 	/* Copied from fs/exec.c:prepare_binprm. */
 
-	/* We don't have VFS support for capabilities yet */
 	cap_clear (bprm->cap_inheritable);
 	cap_clear (bprm->cap_permitted);
 	cap_clear (bprm->cap_effective);
@@ -134,6 +140,44 @@ int cap_bprm_set_security (struct linux_
 		if (bprm->e_uid == 0)
 			cap_set_full (bprm->cap_effective);
 	}
+
+#ifdef CONFIG_SECURITY_FS_CAPABILITIES
+	/* Locate any VFS capabilities: */
+
+	bprm_dentry = bprm->file->f_dentry;
+	if(!(bprm_dentry->d_inode->i_op &&
+				bprm_dentry->d_inode->i_op->getxattr))
+		return 0;
+	bprm_getxattr = bprm_dentry->d_inode->i_op->getxattr;
+
+	down(&bprm_dentry->d_inode->i_sem);
+	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
+	up(&bprm_dentry->d_inode->i_sem);
+	if(ret == sizeof(caps)) {
+		be32_to_cpus(&caps.version);
+		be32_to_cpus(&caps.effective);
+		be32_to_cpus(&caps.mask_effective);
+		be32_to_cpus(&caps.permitted);
+		be32_to_cpus(&caps.mask_permitted);
+		be32_to_cpus(&caps.inheritable);
+		be32_to_cpus(&caps.mask_inheritable);
+		if(caps.version == _LINUX_CAPABILITY_VERSION) {
+			cap_t(bprm->cap_effective) &= caps.mask_effective;
+			cap_t(bprm->cap_effective) |= caps.effective;
+
+			cap_t(bprm->cap_permitted) &= caps.mask_permitted;
+			cap_t(bprm->cap_permitted) |= caps.permitted;
+
+			cap_t(bprm->cap_inheritable) &= caps.mask_inheritable;
+			cap_t(bprm->cap_inheritable) |= caps.inheritable;
+		} else
+			printk(KERN_WARNING "Warning: %s capability set has "
+				"incorrect version %08X. Correct version "
+				"is %08X.\n",bprm->filename,caps.version,
+				_LINUX_CAPABILITY_VERSION);
+	}
+#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
+
 	return 0;
 }
 

Re: [PATCH] Filesystem capabilities support

[Nicholas Hans Simmonds]

On Fri, Jul 15, 2005 at 05:45:58AM +0200, Jesper Juhl wrote:
> On 7/16/05, Nicholas Hans Simmonds <nhstux@gmail.com> wrote:
> 
> While I'm not qualified to comment on the implementation I do have a
> few small codingstyle comments :-)
> 
> 
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/security.h>
> >  #include <linux/module.h>
> >  #include <linux/syscalls.h>
> > +#include <linux/xattr.h>
> > 
> >  #include <asm/uaccess.h>
> >  #include <asm/unistd.h>
> > @@ -303,6 +304,16 @@ ssize_t vfs_write(struct file *file, con
> >                         else
> >                                 ret = do_sync_write(file, buf, count, pos);
> >                         if (ret > 0) {
> > +#ifdef CONFIG_SECURITY_FS_CAPABILITIES
> > +                               struct dentry *d = file->f_dentry;
> > +                               if(d->d_inode->i_op && d->d_inode->i_op->
> 
>                                       if (d->d_inode->i_op ...
> 
> > +                                                               removexattr) {
> > +                                       down(&d->d_inode->i_sem);
> > +                                       d->d_inode->i_op->removexattr(d,
> > +                                                               XATTR_CAP_SET);
> > +                                       up(&d->d_inode->i_sem);
> > +                               }
> > +#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
> >                                 fsnotify_modify(file->f_dentry);
> >                                 current->wchar += ret;
> >                         }
> > diff --git a/include/linux/capability.h b/include/linux/capability.h
> > --- a/include/linux/capability.h
> > +++ b/include/linux/capability.h
> > @@ -39,7 +39,19 @@ typedef struct __user_cap_data_struct {
> >          __u32 permitted;
> >          __u32 inheritable;
> >  } __user *cap_user_data_t;
> > -
> > +
> > +struct cap_xattr_data {
> > +       __u32 version;
> > +       __u32 mask_effective;
> > +       __u32 effective;
> > +       __u32 mask_permitted;
> > +       __u32 permitted;
> > +       __u32 mask_inheritable;
> > +       __u32 inheritable;
> > +};
> > +
> > +#define XATTR_CAP_SET XATTR_SECURITY_PREFIX "cap_set"
> > +
> >  #ifdef __KERNEL__
> > 
> >  #include <linux/spinlock.h>
> > diff --git a/security/Kconfig b/security/Kconfig
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -60,6 +60,13 @@ config SECURITY_CAPABILITIES
> >           This enables the "default" Linux capabilities functionality.
> >           If you are unsure how to answer this question, answer Y.
> > 
> > +config SECURITY_FS_CAPABILITIES
> > +       bool "Filesystem Capabilities (EXPERIMENTAL)"
> > +       depends on SECURITY && EXPERIMENTAL
> > +       help
> > +         This permits a process' capabilities to be set by an extended
> > +         attribute in the security namespace (security.cap_set).
> > +
> >  config SECURITY_ROOTPLUG
> >         tristate "Root Plug Support"
> >         depends on USB && SECURITY
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -111,9 +111,15 @@ void cap_capset_set (struct task_struct
> > 
> >  int cap_bprm_set_security (struct linux_binprm *bprm)
> >  {
> > +#ifdef CONFIG_SECURITY_FS_CAPABILITIES
> > +       ssize_t (*bprm_getxattr)(struct dentry *,const char *,void *,size_t);
> > +       struct dentry *bprm_dentry;
> > +       ssize_t ret;
> > +       struct cap_xattr_data caps;
> > +#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
> > +
> >         /* Copied from fs/exec.c:prepare_binprm. */
> > 
> > -       /* We don't have VFS support for capabilities yet */
> >         cap_clear (bprm->cap_inheritable);
> >         cap_clear (bprm->cap_permitted);
> >         cap_clear (bprm->cap_effective);
> > @@ -134,6 +140,44 @@ int cap_bprm_set_security (struct linux_
> >                 if (bprm->e_uid == 0)
> >                         cap_set_full (bprm->cap_effective);
> >         }
> > +
> > +#ifdef CONFIG_SECURITY_FS_CAPABILITIES
> > +       /* Locate any VFS capabilities: */
> > +
> > +       bprm_dentry = bprm->file->f_dentry;
> > +       if(!(bprm_dentry->d_inode->i_op &&
> 
>               if (!(bprm_dentry->d_inode->i_op ...
> 
> > +                               bprm_dentry->d_inode->i_op->getxattr))
> > +               return 0;
> > +       bprm_getxattr = bprm_dentry->d_inode->i_op->getxattr;
> > +
> > +       down(&bprm_dentry->d_inode->i_sem);
> > +       ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
> > +       up(&bprm_dentry->d_inode->i_sem);
> > +       if(ret == sizeof(caps)) {
> 
>               if (ret == sizeof(caps)) {
> 
> > +               be32_to_cpus(&caps.version);
> > +               be32_to_cpus(&caps.effective);
> > +               be32_to_cpus(&caps.mask_effective);
> > +               be32_to_cpus(&caps.permitted);
> > +               be32_to_cpus(&caps.mask_permitted);
> > +               be32_to_cpus(&caps.inheritable);
> > +               be32_to_cpus(&caps.mask_inheritable);
> > +               if(caps.version == _LINUX_CAPABILITY_VERSION) {
> 
>                       if (caps.version ...
> 
> > +                       cap_t(bprm->cap_effective) &= caps.mask_effective;
> > +                       cap_t(bprm->cap_effective) |= caps.effective;
> > +
> > +                       cap_t(bprm->cap_permitted) &= caps.mask_permitted;
> > +                       cap_t(bprm->cap_permitted) |= caps.permitted;
> > +
> > +                       cap_t(bprm->cap_inheritable) &= caps.mask_inheritable;
> > +                       cap_t(bprm->cap_inheritable) |= caps.inheritable;
> > +               } else
> > +                       printk(KERN_WARNING "Warning: %s capability set has "
> > +                               "incorrect version %08X. Correct version "
> > +                               "is %08X.\n",bprm->filename,caps.version,
> > +                               _LINUX_CAPABILITY_VERSION);
> > +       }
> > +#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
> > +
> >         return 0;
> >  }
> > 
> 
> -- 
> Jesper Juhl <jesper.juhl@gmail.com>
> Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
> Plain text mails only, please      http://www.expita.com/nomime.html

Quite frankly if these are the only problems with my style I'll be more
than happy. The following patch cleans things up.

diff --git a/fs/read_write.c b/fs/read_write.c
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -306,8 +306,9 @@ ssize_t vfs_write(struct file *file, con
 			if (ret > 0) {
 #ifdef CONFIG_SECURITY_FS_CAPABILITIES
 				struct dentry *d = file->f_dentry;
-				if(d->d_inode->i_op && d->d_inode->i_op->
-								removexattr) {
+				if (d->d_inode->i_op
+					&& d->d_inode->i_op->removexattr)
+				{
 					down(&d->d_inode->i_sem);
 					d->d_inode->i_op->removexattr(d,
 								XATTR_CAP_SET);
diff --git a/security/commoncap.c b/security/commoncap.c
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -145,15 +145,15 @@ int cap_bprm_set_security (struct linux_
 	/* Locate any VFS capabilities: */
 
 	bprm_dentry = bprm->file->f_dentry;
-	if(!(bprm_dentry->d_inode->i_op &&
-				bprm_dentry->d_inode->i_op->getxattr))
+	if (!(bprm_dentry->d_inode->i_op
+				&& bprm_dentry->d_inode->i_op->getxattr))
 		return 0;
 	bprm_getxattr = bprm_dentry->d_inode->i_op->getxattr;
 
 	down(&bprm_dentry->d_inode->i_sem);
 	ret = bprm_getxattr(bprm_dentry,XATTR_CAP_SET,&caps,sizeof(caps));
 	up(&bprm_dentry->d_inode->i_sem);
-	if(ret == sizeof(caps)) {
+	if (ret == sizeof(caps)) {
 		be32_to_cpus(&caps.version);
 		be32_to_cpus(&caps.effective);
 		be32_to_cpus(&caps.mask_effective);
@@ -161,7 +161,7 @@ int cap_bprm_set_security (struct linux_
 		be32_to_cpus(&caps.mask_permitted);
 		be32_to_cpus(&caps.inheritable);
 		be32_to_cpus(&caps.mask_inheritable);
-		if(caps.version == _LINUX_CAPABILITY_VERSION) {
+		if (caps.version == _LINUX_CAPABILITY_VERSION) {
 			cap_t(bprm->cap_effective) &= caps.mask_effective;
 			cap_t(bprm->cap_effective) |= caps.effective;
 

Re: [PATCH] Filesystem capabilities support

[Arnout Engelen]

Nicholas Hans Simmonds wrote:
> This is a simple attempt at providing capability support

Very good to see progress in this field. I'm not familiar with the 
technical details yet, but this seems an important security feature imho.

How does this patch relate to the one at
http://www.olafdietsche.de/linux/capability ?

I do think the LD_PRELOAD / LD_LIBRARY_PATH problem (also described by
Olaf) should be mentioned in the kernel config, and fs capabilities should 
remain marked EXPERIMENTAL until that's resolved.


Kind regards,

-- 
Arnout Engelen <arnouten@bzzt.net>

  "If it sounds good, it /is/ good."
          -- Duke Ellington