On Wed, Apr 27, 2022 at 12:18 PM Christian Schoenebeck < qemu_...@crudebyte.com> wrote:
> On Mittwoch, 27. April 2022 15:31:42 CEST Greg Kurz wrote: > > On Wed, 27 Apr 2022 14:32:53 +0200 > > > > Christian Schoenebeck <qemu_...@crudebyte.com> wrote: > > > On Mittwoch, 27. April 2022 12:18:10 CEST Greg Kurz wrote: > > > > On Wed, 27 Apr 2022 11:27:28 +0900 > > > > > > > > Akihiko Odaki <akihiko.od...@gmail.com> wrote: > > > > > On 2022/04/26 21:38, Greg Kurz wrote: > [...] > > > > > Considering the transient states are tolerated in 9pfs, we need to > > > > > design this function to be tolerant with transient states as well. > The > > > > > use of chmod() is not safe when we consider about transient > states. A > > > > > malicious actor may replace the file at the path with a symlink > which > > > > > may escape the shared directory and chmod() will naively follow it. > > > > > > > > You get a point here. Thanks for your tenacity ! :-) > > > > > > Yep, I send a v4 with fchmodat_nofollow() instead of chmod(), thanks! > > > > > > BTW, why is it actually allowed for client to create a symlink pointing > > > outside exported directory tree with security_model=passthrough/none? > Did > > > anybody want that? > > > > The target argument to symlink() is merely a string that is stored in > > the inode. It is only evaluated as a path at the time an action is > > made on the link. Checking at symlink() time is thus useless. > > > > Anyway, we're safe on this side since it's the client's job to > > resolve links and we explicitly don't follow them in the server. > > I wouldn't call it useless, because it is easier to keep exactly 1 hole > stuffed instead of being forced to continuously stuff N holes as new ones > may > popup up over time, as this case shows (mea culpa). > > So you are trading (probably minor) performance for security. > > But my question was actually meant seriously: whether there was anybody in > the > past who probably actually wanted to create relative symlinks outside the > exported tree. For instance for another service with wider host filesystem > access. > For what it's worth, the inability to follow symlinks read-write outside of the tree appears to be, at the moment, the primary pain point for new users of 9pfs in containerization software (see the later comments in https://github.com/lima-vm/lima/pull/726 and to a lesser extent https://github.com/containers/podman/issues/13784). To my knowledge, neither podman nor lima have come up with conclusive preferred solutions for how to address this, much less had a robust discussion with the QEMU team. The lima discussion notes that it works read-only with passthrough/none, so I'd suggest that if there weren't users of it before, there are now! As I understand it, one partial solution for downstream software that allows for read-write may just be to more proactively mount larger directories to minimize the number of external paths that symlinks might get tripped up on. That said, this will stop working when it comes to linking to additional mounts, since /Volumes on darwin will never line up with /mnt. > [...] > > > > This brings up a new problem I hadn't realized before : the > > > > fchmodat_nofollow() implementation in 9p-local.c is really > > > > a linux only thing to cope with AT_SYMLINK_NOFOLLOW not being > > > > supported with fchmodat(). It looks that this should move to > > > > 9p-util-linux.c and a proper version should be added for macOS > > > > in 9p-util-darwin.c > > > > > > Like already agreed on the other thread, yes, that makes sense. But I > > > think > > > this can be handled with a follow-up, separate from this series. > > > > Fair enough if you want to handle fchmodat_nofollow() later but you > > must at least use fchmodat(AT_SYMLINK_NOFOLLOW) in this patch > > instead of chmod(). > > Sounds like a quick and easy workaround. However looking at 'man fchmodat' > on > macOS, this probably does not exactly do what you wanted it to, at least > not > in this particular case: > > AT_SYMLINK_NOFOLLOW > If path names a symbolic link, then the mode of the symbolic > link is changed. > > AT_SYMLINK_NOFOLLOW_ANY > If path names a symbolic link, then the mode of the symbolic > link is changed and > if if the path has any other symbolic links, an error is > returned. > > So if somebody replaced the socket file by a 1st order symlink, you would > adjust the symlink's permissions with both AT_SYMLINK_NOFOLLOW as well as > with > AT_SYMLINK_NOFOLLOW_ANY. I mean it's better than chmod(), but acceptable? > > Using our existing fchmodat_nofollow() instead is no viable alternative > either, as it uses operations that are not supported on socket files on > macOS > (tested). > > Best regards, > Christian Schoenebeck > > >
