Re: [RFC] Improve subvolume usability for a normal user

[Goffredo Baroncelli <kreijack@inwind.it>]

On 12/05/2017 09:17 PM, Austin S. Hemmelgarn wrote:
> On 2017-12-05 14:09, Goffredo Baroncelli wrote:
>> On 12/05/2017 07:46 PM, Graham Cobb wrote:
>>> On 05/12/17 18:01, Goffredo Baroncelli wrote:
>>>> On 12/05/2017 04:42 PM, Graham Cobb wrote:
>> [....]
>>>>>>> Then no impact to kernel, all complex work is done in user space.
>>>>>> Exactly how hard is it to just check ownership of the root inode of a
>>>>>> subvolume from the ioctl context?  You could just as easily push all the
>>>>>> checking out to the VFS layer by taking an open fd for the subvolume
>>>>>> root (and probably implicitly closing it) instead of taking a path, and
>>>>>> that would give you all the benefits of ACL's and whatever security
>>>>>> modules the local system is using.
>>>>>
>>>>> +1 - stop inventing new access control rules for each different action!
>>>>
>>>> A subvolume is like a directory; In all filesystems you cannot remove an inode if you don't have write access to the parent directory. I assume that this is a POSIX requirement; and if so this should be true even for BTRFS.
>>>> This means that in order to remove a subvolume and you are not root, you should check all the contained directories. It is not sufficient to check one inode.
>>>>
>>>> In the past I create a patch [1][2] which extended the unlink (2) syscall to allow removing a subvolume if:
>>>> a) the user is root  or
>>>> b.1) if the subvolume is empty and
>>>> b.2) the user has the write capability to the parent directory
>>>
>>> That is also OK, as it follows a logical and consistent model without
>>> introducing new rules, but it has two disadvantages with snapshots the
>>> user creates and then wants to delete:
>>>
>>> i) they have to change the snapshot to readwrite to remove all the
>>> contents -- how many users will know that that can even be done?
>>
>> I think that this is an orthogonal question.  However the user should use btrfs set....
>>
>> Anyway I am not against to use something more specific like "btrfs sub del...", where it could be possible to implement a more specific checks. I am only highlight that destroy a subvolume without looking to its content could break the POSIX rules
> No, it doesn't break POSIX rules, because you can't do it with POSIX defined interfaces (not counting ioctls, but ioctls are by definition system dependent).

user$ mkdir -p dir1/dir2
user$ touch dir1/dir2/file
user$ sudo chown foo:foo dir1/dir2

user$ rm -rf dir1/
rm: cannot remove 'dir1/dir2/file1': Permission denied

In a POSIX filesystem, you (as user) cannot remove file1.

But if instead of dir1 you have a subvolume (called sub1), and the check of removing the subvolume is performed only on the root of subvolume, you could remove it.

Is it a problem ? I think no, but I am not a security expert. But even if POSIX doesn't have the concept of subvolume, this is definitely a break of a POSIX rule.

On the other side, if the user makes a snapshot of a subvolume containing a not owned and not empty directory, for the user it would be impossible to delete its snapshot (!).


>>
>>>
>>> ii) if the snapshot contains a file the user cannot remove then the user
>>> cannot remove the snapshot at all (even though they could create it).
>>
>> It is the same for the other filesystem: if in a filesystem tree, there is a directory which is not changeable by the user, the user cannot remove all the tree. Again, I am only highlighting that there is a possible break of POSIX rules.
> POSIX says nothing about non-permissions related reasons for not being able to remove a directory. 
Nobody has say the opposite

>  A subvolume is treated (too much in my opinion) like a mount point, regardless of whether or not it's explicitly mounted, and that seems to be most of why you can't remove it with unlink().



>>
>>>
>>> That is why I preferred Austin's first proposal. I suppose your proposal
>>> could be extended:
>>>
>>> a) the user is root  or
>>> b.1) if the subvolume is empty and
>>> b.2) the user has the write capability to the parent directory, or
>>> c) the subvolume is readonly
>>
>> If a subvolume is marked read-only, the user has to change to RW via "btrfs property set...."
> I actually do agree with this assessment to a certain extent, but the user has to be able to change the subvolume to be RW to begin with (and that _does_ need an in-kernel permissions check beyond just knowing whether you can access the subvolume).
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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