On Freitag, 30. August 2019 13:48:27 CEST Greg Kurz wrote:
> > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > > index 8cc65c2c67..39c6c2a894 100644
> > > --- a/hw/9pfs/9p.c
> > > +++ b/hw/9pfs/9p.c
> >
> > [snip]
> >
> > > @@ -1940,6 +2041,19 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> > > *pdu, V9fsFidState *fidp, int32_t count = 0;
> > >
> > > off_t saved_dir_pos;
> > > struct dirent *dent;
> > >
> > > + struct stat stbuf;
> > > + bool fidIsExportRoot;
> > > +
> > > + /*
> > > + * determine if fidp is the export root, which is required for safe
> > > + * handling of ".." below
> > > + */
> > > + err = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
> > > + if (err < 0) {
> > > + return err;
> > > + }
> > > + fidIsExportRoot = pdu->s->dev_id == stbuf.st_dev &&
> > > + pdu->s->root_ino == stbuf.st_ino;
> > >
> > > /* save the directory position */
> > > saved_dir_pos = v9fs_co_telldir(pdu, fidp);
> > >
> > > @@ -1964,16 +2078,51 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU
> > > *pdu, V9fsFidState *fidp, v9fs_string_free(&name);
> > >
> > > return count;
> > >
> > > }
> > >
> > > - /*
> > > - * Fill up just the path field of qid because the client uses
> > > - * only that. To fill the entire qid structure we will have
> > > - * to stat each dirent found, which is expensive
> > > - */
> > > - size = MIN(sizeof(dent->d_ino), sizeof(qid.path));
> > > - memcpy(&qid.path, &dent->d_ino, size);
> > > - /* Fill the other fields with dummy values */
> > > - qid.type = 0;
> > > - qid.version = 0;
> > > +
> > > + if (fidIsExportRoot && !strcmp("..", dent->d_name)) {
> > > + /*
> > > + * if "." is export root, then return qid of export root
> > > for
> > > + * ".." to avoid exposing anything outside the export
> > > + */
> > > + err = fid_to_qid(pdu, fidp, &qid);
> > > + if (err < 0) {
> > > + v9fs_readdir_unlock(&fidp->fs.dir);
> > > + v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
> > > + v9fs_string_free(&name);
> > > + return err;
> > > + }
> >
> > Hmm, I start to wonder whether I should postpone that particular bug fix
> > and not make it part of that QID fix patch series (not even as separate
> > patch there). Because that fix needs some more adjustments. E.g. I should
> > adjust dent->d_type here as well; but more notably it should also
> > distinguish between the case where the export root is mounted as / on
> > guest or not and that's where this fix could become ugly and grow in
> > size.
> >
> > To make the case clear: calling on guest
> >
> > readdir(pathOfSome9pExportRootOnGuest);
> >
> > currently always returns for its ".." result entry the inode number and
> > d_type of the export root's parent directory on host, so it exposes
> > information of host outside the 9p export.
> >
> > I don't see that as security issue, since the information revealed is
> > limited to the inode number and d_type, but it is definitely incorrect
> > behaviour.
> Definitely. This should be fixed independently of this series. Maybe follow
> the same approach as commit 56f101ecce0e "9pfs: handle walk of ".." in the
> root directory", ie. basically make /.. an alias of /.
That's actually what the suggested fix already did in v5 here (see diff above).
However I was worried whether I thought about all edge cases. So I also need
some more testing and hence clearly decided to postpone this fix for now.
Best regards,
Christian Schoenebeck
