[Security] XFS swapext ioctl minor security issues

[Dan Rosenberg]

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 at gmail.com>

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 at infradead.org> 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;
>
>
>