[PATCH] Fix user.* xattr permission check for sticky dirs

[PATCH] Fix user.* xattr permission check for sticky dirs

[Andreas Gruenbacher]

The user.* extended attributes are only allowed on regular files and
directories. Sticky directories further restrict write access to the
owner and privileged users. (See the attr(5) man page for an
explanation.)

The original check in ext2/ext3 when user.* xattrs were merged was more
restrictive than intended, and when the xattr permission checks were moved 
into the VFS, read access to user.* attributes on sticky directores ended up
being denied in addition.

Originally-from: Gerard Neil <xyzzy@devferret.org>
Signed-off-by: Andreas Gruenbacher <agruen@suse.de>

Index: linux-2.6.19-rc4/fs/xattr.c
===================================================================
--- linux-2.6.19-rc4.orig/fs/xattr.c
+++ linux-2.6.19-rc4/fs/xattr.c
@@ -48,14 +48,21 @@ xattr_permission(struct inode *inode, co
 		return 0;
 
 	/*
-	 * The trusted.* namespace can only accessed by a privilegued user.
+	 * The trusted.* namespace can only be accessed by a privileged user.
 	 */
 	if (!strncmp(name, XATTR_TRUSTED_PREFIX, XATTR_TRUSTED_PREFIX_LEN))
 		return (capable(CAP_SYS_ADMIN) ? 0 : -EPERM);
 
+	/* In user.* namespace, only regular files and directories can have
+	 * extended attributes. For sticky directories, only the owner and
+	 * privileged user can write attributes.
+	 */
 	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))
+		if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
+			return -EPERM;
+		if (S_ISDIR(inode->i_mode) && (inode->i_mode & S_ISVTX) &&
+		    (mask & MAY_WRITE) && (current->fsuid != inode->i_uid) &&
+		    !capable(CAP_FOWNER))
 			return -EPERM;
 	}
 

Re: [PATCH] Fix user.* xattr permission check for sticky dirs

[Jan Engelhardt]

>
>The user.* extended attributes are only allowed on regular files and
>directories. Sticky directories further restrict write access to the
>owner and privileged users. (See the attr(5) man page for an
>explanation.)
>

Does this effect ACL handling for sticky dirs in any way?


	-`J'
-- 

Re: [PATCH] Fix user.* xattr permission check for sticky dirs

[Dave Kleikamp]

On Thu, 2006-11-02 at 17:59 +0100, Jan Engelhardt wrote:
> >
> >The user.* extended attributes are only allowed on regular files and
> >directories. Sticky directories further restrict write access to the
> >owner and privileged users. (See the attr(5) man page for an
> >explanation.)
> >
> 
> Does this effect ACL handling for sticky dirs in any way?

No.  xattr_permission (which this patch hits) always returns 0 for
system.* attributes and leaves it to the file system to handle.

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

Re: [PATCH] Fix user.* xattr permission check for sticky dirs

[Andrew Morton]

On Thu, 2 Nov 2006 17:24:02 +0100
Andreas Gruenbacher <agruen@suse.de> wrote:

> The user.* extended attributes are only allowed on regular files and
> directories. Sticky directories further restrict write access to the
> owner and privileged users. (See the attr(5) man page for an
> explanation.)
> 
> The original check in ext2/ext3 when user.* xattrs were merged was more
> restrictive than intended, and when the xattr permission checks were moved 
> into the VFS, read access to user.* attributes on sticky directores ended up
> being denied in addition.

Am struggling to understand the impact of this.  I assume this problem was
introduced on Jan 9 by e0ad7b073eb7317e5afe0385b02dcb1d52a1eedf "move xattr
permission checks into the VFS"?

If so, the fix is applicable to 2.6.18, 2.6.19 and of course 2.6.20.

But to which of those should it be applied?

Re: [PATCH] Fix user.* xattr permission check for sticky dirs

[Andreas Gruenbacher]

On Thursday 02 November 2006 20:27, Andrew Morton wrote:
> On Thu, 2 Nov 2006 17:24:02 +0100
>
> Andreas Gruenbacher <agruen@suse.de> wrote:
> > The user.* extended attributes are only allowed on regular files and
> > directories. Sticky directories further restrict write access to the
> > owner and privileged users. (See the attr(5) man page for an
> > explanation.)
> >
> > The original check in ext2/ext3 when user.* xattrs were merged was more
> > restrictive than intended, and when the xattr permission checks were
> > moved into the VFS, read access to user.* attributes on sticky directores
> > ended up being denied in addition.
>
> Am struggling to understand the impact of this.  I assume this problem was
> introduced on Jan 9 by e0ad7b073eb7317e5afe0385b02dcb1d52a1eedf "move xattr
> permission checks into the VFS"?

Commits e0ad7b073eb7317e5afe0385b02dcb1d52a1eedf and 
c37ef806a3e1c0bca65fd03b7590d56d19625da4 move the following check from 
ext3_xattr_user_set() to xattr_permission(), which is used in vfs_getxattr() 
as well as xfs_setxattr() and vfs_removexattr(), so this added the check to 
the xfs_getxattr() path by accident:

[]	if (!S_ISREG(inode->i_mode) &&
[]	    (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
[]		return -EPERM;

The check itself completely forbids user.* xattrs for sticky directories 
already though, and this conflicts with the xattr(5) manual page as well as 
the xfs code. It looks as if the ckeck was more strict than intended since 
forever. The patch I have sent relaxes the unintended restriction.

> If so, the fix is applicable to 2.6.18, 2.6.19 and of course 2.6.20.

... and further back.

> But to which of those should it be applied?

I don't think we'll need backports; this doesn't address a security problem.

Thanks,
Andreas

Re: [PATCH] Fix user.* xattr permission check for sticky dirs

[Timothy Shimmin]

Hi Andreas,

--On 2 November 2006 10:51:21 PM +0100 Andreas Gruenbacher <agruen@suse.de> wrote:

> On Thursday 02 November 2006 20:27, Andrew Morton wrote:
>> On Thu, 2 Nov 2006 17:24:02 +0100
>>
>> Andreas Gruenbacher <agruen@suse.de> wrote:
>> > The user.* extended attributes are only allowed on regular files and
>> > directories. Sticky directories further restrict write access to the
>> > owner and privileged users. (See the attr(5) man page for an
>> > explanation.)
>> >
>> > The original check in ext2/ext3 when user.* xattrs were merged was more
>> > restrictive than intended, and when the xattr permission checks were
>> > moved into the VFS, read access to user.* attributes on sticky directores
>> > ended up being denied in addition.
>>
>> Am struggling to understand the impact of this.  I assume this problem was
>> introduced on Jan 9 by e0ad7b073eb7317e5afe0385b02dcb1d52a1eedf "move xattr
>> permission checks into the VFS"?
>
> Commits e0ad7b073eb7317e5afe0385b02dcb1d52a1eedf and
> c37ef806a3e1c0bca65fd03b7590d56d19625da4 move the following check from
> ext3_xattr_user_set() to xattr_permission(), which is used in vfs_getxattr()
> as well as xfs_setxattr() and vfs_removexattr(),

> so this added the check to
> the xfs_getxattr() path by accident:
>
> []	if (!S_ISREG(inode->i_mode) &&
> []	    (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
> []		return -EPERM;
>
>
Now, I'm a bit confused.
xfs_getxattr?
I see the "correct" version of the test in xfs_attr.c/attr_user_capable().

--Tim

Re: [PATCH] Fix user.* xattr permission check for sticky dirs

[Andreas Gruenbacher]

On Friday 03 November 2006 05:57, Timothy Shimmin wrote:
> > so this added the check to the xfs_getxattr() path by accident:
> >
> > []	if (!S_ISREG(inode->i_mode) &&
> > []	    (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
> > []		return -EPERM;
>
> Now, I'm a bit confused.
> xfs_getxattr?
> I see the "correct" version of the test in xfs_attr.c/attr_user_capable().

I meant to say fs/xattr.c:vfs_getxattr() and fs/xattr.c:vfs_setxattr(), sorry. 
The xfs code is fine, it just contains the same check once again in 
fs/xfs/xfs_attr.c:attr_user_capable().

Thanks,
Andreas