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