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 majord...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
