From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53401) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bADJp-0007Es-8c for qemu-devel@nongnu.org; Tue, 07 Jun 2016 05:29:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bADJl-00035q-9T for qemu-devel@nongnu.org; Tue, 07 Jun 2016 05:29:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35272) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bADJl-00035l-3f for qemu-devel@nongnu.org; Tue, 07 Jun 2016 05:29:01 -0400 Date: Tue, 7 Jun 2016 10:28:57 +0100 From: "Daniel P. Berrange" Message-ID: <20160607092857.GC20196@redhat.com> Reply-To: "Daniel P. Berrange" References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH] Bind VNC to localhost unless otherwise specified to increase security List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Attila-Mihaly Balazs Cc: qemu-devel@nongnu.org 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 5900+@var{d}. 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 > +@var{0.0.0.0}:@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 :|