| Message ID | 149399504988.29022.13818395938763860712.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
On 05/05/2017 09:37 AM, Greg Kurz wrote: > When using the mapped-file security, credentials are stored in a metadata > directory located in the parent directory. This is okay for all paths with > the notable exception of the root path, since we don't want and probably > can't create a metadata directory above the virtfs directory on the host. > > This patch introduces a dedicated metadata file, sitting in the virtfs root > for this purpose. It relies on the fact that the "." name necessarily refer s/refer/refers/ > to the virtfs root. > > As for the metadata directory, we don't want the client to see this file. > The current code only cares for readdir() but there are many other places > to fix actually. The filtering logic is hence put in a separate function. > > Before: > > # ls -ld > drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . > # chown root.root . > chown: changing ownership of '.': Is a directory > # ls -ld > drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . > > After: > > # ls -ld > drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . > # chown bin.bin . > # ls -ld > drwxr-xr-x. 3 root root 4096 May 5 12:50 . Huh? How does chown bin.bin result in root.root? Copy-and-paste error? > > and from the host: > > ls -al .virtfs_metadata_root > -rwx------. 1 greg greg 26 May 5 12:50 .virtfs_metadata_root > $ cat .virtfs_metadata_root > virtfs.uid=0 > virtfs.gid=0 So what happens if the guest tries to manipulate ./.virtfs_metadata_root? I know you are filtering it so that readdir() and such can't see it, but do you have sane errors for the guest's attempt to circumvent those hidden names? > > Reported-by: Léo Gaspard <leo@gaspard.io> > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p-local.c | 88 ++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 61 insertions(+), 27 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
On Fri, 5 May 2017 12:11:14 -0500 Eric Blake <eblake@redhat.com> wrote: > On 05/05/2017 09:37 AM, Greg Kurz wrote: > > When using the mapped-file security, credentials are stored in a metadata > > directory located in the parent directory. This is okay for all paths with > > the notable exception of the root path, since we don't want and probably > > can't create a metadata directory above the virtfs directory on the host. > > > > This patch introduces a dedicated metadata file, sitting in the virtfs root > > for this purpose. It relies on the fact that the "." name necessarily refer > > s/refer/refers/ > I'll fix this. > > to the virtfs root. > > > > As for the metadata directory, we don't want the client to see this file. > > The current code only cares for readdir() but there are many other places > > to fix actually. The filtering logic is hence put in a separate function. > > > > Before: > > > > # ls -ld > > drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . > > # chown root.root . > > chown: changing ownership of '.': Is a directory > > # ls -ld > > drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . > > > > After: > > > > # ls -ld > > drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . > > # chown bin.bin . > > # ls -ld > > drwxr-xr-x. 3 root root 4096 May 5 12:50 . > > Huh? How does chown bin.bin result in root.root? Copy-and-paste error? > Oops, I had first tested with bin.bin, then I decided that root.root would make more sense but I fumbled when updating the changelog... :-\ > > > > and from the host: > > > > ls -al .virtfs_metadata_root > > -rwx------. 1 greg greg 26 May 5 12:50 .virtfs_metadata_root > > $ cat .virtfs_metadata_root > > virtfs.uid=0 > > virtfs.gid=0 > > So what happens if the guest tries to manipulate > ./.virtfs_metadata_root? I know you are filtering it so that readdir() > and such can't see it, but do you have sane errors for the guest's > attempt to circumvent those hidden names? > This is addressed by patch 5. > > > > Reported-by: Léo Gaspard <leo@gaspard.io> > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p-local.c | 88 ++++++++++++++++++++++++++++++++++++---------------- > > 1 file changed, 61 insertions(+), 27 deletions(-) > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index bb6e296df317..b427d2928800 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -78,6 +78,7 @@ static void unlinkat_preserve_errno(int dirfd, const char *path, int flags) } #define VIRTFS_META_DIR ".virtfs_metadata" +#define VIRTFS_META_ROOT_FILE VIRTFS_META_DIR "_root" static FILE *local_fopenat(int dirfd, const char *name, const char *mode) { @@ -114,13 +115,17 @@ static void local_mapped_file_attr(int dirfd, const char *name, char buf[ATTR_MAX]; int map_dirfd; - map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); - if (map_dirfd == -1) { - return; - } + if (strcmp(name, ".")) { + map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); + if (map_dirfd == -1) { + return; + } - fp = local_fopenat(map_dirfd, name, "r"); - close_preserve_errno(map_dirfd); + fp = local_fopenat(map_dirfd, name, "r"); + close_preserve_errno(map_dirfd); + } else { + fp = local_fopenat(dirfd, VIRTFS_META_ROOT_FILE, "r"); + } if (!fp) { return; } @@ -198,26 +203,38 @@ static int local_set_mapped_file_attrat(int dirfd, const char *name, int ret; char buf[ATTR_MAX]; int uid = -1, gid = -1, mode = -1, rdev = -1; - int map_dirfd; - - ret = mkdirat(dirfd, VIRTFS_META_DIR, 0700); - if (ret < 0 && errno != EEXIST) { - return -1; - } - - map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); - if (map_dirfd == -1) { - return -1; - } + int map_dirfd, map_fd; + bool is_root = !strcmp(name, "."); + + if (is_root) { + fp = local_fopenat(dirfd, VIRTFS_META_ROOT_FILE, "r"); + if (!fp) { + if (errno == ENOENT) { + goto update_map_file; + } else { + return -1; + } + } + } else { + ret = mkdirat(dirfd, VIRTFS_META_DIR, 0700); + if (ret < 0 && errno != EEXIST) { + return -1; + } - fp = local_fopenat(map_dirfd, name, "r"); - if (!fp) { - if (errno == ENOENT) { - goto update_map_file; - } else { - close_preserve_errno(map_dirfd); + map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); + if (map_dirfd == -1) { return -1; } + + fp = local_fopenat(map_dirfd, name, "r"); + if (!fp) { + if (errno == ENOENT) { + goto update_map_file; + } else { + close_preserve_errno(map_dirfd); + return -1; + } + } } memset(buf, 0, ATTR_MAX); while (fgets(buf, ATTR_MAX, fp)) { @@ -235,12 +252,21 @@ static int local_set_mapped_file_attrat(int dirfd, const char *name, fclose(fp); update_map_file: - fp = local_fopenat(map_dirfd, name, "w"); - close_preserve_errno(map_dirfd); + if (is_root) { + fp = local_fopenat(dirfd, VIRTFS_META_ROOT_FILE, "w"); + } else { + fp = local_fopenat(map_dirfd, name, "w"); + close_preserve_errno(map_dirfd); + } if (!fp) { return -1; } + map_fd = fileno(fp); + assert(map_fd != -1); + ret = fchmod(map_fd, 0700); + assert(ret == 0); + if (credp->fc_uid != -1) { uid = credp->fc_uid; } @@ -447,6 +473,14 @@ static off_t local_telldir(FsContext *ctx, V9fsFidOpenState *fs) return telldir(fs->dir.stream); } +static bool local_must_skip_metadata(FsContext *fs_ctx, const char *name) +{ + return + fs_ctx->export_flags & V9FS_SM_MAPPED_FILE && + (!strcmp(name, VIRTFS_META_DIR) || + !strcmp(name, VIRTFS_META_ROOT_FILE)); +} + static struct dirent *local_readdir(FsContext *ctx, V9fsFidOpenState *fs) { struct dirent *entry; @@ -460,8 +494,8 @@ again: if (ctx->export_flags & V9FS_SM_MAPPED) { entry->d_type = DT_UNKNOWN; } else if (ctx->export_flags & V9FS_SM_MAPPED_FILE) { - if (!strcmp(entry->d_name, VIRTFS_META_DIR)) { - /* skp the meta data directory */ + if (local_must_skip_metadata(ctx, entry->d_name)) { + /* skip the meta data stuff */ goto again; } entry->d_type = DT_UNKNOWN;
When using the mapped-file security, credentials are stored in a metadata directory located in the parent directory. This is okay for all paths with the notable exception of the root path, since we don't want and probably can't create a metadata directory above the virtfs directory on the host. This patch introduces a dedicated metadata file, sitting in the virtfs root for this purpose. It relies on the fact that the "." name necessarily refer to the virtfs root. As for the metadata directory, we don't want the client to see this file. The current code only cares for readdir() but there are many other places to fix actually. The filtering logic is hence put in a separate function. Before: # ls -ld drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . # chown root.root . chown: changing ownership of '.': Is a directory # ls -ld drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . After: # ls -ld drwxr-xr-x. 3 greg greg 4096 May 5 12:49 . # chown bin.bin . # ls -ld drwxr-xr-x. 3 root root 4096 May 5 12:50 . and from the host: ls -al .virtfs_metadata_root -rwx------. 1 greg greg 26 May 5 12:50 .virtfs_metadata_root $ cat .virtfs_metadata_root virtfs.uid=0 virtfs.gid=0 Reported-by: Léo Gaspard <leo@gaspard.io> Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p-local.c | 88 ++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 61 insertions(+), 27 deletions(-)