[PATCH 2/8] move xattr permission checks into the VFS

[PATCH 2/8] move xattr permission checks into the VFS

[Christoph Hellwig]

The xattr code has rather complex permission checks because the
rules are very different for different attribute namespaces.  This
patch moves as much as we can into the generic code.  Currenyly
all the major disk based filesystems duplicate these checks, while
many minor filesystems or network filesystems lack some or all of them.

To do this we need defines for the extended attribute names in common
code, I moved them up from JFS which had the nicest defintions.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/fs/jfs/xattr.c
===================================================================
--- linux-2.6.orig/fs/jfs/xattr.c	2005-10-31 12:23:57.000000000 +0100
+++ linux-2.6/fs/jfs/xattr.c	2005-10-31 14:34:40.000000000 +0100
@@ -83,21 +83,6 @@
 #define EA_NEW		0x0004
 #define EA_MALLOC	0x0008
 
-/* Namespaces */
-#define XATTR_SYSTEM_PREFIX "system."
-#define XATTR_SYSTEM_PREFIX_LEN (sizeof (XATTR_SYSTEM_PREFIX) - 1)
-
-#define XATTR_USER_PREFIX "user."
-#define XATTR_USER_PREFIX_LEN (sizeof (XATTR_USER_PREFIX) - 1)
-
-#define XATTR_OS2_PREFIX "os2."
-#define XATTR_OS2_PREFIX_LEN (sizeof (XATTR_OS2_PREFIX) - 1)
-
-/* XATTR_SECURITY_PREFIX is defined in include/linux/xattr.h */
-#define XATTR_SECURITY_PREFIX_LEN (sizeof (XATTR_SECURITY_PREFIX) - 1)
-
-#define XATTR_TRUSTED_PREFIX "trusted."
-#define XATTR_TRUSTED_PREFIX_LEN (sizeof (XATTR_TRUSTED_PREFIX) - 1)
 
 /*
  * These three routines are used to recognize on-disk extended attributes
Index: linux-2.6/fs/xattr.c
===================================================================
--- linux-2.6.orig/fs/xattr.c	2005-10-31 14:34:33.000000000 +0100
+++ linux-2.6/fs/xattr.c	2005-10-31 14:35:23.000000000 +0100
@@ -20,6 +20,47 @@
 #include <asm/uaccess.h>
 
 
+/*
+ * Check permissions for extended attribute access.  This is a bit complicated
+ * because different namespaces have very different rules.
+ */
+static int
+xattr_permission(struct inode *inode, const char *name, int mask)
+{
+	/*
+	 * We can never set or remove an extended attribute on a read-only
+	 * filesystem  or on an immutable / append-only inode.
+	 */
+	if (mask & MAY_WRITE) {
+		if (IS_RDONLY(inode))
+			return -EROFS;
+		if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
+			return -EPERM;
+	}
+
+	/*
+	 * No restriction for security.* and system.* from the VFS.  Decision
+	 * on these is left to the underlying filesystem / security module.
+	 */
+	if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN) ||
+	    !strncmp(name, XATTR_SYSTEM_PREFIX, XATTR_SYSTEM_PREFIX_LEN))
+		return 0;
+
+	/*
+	 * The trusted.* namespace can only accessed by a privilegued user.
+	 */
+	if (!strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN))
+		return (capable(CAP_SYS_ADMIN) ? 0 : -EPERM);
+
+	if (!strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN)) {
+		if (!S_ISREG(inode->i_mode) &&
+		    (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
+			return -EPERM;
+	}
+
+	return permission(inode, mask, NULL);
+}
+
 int
 vfs_setxattr(struct dentry *dentry, char *name, void *value,
 		size_t size, int flags)
@@ -27,6 +68,10 @@
 	struct inode *inode = dentry->d_inode;
 	int error;
 
+	error = xattr_permission(inode, name, MAY_WRITE);
+	if (error)
+		return error;
+
 	down(&inode->i_sem);
 	error = security_inode_setxattr(dentry, name, value, size, flags);
 	if (error)
@@ -40,8 +85,8 @@
 						     size, flags);
 		}
 	} else if (!strncmp(name, XATTR_SECURITY_PREFIX,
-				sizeof XATTR_SECURITY_PREFIX - 1)) {
-		const char *suffix = name + sizeof XATTR_SECURITY_PREFIX - 1;
+				XATTR_SECURITY_PREFIX_LEN)) {
+		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
 		error = security_inode_setsecurity(inode, suffix, value,
 						   size, flags);
 		if (!error)
@@ -59,6 +104,10 @@
 	struct inode *inode = dentry->d_inode;
 	int error;
 
+	error = xattr_permission(inode, name, MAY_READ);
+	if (error)
+		return error;
+
 	error = security_inode_getxattr(dentry, name);
 	if (error)
 		return error;
@@ -69,8 +118,8 @@
 		error = -EOPNOTSUPP;
 
 	if (!strncmp(name, XATTR_SECURITY_PREFIX,
-				sizeof XATTR_SECURITY_PREFIX - 1)) {
-		const char *suffix = name + sizeof XATTR_SECURITY_PREFIX - 1;
+				XATTR_SECURITY_PREFIX_LEN)) {
+		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
 		int ret = security_inode_getsecurity(inode, suffix, value,
 						     size, error);
 		/*
@@ -94,6 +143,10 @@
 	if (!inode->i_op->removexattr)
 		return -EOPNOTSUPP;
 
+	error = xattr_permission(inode, name, MAY_WRITE);
+	if (error)
+		return error;
+
 	error = security_inode_removexattr(dentry, name);
 	if (error)
 		return error;
Index: linux-2.6/include/linux/xattr.h
===================================================================
--- linux-2.6.orig/include/linux/xattr.h	2005-10-31 14:34:33.000000000 +0100
+++ linux-2.6/include/linux/xattr.h	2005-10-31 14:34:40.000000000 +0100
@@ -13,7 +13,22 @@
 #define XATTR_CREATE	0x1	/* set value, fail if attr already exists */
 #define XATTR_REPLACE	0x2	/* set value, fail if attr does not exist */
 
+/* Namespaces */
+#define XATTR_OS2_PREFIX "os2."
+#define XATTR_OS2_PREFIX_LEN (sizeof (XATTR_OS2_PREFIX) - 1)
+
 #define XATTR_SECURITY_PREFIX	"security."
+#define XATTR_SECURITY_PREFIX_LEN (sizeof (XATTR_SECURITY_PREFIX) - 1)
+
+#define XATTR_SYSTEM_PREFIX "system."
+#define XATTR_SYSTEM_PREFIX_LEN (sizeof (XATTR_SYSTEM_PREFIX) - 1)
+
+#define XATTR_TRUSTED_PREFIX "trusted."
+#define XATTR_TRUSTED_PREFIX_LEN (sizeof (XATTR_TRUSTED_PREFIX) - 1)
+
+#define XATTR_USER_PREFIX "user."
+#define XATTR_USER_PREFIX_LEN (sizeof (XATTR_USER_PREFIX) - 1)
+
 
 struct xattr_handler {
 	char *prefix;

Re: [PATCH 2/8] move xattr permission checks into the VFS

[Trond Myklebust]

On Tue, 2005-11-01 at 03:30 +0100, Christoph Hellwig wrote:
> The xattr code has rather complex permission checks because the
> rules are very different for different attribute namespaces.  This
> patch moves as much as we can into the generic code.  Currenyly
> all the major disk based filesystems duplicate these checks, while
> many minor filesystems or network filesystems lack some or all of them.
> 
> To do this we need defines for the extended attribute names in common
> code, I moved them up from JFS which had the nicest defintions.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/fs/jfs/xattr.c
> ===================================================================
> --- linux-2.6.orig/fs/jfs/xattr.c	2005-10-31 12:23:57.000000000 +0100
> +++ linux-2.6/fs/jfs/xattr.c	2005-10-31 14:34:40.000000000 +0100
> @@ -83,21 +83,6 @@
>  #define EA_NEW		0x0004
>  #define EA_MALLOC	0x0008
>  
> -/* Namespaces */
> -#define XATTR_SYSTEM_PREFIX "system."
> -#define XATTR_SYSTEM_PREFIX_LEN (sizeof (XATTR_SYSTEM_PREFIX) - 1)
> -
> -#define XATTR_USER_PREFIX "user."
> -#define XATTR_USER_PREFIX_LEN (sizeof (XATTR_USER_PREFIX) - 1)
> -
> -#define XATTR_OS2_PREFIX "os2."
> -#define XATTR_OS2_PREFIX_LEN (sizeof (XATTR_OS2_PREFIX) - 1)
> -
> -/* XATTR_SECURITY_PREFIX is defined in include/linux/xattr.h */
> -#define XATTR_SECURITY_PREFIX_LEN (sizeof (XATTR_SECURITY_PREFIX) - 1)
> -
> -#define XATTR_TRUSTED_PREFIX "trusted."
> -#define XATTR_TRUSTED_PREFIX_LEN (sizeof (XATTR_TRUSTED_PREFIX) - 1)
>  
>  /*
>   * These three routines are used to recognize on-disk extended attributes
> Index: linux-2.6/fs/xattr.c
> ===================================================================
> --- linux-2.6.orig/fs/xattr.c	2005-10-31 14:34:33.000000000 +0100
> +++ linux-2.6/fs/xattr.c	2005-10-31 14:35:23.000000000 +0100
> @@ -20,6 +20,47 @@
>  #include <asm/uaccess.h>
>  
> 
> +/*
> + * Check permissions for extended attribute access.  This is a bit complicated
> + * because different namespaces have very different rules.
> + */
> +static int
> +xattr_permission(struct inode *inode, const char *name, int mask)
> +{
> +	/*
> +	 * We can never set or remove an extended attribute on a read-only
> +	 * filesystem  or on an immutable / append-only inode.
> +	 */
> +	if (mask & MAY_WRITE) {
> +		if (IS_RDONLY(inode))
> +			return -EROFS;
> +		if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> +			return -EPERM;
> +	}
> +
> +	/*
> +	 * No restriction for security.* and system.* from the VFS.  Decision
> +	 * on these is left to the underlying filesystem / security module.
> +	 */
> +	if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN) ||
> +	    !strncmp(name, XATTR_SYSTEM_PREFIX, XATTR_SYSTEM_PREFIX_LEN))
> +		return 0;
> +
> +	/*
> +	 * The trusted.* namespace can only accessed by a privilegued user.
> +	 */
> +	if (!strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN))
> +		return (capable(CAP_SYS_ADMIN) ? 0 : -EPERM);
> +
> +	if (!strncmp(name, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN)) {
> +		if (!S_ISREG(inode->i_mode) &&
> +		    (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
> +			return -EPERM;
> +	}
> +
> +	return permission(inode, mask, NULL);
> +}

The call to permission() here is not always applicable. Some filesystems
may have different permissions when it comes to the right to read or set
ACLs (both AFS and NFSv4, for instance, have such features).

For the NFSv4 client, therefore, I'd like to be able to override this
particular check (and leave it up to the server to verify that we are
authorised).

Cheers,
  Trond

Re: [PATCH 2/8] move xattr permission checks into the VFS

[Christoph Hellwig]

On Mon, Oct 31, 2005 at 10:22:09PM -0500, Trond Myklebust wrote:
> > +	/*
> > +	 * No restriction for security.* and system.* from the VFS.  Decision
> > +	 * on these is left to the underlying filesystem / security module.
> > +	 */
> > +	if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN) ||
> > +	    !strncmp(name, XATTR_SYSTEM_PREFIX, XATTR_SYSTEM_PREFIX_LEN))
> > +		return 0;

> The call to permission() here is not always applicable. Some filesystems
> may have different permissions when it comes to the right to read or set
> ACLs (both AFS and NFSv4, for instance, have such features).
> 
> For the NFSv4 client, therefore, I'd like to be able to override this
> particular check (and leave it up to the server to verify that we are
> authorised).

See the code above.  system namespace attributes (which ACLs are) are
never handled in the VFS but always left to the filesystem.

Re: [PATCH 2/8] move xattr permission checks into the VFS

[Trond Myklebust]

On Tue, 2005-11-01 at 04:24 +0100, Christoph Hellwig wrote:
> On Mon, Oct 31, 2005 at 10:22:09PM -0500, Trond Myklebust wrote:
> > > +	/*
> > > +	 * No restriction for security.* and system.* from the VFS.  Decision
> > > +	 * on these is left to the underlying filesystem / security module.
> > > +	 */
> > > +	if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN) ||
> > > +	    !strncmp(name, XATTR_SYSTEM_PREFIX, XATTR_SYSTEM_PREFIX_LEN))
> > > +		return 0;
> 
> > The call to permission() here is not always applicable. Some filesystems
> > may have different permissions when it comes to the right to read or set
> > ACLs (both AFS and NFSv4, for instance, have such features).
> > 
> > For the NFSv4 client, therefore, I'd like to be able to override this
> > particular check (and leave it up to the server to verify that we are
> > authorised).
> 
> See the code above.  system namespace attributes (which ACLs are) are
> never handled in the VFS but always left to the filesystem.

Ah... I see...

...but then I'm a bit confused w.r.t. your removal of the checks for
S_ISREG(inode->i_mode) & friends in nfs4proc.c nfs4_setxattr(). You
motivate the removal by the fact that you've moved those checks into the
VFS, but AFAICS we will never hit the VFS checks for the case of
system.nfs4_acl (which is the only case we care about right now).

Cheers,
  Trond

Re: [PATCH 2/8] move xattr permission checks into the VFS

[Christoph Hellwig]

On Mon, Oct 31, 2005 at 10:33:23PM -0500, Trond Myklebust wrote:
> ...but then I'm a bit confused w.r.t. your removal of the checks for
> S_ISREG(inode->i_mode) & friends in nfs4proc.c nfs4_setxattr(). You
> motivate the removal by the fact that you've moved those checks into the
> VFS, but AFAICS we will never hit the VFS checks for the case of
> system.nfs4_acl (which is the only case we care about right now).

Hmm, that's was at least half of an oversight.  The other filesystems
do exactly this check only for user ACLs. Posix ACLs are supported
on all files but links and in the other filesystems these link checks
are hidden much deeper in the callchanin.

If this check is the right thing for NFSv4 ACLs we should drop the nfs
patch.