[Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command

[Michael Roth]

On 12/10/2010 12:43 AM, Jes Sorensen wrote:
> On 12/09/10 22:12, Michael Roth wrote:
> > On 12/07/2010 08:26 AM, Jes Sorensen wrote:
> > > I believe this suffers from the same architectural problem I mentioned
> > > in my comment to 07/21 - you don't restrict the file size, so it could
> > > blow up the QEMU process on the host trying to view the wrong file.
> >
> > It's restricted on the guest side:
> >
> > virtagent-server.c:va_getfile():
> >
> >      while ((ret = read(fd, buf, VA_FILEBUF_LEN))>  0) {
> >          file_contents = qemu_realloc(file_contents, count +
> > VA_FILEBUF_LEN);
> >          memcpy(file_contents + count, buf, ret);
> >          count += ret;
> >          if (count>  VA_GETFILE_MAX) {
> >              xmlrpc_faultf(env, "max file size (%d bytes) exceeded",
> >                            VA_GETFILE_MAX);
> >              goto EXIT_CLOSE_BAD;
> >          }
> >      }
>
> You cannot rely on the guest controlling this. You really have to treat
> any guest as hostile and keep control and security in the host,
> otherwise a hacked guest could end up attacking the host by blowing up
> the host's QEMU process.

Definetely agree on this, I mentioned some other checks here at the 
transport and host xmlrpc level that would also limit this possibility:

> There are additional limits at the transport layer well to deal with a
> potentially malicious/buggy agent as well:
>
> virtagent-common.c:va_http_read_handler():
>
>              } else if (s->content_len > VA_CONTENT_LEN_MAX) {
>                  LOG("http content length too long");
>                  goto out_bad;
>              }

I think with strictly enforced size limits the major liability for 
viewfile is, as you mentioned, users using it to view binary data or 
carefully crafted files that can mess up or fool users/shells/programs 
interpreting monitor output.

But plain-text does not include escape sequences, so it's completely 
reasonable that we'd scrape them. And I'm not sure if a "(qemu)" in the 
text is a potential liability. Would there be any other issues to consider?

If we can guard against those things, do you agree it wouldn't be an 
inherently dangerous interface? State-full, asynchronous RPCs like 
copyfile and exec are not really something I'd planned for the initial 
release. I think they'll take some time to get right, and a simple 
low-risk interface to cover what I'm fairly sure is the most common use 
case seems reasonable.

> Cheers,
> Jes