xfs
[Top] [All Lists]

Re: [Security] XFS swapext ioctl minor security issues

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [Security] XFS swapext ioctl minor security issues
From: Dan Rosenberg <dan.j.rosenberg@xxxxxxxxx>
Date: Wed, 16 Jun 2010 09:57:35 -0400
Cc: xfs@xxxxxxxxxxx, Eugene Teo <eugeneteo@xxxxxxxxx>, aelder@xxxxxxx, security@xxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:received:in-reply-to :references:date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=rdQLof4Ca3Mtl1MQJ1g1F3kTotdXTqIKCIaV6asNGeo=; b=kZrwiaB8lI5DTdHqvRBccJOzB1TTlzyxPEx26lnb+NjVYxbELAajuYWC3Vqq2IaMeX jmT8P3+XbpvN496mthy7jij5RpYcSeJbQCVActkJgzxdBtcP7Yr8Ac/uVNpZgEbeUJdD L+Njw4mUqvbkiLuGcQD/w8k1rxBYk1Z+63LMw=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=LmVPN7aalCLFK0N1GGZ8qjjnnrYKG/N2Bn/CS8qW/DprJ6Gv4nJyNBhg5olkZ2kv22 /Se1OczT6YZtIlANzjDTTnbfyO7oSNNljwBqnyDE0CfL8r5D0KCqOcNqTmyBXoKSwqBW g2sV6RxH04QxWlfNfGEB1/JA/ATxNZH6CSNik=
In-reply-to: <20100616133433.GA16437@xxxxxxxxxxxxx>
References: <AANLkTilrwmh6n7yYkqyvy_y5-bgS-BEDept0WlLg5GE1@xxxxxxxxxxxxxx> <AANLkTikGFq8iv4S3QWp5ZCvXJsjuiP2tKweSl6QwHc6U@xxxxxxxxxxxxxx> <20100616121142.GA22317@xxxxxxxxxxxxx> <AANLkTimVtpQuRZpOJ4IGlFzRqWG6XaYGzvUnDuF06MyG@xxxxxxxxxxxxxx> <20100616133433.GA16437@xxxxxxxxxxxxx>
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;
>
>
>

<Prev in Thread] Current Thread [Next in Thread>