Re: [Qemu-devel] [PATCH v2 4/4] 9pfs: local: metadata file for the VirtFS root

[Greg Kurz]

On Wed, 24 May 2017 01:08:55 +0200
Leo Gaspard  wrote:

> On 05/23/2017 07:13 PM, Eric Blake wrote:> We have to block
> VIRTFS_META_DIR at any depth in the hierarchy, but
> > can/should we change the blocking of VIRTFS_META_ROOT_FILE to only
> > happen at the root directory, rather than at all directories?  On the
> > other hand, if you can simultaneously map /path/to/a for one mount
> > point, and /path/to/a/b for another, then the root file for B is visible
> > at a lower depth than the root file for A, and blocking the metafile
> > name everywhere means that the mount A can't influence the behavior on
> > the mount for B.  
> 
> If you take this kind of vulnerabilities into account, then you also
> have to consider a mix of mapped-file and mapped-attr mounts, or even
> worse a proxy with a mapped-file mount (which I think is currently
> vulnerable to this threat if the "proxy" path points above the
> "local,security_model=mapped-file" path, as the check is done in
> "local_" functions, which are I guess not used for proxy-type virtfs)
> 
> I'm clearly not saying it's an invalid attack (there is no explicit
> documentation stating it's insecure to "nest" virtual mounts"), just
> saying it's a much larger attack surface than one internal to virtfs
> mapped-file only. Then the question of what is reasonably possible to
> forbid to the user arises, and that's not one I could answer.
> 

The attack surface is infinite. Untrusted code that could help a guest to
forge custom metadata may reside anywhere actually, not necessarily in
another QEMU-based guest... 

My motivation to hide VIRTFS_META_ROOT_FILE everywhere was more for
consistency (ie. open(VIRTFS_META_ROOT_FILE) always fails, no matter
the cwd) and for simplicity.

Cheers,

--
Greg

> Cheers & HTH,
> Leo
> 



pgpdIqlSe6iVz.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 4/4] 9pfs: local: metadata file for the VirtFS root

[Leo Gaspard]

On 05/23/2017 07:13 PM, Eric Blake wrote:> We have to block
VIRTFS_META_DIR at any depth in the hierarchy, but
> can/should we change the blocking of VIRTFS_META_ROOT_FILE to only
> happen at the root directory, rather than at all directories?  On the
> other hand, if you can simultaneously map /path/to/a for one mount
> point, and /path/to/a/b for another, then the root file for B is visible
> at a lower depth than the root file for A, and blocking the metafile
> name everywhere means that the mount A can't influence the behavior on
> the mount for B.

If you take this kind of vulnerabilities into account, then you also
have to consider a mix of mapped-file and mapped-attr mounts, or even
worse a proxy with a mapped-file mount (which I think is currently
vulnerable to this threat if the "proxy" path points above the
"local,security_model=mapped-file" path, as the check is done in
"local_" functions, which are I guess not used for proxy-type virtfs)

I'm clearly not saying it's an invalid attack (there is no explicit
documentation stating it's insecure to "nest" virtual mounts"), just
saying it's a much larger attack surface than one internal to virtfs
mapped-file only. Then the question of what is reasonably possible to
forbid to the user arises, and that's not one I could answer.

Cheers & HTH,
Leo



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 4/4] 9pfs: local: metadata file for the VirtFS root

[Greg Kurz]

On Tue, 23 May 2017 12:13:17 -0500
Eric Blake  wrote:

> On 05/23/2017 09:32 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 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.
> > 
> > @@ -478,7 +504,8 @@ static off_t local_telldir(FsContext *ctx, 
> > V9fsFidOpenState *fs)
> >  
> >  static bool local_is_mapped_file_metadata(FsContext *fs_ctx, const char 
> > *name)
> >  {
> > -return !strcmp(name, VIRTFS_META_DIR);
> > +return
> > +!strcmp(name, VIRTFS_META_DIR) || !strcmp(name, 
> > VIRTFS_META_ROOT_FILE);  
> 
> We have to block VIRTFS_META_DIR at any depth in the hierarchy, but
> can/should we change the blocking of VIRTFS_META_ROOT_FILE to only
> happen at the root directory, rather than at all directories?  On the
> other hand, if you can simultaneously map /path/to/a for one mount
> point, and /path/to/a/b for another, then the root file for B is visible
> at a lower depth than the root file for A, and blocking the metafile
> name everywhere means that the mount A can't influence the behavior on
> the mount for B.
> 

I must confess I hadn't this scenario in mind but it's safer from a
security standpoint indeed.

> Not tested, but looks sane, so:
> Reviewed-by: Eric Blake 
> 

Thanks again for the review, Eric.

Cheers,

--
Greg


pgpZnM41L6JdP.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 4/4] 9pfs: local: metadata file for the VirtFS root

[Eric Blake]

On 05/23/2017 09:32 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 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.
> 
> @@ -478,7 +504,8 @@ static off_t local_telldir(FsContext *ctx, 
> V9fsFidOpenState *fs)
>  
>  static bool local_is_mapped_file_metadata(FsContext *fs_ctx, const char 
> *name)
>  {
> -return !strcmp(name, VIRTFS_META_DIR);
> +return
> +!strcmp(name, VIRTFS_META_DIR) || !strcmp(name, 
> VIRTFS_META_ROOT_FILE);

We have to block VIRTFS_META_DIR at any depth in the hierarchy, but
can/should we change the blocking of VIRTFS_META_ROOT_FILE to only
happen at the root directory, rather than at all directories?  On the
other hand, if you can simultaneously map /path/to/a for one mount
point, and /path/to/a/b for another, then the root file for B is visible
at a lower depth than the root file for A, and blocking the metafile
name everywhere means that the mount A can't influence the behavior on
the mount for B.

Not tested, but looks sane, so:
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v2 4/4] 9pfs: local: metadata file for the VirtFS root

[Greg Kurz]

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 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 root.root .
# 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: Leo Gaspard 
Signed-off-by: Greg Kurz 
---
v2: - changed metadata file mode bits to 0600
- fixed typo in commit message
- rebased
---
 hw/9pfs/9p-local.c |   81 +++-
 1 file changed, 54 insertions(+), 27 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index b30f68f91026..8babd5542f06 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -107,6 +107,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)
 {
@@ -143,13 +144,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;
 }
@@ -227,26 +232,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)) {
@@ -264,12 +281,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, 0600);
+assert(ret == 0);
+
 if (credp->fc_uid != -1) {
 uid = credp->fc_uid;
 }
@@ -478,7 +504,8 @@ static off_t local_telldir(FsContext *ctx, V9fsFidOpenState 
*fs)
 
 static bool local_is_mapped_file_metadata(FsContext *fs_ctx, const char *name)