Re: [PATCH v2 2/5] 9pfs: fix qemu_mknodat(S_IFSOCK) on macOS

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Mittwoch, 27. April 2022 15:31:42 CEST Greg Kurz wrote:
> On Wed, 27 Apr 2022 14:32:53 +0200
> 
> Christian Schoenebeck <qemu_oss@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.odaki@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.

[...]
> > > 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