I removed the part of the patch dealing with suid/sgid bits - your
reasoning seems good, we clearly don't want to just drop the suid/sgid
bits. I was just trying to point out the case where the caller is not
the owner and has write access to the file; since in the ordinary case
writing to that file would result in dropping the suid bit, I thought
this ioctl should try to replicate that behavior.
Signed-off-by: Dan Rosenberg <dan.j.rosenberg@xxxxxxxxx>
diff -up fs/xfs/xfs_dfrag.c.orig fs/xfs/xfs_dfrag.c
--- fs/xfs/xfs_dfrag.c.orig 2010-06-15 09:16:05.000000000 -0400
+++ fs/xfs/xfs_dfrag.c 2010-06-16 09:47:01.000000000 -0400
@@ -69,7 +69,9 @@ xfs_swapext(
goto out;
}
- if (!(file->f_mode & FMODE_WRITE) || (file->f_flags & O_APPEND)) {
+ if (!(file->f_mode & FMODE_WRITE) ||
+ !(file->f_mode & FMODE_READ) ||
+ (file->f_flags & O_APPEND)) {
error = XFS_ERROR(EBADF);
goto out_put_file;
}
@@ -81,7 +83,8 @@ xfs_swapext(
}
if (!(tmp_file->f_mode & FMODE_WRITE) ||
- (tmp_file->f_flags & O_APPEND)) {
+ !(tmp_file->f_mode & FMODE_READ) ||
+ (tmp_file->f_flags & O_APPEND)) {
error = XFS_ERROR(EBADF);
goto out_put_tmp_file;
}
On Wed, Jun 16, 2010 at 9:34 AM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> On Wed, Jun 16, 2010 at 09:07:10AM -0400, Dan Rosenberg wrote:
>> Sure thing. This patch is against 2.6.34, but it appears that it can
>> apply to >= 2.6.25. Let me know if you need a fix for < 2.6.25.
>>
>> For those new to the conversation, this patch prevents user "foo" from
>> using the SWAPEXT ioctl to swap a write-only file owned by user "bar"
>> into a file owned by "foo" and subsequently reading it. It does so by
>> checking that the file descriptors passed to the ioctl are also opened
>> for reading.
>
> This part is okay. If you provide a Signed-off-by: line it
> can be put into the tree ASAP.
>
>> In addition, after swapping any suid/sgid bits should be
>> cleared.
>
> I'm still trying to understand this one. Clearly we do not want to
> simply drop the suid/sgid bits here - swapext is just supposed to
> optimize file layout, but not change i_mode. So if the suid bit
> really is a risk here we need to refuse to swapext it.
>
> What's the scenario here:
>
> - swapext is called by the owner and the suid bit is set, or
> the owner is member of the group and the sgid bit is set,
> this should be fine.
> - the caller is not the owner, but has write access to the file.
> an actual write to change the data by the user would already drop
> the bits, so in theory swapext should be fine. But we might lose
> some important metadata in extended attributes that previously
> might have made the file safe, say per-file capabilities.
> I'd love to here the exact scenario, but for let's play safe and
> refuse the swapex for that case, by doing something:
>
> /*
> * SGID without any exec bits is just a mandatory locking mark.
> */
> #define is_sgid(mode) \
> (((mode) & (S_ISGID|S_IXGRP)) == (S_ISGID|S_IXGRP))
>
> ...
>
> struct inode *inode, *tmp_inode;
>
> ...
>
> inode = file->f_path.dentry->d_inode;
> tmp_inode = tmp_file->f_path.dentry->d_inode;
>
> ...
>
> error = XFS_ERROR(EPERM);
> if ((inode->i_mode & S_ISUID) && !is_owner_or_cap(inode))
> goto out;
> if ((tmp_inode->i_mode & S_ISUID) && !is_owner_or_cap(tmp_inode))
> goto out;
> if (is_sgid(inode->i_mode) && !in_group_p(inode->i_gid))
> goto out;
> if (is_sgid(tmp_inode->i_mode) && !in_group_p(tmp_inode->i_gid))
> goto out;
>
>
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
