On Sat, 06 Jul 2019 12:22:27 +0200
Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org> wrote:
> On Mittwoch, 3. Juli 2019 13:13:26 CEST Christian Schoenebeck wrote:
> > To support multiple devices on the 9p share, and avoid
> > qid path collisions we take the device id as input
> [snip]
> > - Fixed v9fs_do_readdir() having exposed info outside
> > export root with '..' entry.
> [snip]
> > 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 /.
> Best regards,
> Christian Schoenebeck
>
