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

[Christian Schoenebeck]

On Mittwoch, 27. April 2022 19:37:39 CEST Greg Kurz wrote:
> On Wed, 27 Apr 2022 18:18:31 +0200
> 
> Christian Schoenebeck <qemu_oss@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_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.
> 
> I took the question seriously :-), the problem is that even if you
> do a check on the target at symlink() time, you don't know how it
> will be evaluated in the end.
> 
> Quick demonstration:
> 
> $ cd /var/tmp/
> $ mkdir foo
> $ cd foo/
> $ # Suppose foo is the jail
> $ mkdir bar
> $ ln -sf .. bar/link
> $ realpath bar/link
> /var/tmp/foo
> $ # Good, we're still under foo
> $ mv bar/link .
> $ realpath link
> /var/tmp
> $ # Ouch we've escaped
> 
> So in the end, the only real fix is to ban path based syscalls and
> pass AT_SYMLINK_NOFOLLOW everywhere. This was the justification for
> rewriting nearly all 9p local in order to fix CVE-2016-9602.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg06225.html

Touché :) Agreed, it's not worth it.

I mean this simple example could still be addressed by catching the move, but 
if you have like several nested directories, each with a huge number of 
chained symlinks, on top of it non-atomic issues etc., then things would get 
way expensive to check, as you would actually have to traverse an entire tree 
and validate an even bigger amount of symlink pathes for every single symlink 
modification attempt on guest, probably even with exclusive locks, and so on.

> > [...]
> > 
> > > > > 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?
> As long as the link is not followed outside, we're good : it will change the
> symlink mode and then what ?

OK, then fine with me. I already filed a bug report with Apple for supporting 
fchmod(socket()). Hopefully we'll have a clean atomic solution in near future 
for this issue then.

I'll post v4 now. Thanks!

Best regards,
Christian Schoenebeck