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 <[email protected]>
Signed-off-by: Andreas Gruenbacher <[email protected]>
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;
}
>
>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'
--
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
On Thu, 2 Nov 2006 17:24:02 +0100
Andreas Gruenbacher <[email protected]> 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?
On Thursday 02 November 2006 20:27, Andrew Morton wrote:
> On Thu, 2 Nov 2006 17:24:02 +0100
>
> Andreas Gruenbacher <[email protected]> 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
Hi Andreas,
--On 2 November 2006 10:51:21 PM +0100 Andreas Gruenbacher <[email protected]> wrote:
> On Thursday 02 November 2006 20:27, Andrew Morton wrote:
>> On Thu, 2 Nov 2006 17:24:02 +0100
>>
>> Andreas Gruenbacher <[email protected]> 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
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