Skip to contentSkip to conversation

Filesystem capabilities support

PATCH In review Nicholas Hans Simmonds 5 participants 12 messages Jul 2, 2005 – Jul 15, 2005

Subsystems (3)
VFS Security (LSM) Capabilities
lore
Share & tools
lore

Nicholas Hans Simmonds

author
#0 lore raw

PATCH 1/1 Filesystem capabilities support

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 <[email protected]>
fs/read_write.c +11 -0
@@ -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;
 			}
include/linux/capability.h +13 -1
@@ -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>
security/Kconfig +7 -0
@@ -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
security/commoncap.c +33 -1
@@ -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;
 }
#1 lore raw
Quoted 9 lines · 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.

security/commoncap.c void cap_capset_set (struct task_struct · line 118

Quoted 8 lines
security/commoncap.c +5 -0
 
 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

Quoted 14 lines
security/commoncap.c +14 -0
+#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;

security/commoncap.c int cap_bprm_set_security (struct linux_ · line 165

Quoted 10 lines
security/commoncap.c +10 -0
+			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.

Quoted 3 lines
security/commoncap.c +3 -0
+	}
+	up(&bprm_dentry->d_inode->i_sem);
+#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
author
#2 lore raw
Quoted 24 lines · Alexey Dobriyan
Quoted 13 lines · 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.

Quoted 1 line

<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'
Quoted 1 line

with an obvious change in .config

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

Quoted 5 lines

Review

Quoted 3 lines

<code snipped/>

(quoted diff) +2 -0
+			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.

fs/read_write.c +11 -0
@@ -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;
 			}
include/linux/capability.h +13 -1
@@ -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>
security/Kconfig +7 -0
@@ -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
security/commoncap.c +38 -1
@@ -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;
 }
#3 lore raw
In reply to Nicholas Hans Simmonds

Hi Nicholas,

security/commoncap.c int cap_bprm_set_security (struct linux_ · line 155

Quoted 10 lines · Nicholas Hans Simmonds

This is a simple attempt at providing capability support through extended attributes.
...

security/commoncap.c +5 -0
+#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.

Signature · 1 line
Nathan
author
#4 lore raw
Quoted 21 lines · Nathan Scott

Hi Nicholas,

Review

Quoted 10 lines · Nicholas Hans Simmonds

This is a simple attempt at providing capability support through extended attributes.
...

(quoted diff) +5 -0
+#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.

Quoted 7 lines

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

cheers.

Signature · 1 line
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.

security/commoncap.c +9 -0
@@ -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;
author
#5 lore raw
In reply to Nathan Scott
Quoted 21 lines · Nathan Scott

Hi Nicholas,

Review

Quoted 10 lines · Nicholas Hans Simmonds

This is a simple attempt at providing capability support through extended attributes.
...

(quoted diff) +5 -0
+#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.

Quoted 5 lines

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.

security/commoncap.c +9 -0
@@ -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;
#6 lore raw
Quoted 10 lines · Nicholas Hans Simmonds

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.

security/commoncap.c int cap_bprm_set_security (struct linux_ · line 156

Quoted 4 lines
security/commoncap.c +1 -0
@@ -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?!
Quoted 11 lines
security/commoncap.c +8 -0
+			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;
Signature · 4 lines
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
author
#7 lore raw
Quoted 7 lines · Horst von Brand
Quoted 6 lines · Nicholas Hans Simmonds

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.

Quoted 12 lines

security/commoncap.c int cap_bprm_set_security (struct linux_ · line 156

Quoted 8 lines
security/commoncap.c +1 -0
@@ -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.

Quoted 16 lines
Quoted 11 lines
security/commoncap.c +8 -0
+			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;
Signature · 4 lines
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

#8 lore raw

Nicholas Hans Simmonds <[email protected]> wrote:

[...]

Quoted 2 lines

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.

Signature · 4 lines
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
author
#9 lore raw
Quoted 14 lines · Horst von Brand

Nicholas Hans Simmonds <[email protected]> wrote:

[...]

Quoted 2 lines

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.

Signature · 4 lines
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

fs/read_write.c +11 -0
@@ -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;
 			}
include/linux/capability.h +13 -1
@@ -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>
security/Kconfig +7 -0
@@ -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
security/commoncap.c +45 -1

60 lines not on this page · raw message

#10 lore raw

On 7/16/05, Nicholas Hans Simmonds <[email protected]> wrote:

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

Quoted 12 lines
fs/read_write.c +1 -0
@@ -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>

fs/read_write.c ssize_t vfs_write(struct file *file, con · line 309

Quoted 6 lines
fs/read_write.c +3 -0
@@ -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 ...
Quoted 72 lines
fs/read_write.c +7 -0
+                                                               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;
                        }
include/linux/capability.h +13 -1
@@ -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>
security/Kconfig +7 -0
@@ -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
security/commoncap.c +7 -1
@@ -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);

security/commoncap.c int cap_bprm_set_security (struct linux_ · line 148

Quoted 9 lines
security/commoncap.c +6 -0
@@ -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 ...

security/commoncap.c int cap_bprm_set_security (struct linux_ · line 156

Quoted 8 lines
security/commoncap.c +8 -0
+                               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)) {

security/commoncap.c int cap_bprm_set_security (struct linux_ · line 164

Quoted 8 lines
security/commoncap.c +8 -0
+               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 ...

Quoted 19 lines
security/commoncap.c +16 -0
+                       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;
 }
Signature · 3 lines
Jesper Juhl <[email protected]>
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
author
#11 lore raw
Quoted 144 lines · Jesper Juhl

On 7/16/05, Nicholas Hans Simmonds <[email protected]> wrote:

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

Quoted 12 lines
fs/read_write.c +1 -0
@@ -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>

fs/read_write.c ssize_t vfs_write(struct file *file, con · line 309

Quoted 6 lines
fs/read_write.c +3 -0
@@ -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 ...
Quoted 72 lines
fs/read_write.c +7 -0
+                                                               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;
                        }
include/linux/capability.h +13 -1
@@ -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>
security/Kconfig +7 -0
@@ -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
security/commoncap.c +7 -1
@@ -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);

security/commoncap.c int cap_bprm_set_security (struct linux_ · line 148

Quoted 9 lines
security/commoncap.c +6 -0
@@ -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 ...

security/commoncap.c int cap_bprm_set_security (struct linux_ · line 156

Quoted 8 lines
security/commoncap.c +8 -0
+                               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)) {

security/commoncap.c int cap_bprm_set_security (struct linux_ · line 164

Quoted 8 lines
security/commoncap.c +8 -0
+               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 ...

Quoted 11 lines
security/commoncap.c +11 -0
+                       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 "
Quoted 13 lines
Quoted 8 lines
security/commoncap.c +5 -0
+                               "is %08X.\n",bprm->filename,caps.version,
+                               _LINUX_CAPABILITY_VERSION);
+       }
+#endif /* CONFIG_SECURITY_FS_CAPABILITIES */
+
        return 0;
 }
Signature · 3 lines
Jesper Juhl <[email protected]>
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.

fs/read_write.c +3 -2
@@ -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);
security/commoncap.c +4 -4
@@ -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;