Re: [Security] XFS swapext ioctl minor security issues

Re: [Security] XFS swapext ioctl minor security issues

[Dan Rosenberg]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

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.  In addition, after swapping any suid/sgid bits should be
cleared.

-Dan

On Wed, Jun 16, 2010 at 8:11 AM, Christoph Hellwig <hch@infradead.org> wrote:
> Dan, can you please send your fixes to the XFS list so that we can
> include them?
>
>

[-- Attachment #2: xfs-swapext-ioctl.patch --]
[-- Type: text/x-patch, Size: 1076 bytes --]

--- fs/xfs/xfs_dfrag.c.orig     2010-06-15 09:16:05.000000000 -0400
+++ fs/xfs/xfs_dfrag.c  2010-06-15 09:30:17.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;
        }
@@ -112,6 +115,11 @@ xfs_swapext(

        error = xfs_swap_extents(ip, tip, sxp);

+       if(!error) {
+               file_remove_suid(tmp_file);
+               file_remove_suid(file);
+       }
+ 
  out_put_tmp_file:
        fput(tmp_file);
  out_put_file:


[-- Attachment #3: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [Security] XFS swapext ioctl minor security issues

[Christoph Hellwig]

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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [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@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@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;
>
>
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [Security] XFS swapext ioctl minor security issues

[Christoph Hellwig]

On Wed, Jun 16, 2010 at 09:57:35AM -0400, Dan Rosenberg wrote:
> 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.

Looks good,

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

Alex, can you push it to Linus ASAP?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs