[PATCH 1/2] Btrfs: don't check the permission of the subvolume which we want to delete

[PATCH 1/2] Btrfs: don't check the permission of the subvolume which we want to delete

[Miao Xie]

Step to reproduce:
 # mkfs.btrfs <disk>
 # mount -o user_subvol_rm_allowed <disk> <mnt>
 # mkdir <mnt>/dir0
 # chmod 777 <mnt>/dir0
 # btrfs sub snap <mnt> <mnt>/dir0/snap0
 # su -c "btrfs sub del <mnt>/dir0/snap0" -s /bin/bash nobody
 ERROR: cannot delete '<mnt>/dir0/snap0' - Permission denied

This is because we checked the permission of the subvolume that we want to
delete, and found the user - nobody have no WRITE permission of this subvolume.

I think we need not check the permission of the subvolume we want to delete,
because we have the right to clean up the directory since we have WRITE and
EXECUTE permission, just like rmdir command.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index f5a2e6c..29fb07c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2062,10 +2062,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		if (root == dest)
 			goto out_dput;
 
-		err = inode_permission(inode, MAY_WRITE | MAY_EXEC);
-		if (err)
-			goto out_dput;
-
 		/* check if subvolume may be deleted by a non-root user */
 		err = btrfs_may_delete(dir, dentry, 1);
 		if (err)
-- 
1.7.6.5

Re: [PATCH 1/2] Btrfs: don't check the permission of the subvolume which we want to delete

[Goffredo Baroncelli]

On 2012-10-22 13:38, Miao Xie wrote:
> Step to reproduce:
>  # mkfs.btrfs <disk>
>  # mount -o user_subvol_rm_allowed <disk> <mnt>
>  # mkdir <mnt>/dir0
>  # chmod 777 <mnt>/dir0
>  # btrfs sub snap <mnt> <mnt>/dir0/snap0
>  # su -c "btrfs sub del <mnt>/dir0/snap0" -s /bin/bash nobody
>  ERROR: cannot delete '<mnt>/dir0/snap0' - Permission denied
> 
> This is because we checked the permission of the subvolume that we want to
> delete, and found the user - nobody have no WRITE permission of this subvolume.
> 
> I think we need not check the permission of the subvolume we want to delete,
> because we have the right to clean up the directory since we have WRITE and
> EXECUTE permission, just like rmdir command.

I think that removing a subvolume is a bit different than removing a
directory.
With "user_subvol_rm_allowed" allow an ordinary user to remove  things
that an plain "rm -rf" is not allowed.

For example if inside a directories tree there is a directory with
permission 700 (uid==root) an ordinary user is not able to remove this
directory.
Instead with the subvolumes (and the flag user_subvol_rm_allowed) an
ordinary user would be allowed to do it.
As mitigation it is required that user has WX permission on subvolume.
IIRC this is what we discussed at time.

See the thread "[PATCH] Btrfs: allow subvol deletion by unprivileged
user with -o user_subvol_rm_allowed"

http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg06525.html


> 
> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
>  fs/btrfs/ioctl.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index f5a2e6c..29fb07c 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2062,10 +2062,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>  		if (root == dest)
>  			goto out_dput;
>  
> -		err = inode_permission(inode, MAY_WRITE | MAY_EXEC);
> -		if (err)
> -			goto out_dput;
> -
>  		/* check if subvolume may be deleted by a non-root user */
>  		err = btrfs_may_delete(dir, dentry, 1);
>  		if (err)


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 1/2] Btrfs: don't check the permission of the subvolume which we want to delete

[Miao Xie]

On Mon, 22 Oct 2012 19:00:18 +0200, Goffredo Baroncelli wrote:
> On 2012-10-22 13:38, Miao Xie wrote:
>> Step to reproduce:
>>  # mkfs.btrfs <disk>
>>  # mount -o user_subvol_rm_allowed <disk> <mnt>
>>  # mkdir <mnt>/dir0
>>  # chmod 777 <mnt>/dir0
>>  # btrfs sub snap <mnt> <mnt>/dir0/snap0
>>  # su -c "btrfs sub del <mnt>/dir0/snap0" -s /bin/bash nobody
>>  ERROR: cannot delete '<mnt>/dir0/snap0' - Permission denied
>>
>> This is because we checked the permission of the subvolume that we want to
>> delete, and found the user - nobody have no WRITE permission of this subvolume.
>>
>> I think we need not check the permission of the subvolume we want to delete,
>> because we have the right to clean up the directory since we have WRITE and
>> EXECUTE permission, just like rmdir command.
> 
> I think that removing a subvolume is a bit different than removing a
> directory.
> With "user_subvol_rm_allowed" allow an ordinary user to remove  things
> that an plain "rm -rf" is not allowed.
> 
> For example if inside a directories tree there is a directory with
> permission 700 (uid==root) an ordinary user is not able to remove this
> directory.
> Instead with the subvolumes (and the flag user_subvol_rm_allowed) an
> ordinary user would be allowed to do it.
> As mitigation it is required that user has WX permission on subvolume.
> IIRC this is what we discussed at time.
> 
> See the thread "[PATCH] Btrfs: allow subvol deletion by unprivileged
> user with -o user_subvol_rm_allowed"
> 
> http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg06525.html

I don't think this mitigation is reasonable, it will make the user be
confused. As I said in another mail:

>> I don't think we can identify "btrfs sub del" with "rm -rf", because "rm -rf"
>> will check the permission of the parent directory of each file/directory which
>> is going to be deleted, but "btrfs sub del" doesn't do it, it will see all the
>> file/directory in the subvolume as one, so I think it seems like a special
>> "rmdir".

So I think we needn't the permission of the subvolume which is going to be deleted.

Thanks
Miao

> 
> 
>>
>> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
>> ---
>>  fs/btrfs/ioctl.c |    4 ----
>>  1 files changed, 0 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index f5a2e6c..29fb07c 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -2062,10 +2062,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>>  		if (root == dest)
>>  			goto out_dput;
>>  
>> -		err = inode_permission(inode, MAY_WRITE | MAY_EXEC);
>> -		if (err)
>> -			goto out_dput;
>> -
>>  		/* check if subvolume may be deleted by a non-root user */
>>  		err = btrfs_may_delete(dir, dentry, 1);
>>  		if (err)
> 
>