linux-kernel - Re: [patch 1/1] ecryptfs: call __vfs_setxattr_noperm() in ecryptfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 5 Oct 2010 17:39:39 +0200
From:	Roberto Sassu <roberto.sassu@...ito.it>
To:	Tyler Hicks <tyhicks@...ux.vnet.ibm.com>
Cc:	kirkland@...onical.com, jmorris@...ei.org,
	akpm@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [patch 1/1] ecryptfs: call __vfs_setxattr_noperm() in ecryptfs_setxattr()

On Tuesday, October 05, 2010 08:23:53 am Tyler Hicks wrote:
> On Fri Oct 01, 2010 at 02:14:00PM -0700, akpm@...ux-foundation.org <akpm@...ux-foundation.org> wrote:
> > From: Roberto Sassu <roberto.sassu@...ito.it>
> 
> Andrew - thanks for not letting this one slip through.
> 
> > 
> > Ecryptfs is a stackable filesystem which relies on lower filesystems the
> > ability of setting/getting extended attributes.
> > 
> > If there is a security module enabled on the system it updates the
> > 'security' field of inodes according to the owned extended attribute set
> > with the function vfs_setxattr().  When this function is performed on a
> > ecryptfs filesystem the 'security' field is not updated for the lower
> > filesystem since the call security_inode_post_setxattr() is missing for
> > the lower inode.
> > 
> > This patch makes the function __vfs_setxattr_noperm() available for
> > modules and replaces the call to the setxattr() method of the lower inode
> > with the exported function.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@...ito.it>
> > Cc: Tyler Hicks <tyhicks@...ux.vnet.ibm.com>
> > Cc: Dustin Kirkland <kirkland@...onical.com>
> > Cc: James Morris <jmorris@...ei.org>
> > Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
> > ---
> > 
> >  fs/ecryptfs/inode.c |    5 +++--
> >  fs/xattr.c          |    1 +
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff -puN fs/ecryptfs/inode.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr fs/ecryptfs/inode.c
> > --- a/fs/ecryptfs/inode.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr
> > +++ a/fs/ecryptfs/inode.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/crypto.h>
> >  #include <linux/fs_stack.h>
> >  #include <linux/slab.h>
> > +#include <linux/xattr.h>
> >  #include <asm/unaligned.h>
> >  #include "ecryptfs_kernel.h"
> > 
> > @@ -1109,8 +1110,8 @@ ecryptfs_setxattr(struct dentry *dentry,
> >  		goto out;
> >  	}
> >  	mutex_lock(&lower_dentry->d_inode->i_mutex);
> > -	rc = lower_dentry->d_inode->i_op->setxattr(lower_dentry, name, value,
> > -						   size, flags);
> > +	rc = __vfs_setxattr_noperm(lower_dentry, name, value,
> > +				   size, flags);
> 
> Hi Roberto - Thanks for the fix. However, it seems to me like we should
> call vfs_setxattr(). We can't guarantee consistency among the eCryptfs
> inode and the lower inode, so the extra call to xattr_permission()
> isn't a waste.
> 

Hi Tyler

i look in deep at the code and i made the following considerations:
first, i think calling vfs_setxattr() instead of __vfs_setxattr is not necessary when ecryptfs is 
mounted on the top of a directory in the lower filesystem: in this case the only way
to modify the extended attributes of lower inodes is to call ecryptfs_setxattr().
Second, the use of vfs_setxattr() causes functions xattr_permission() and 
security_inode_setxattr() to be called twice. While the former is not needed the second time
because it calls ecryptfs_permission() which in turn invokes inode_permission() 
on the lower inode, i noted the second is required at least for SELinux in this situation:
suppose that ecryptfs and the lower filesystem have different security contexts for the 
filesystem object; in this case if the security_inode_setxattr() is missing for the lower inode,
the filesystem 'associate' permission is not checked (SELinux verifies if an inode with a 
given label can be stored on a filesystem bound to a specific security context).
I will post a second version of the patch which has the function __vfs_setxattr() replaced with
vfs_setxattr().


> >  	mutex_unlock(&lower_dentry->d_inode->i_mutex);
> >  out:
> >  	return rc;
> > diff -puN fs/xattr.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr fs/xattr.c
> > --- a/fs/xattr.c~ecryptfs-call-__vfs_setxattr_noperm-in-ecryptfs_setxattr
> > +++ a/fs/xattr.c
> > @@ -106,6 +106,7 @@ int __vfs_setxattr_noperm(struct dentry 
> > 
> >  	return error;
> >  }
> > +EXPORT_SYMBOL_GPL(__vfs_setxattr_noperm);
> > 
> > 
> >  int
> > _
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4707 bytes)