Re: [Qemu-devel] [PATCH] Bind VNC to localhost unless otherwise specified to increase security

[Daniel P. Berrange]

On Mon, Jun 06, 2016 at 06:39:15PM +0300, Attila-Mihaly Balazs wrote:
> Signed-off-by: Attila-Mihaly Balazs
> ---
>  qemu-options.hx | 7 ++++++-
>  ui/vnc.c        | 2 ++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9f33361..80ade0d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1245,7 +1245,12 @@ syntax for the @var{display} is
> 
>  TCP connections will only be allowed from @var{host} on display @var{d}.
>  By convention the TCP port is address@hidden Optionally, @var{host} can
> -be omitted in which case the server will accept connections from any host.
> +be omitted in which case the server will only accept connections from
> +localhost. To accept connections on a given network interface use the
> +syntax @var{interface IP}:@var{d} (for example @var{192.168.1.2}:@var{1}
> +or @var{[::1]}:@var{1}). To listen on all network interfaces specify
> address@hidden:@var{d}. Warning! Please make sure that you have authentication
> +set up before exposing VNC to the internet!
> 
>  @item unix:@var{path}
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index c862fdc..b4597e4 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3576,6 +3576,8 @@ void vnc_display_open(const char *id, Error **errp)
>              inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1);
>              if (vnc[0] == '[' && vnc[hlen - 1] == ']') {
>                  inet->host = g_strndup(vnc + 1, hlen - 2);
> +            } else if (hlen == 0) {
> +                inet->host = g_strdup("localhost");

I understand the reason you want to do this change, but I don't really
like the fact that this is making "empty hostname" semantics for the -vnc
option, diverge from the "empty hostname" semantics for other QEMU args
like -chardev.  The main point of having all of QEMU use the same code
for sockets listen/connect setup via the InetSocketAddress struct is that
we gain consistent semantics across the whole codebase. This change to
VNC code is throwing away that consistency, so I'm against this change
really.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|