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:
> [..skip..]
>
> > > I think Christian's explanation is clear enough. We don't guarantee
> > > that v9fs_co_foo() calls run atomically. As a consequence, the client
> > > might see transient states or be able to interact with an ongoing
> > > request. And to answer your question, we have no specific rationale
> > > on security with that.
> > >
> > > I'm not sure what the concerns are but unless you come up with a
> > > valid scenario [*] I don't see any reason to prevent this patch
> > > to go forward.
> > >
> > > [*] things like:
> > > - client escaping the shared directory
> > > - QEMU crashing
> > > - QEMU hogging host resources
> > > - client-side unprivileged user gaining elevated privleges
> > >
> > > in the guest
> >
> > I was just not sure if such transient states are safe. The past
> > discussion was about the length of the non-atomic time window where a
> > path name is used to identify a particular file, but if such states are
> > not considered problematic, the length does not matter all and we can
> > confidently say the sequence of bind() and chmod() is safe.
> >
> > 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?
> > chmod() should be replaced with fchmodat_nofollow() or something similar.
>
> On a GNU/Linux system, this could be achieved by calling fchmod() on
> the socket fd *before* calling bind() but I'm afraid this hack might
> not work with a BSDish OS.
As you already imagined, this is unfortunately not supported by any BSDs,
including macOS. I'll file a bug report with Apple though.
> Replacing chmod() with fchmodat_nofollow(dirfd, addr.sun_path, mode)
> won't make things atomic as above but at least it won't follow a
> malicious symbolic link : mknod() on the client will fail with
> ELOOP, which is fine when it comes to not breaking out of the shared
> directory.
Current security_model=passthrough/none already has similar non-atomic
operations BTW, so this was not something new. E.g.:
static int local_symlink(FsContext *fs_ctx, const char *oldpath,
V9fsPath *dir_path, const char *name, FsCred *credp)
{
...
} else if (fs_ctx->export_flags & V9FS_SM_PASSTHROUGH ||
fs_ctx->export_flags & V9FS_SM_NONE) {
err = symlinkat(oldpath, dirfd, name);
if (err) {
goto out;
}
err = fchownat(dirfd, name, credp->fc_uid, credp->fc_gid,
AT_SYMLINK_NOFOLLOW);
...
}
In general, if you care about a higher degree of security, I'd always
recommend to use security_model=mapped in the first place.
> 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.
Best regards,
Christian Schoenebeck
