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

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

[Attila-Mihaly Balazs]

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");
             } else {
                 inet->host = g_strndup(vnc, hlen);
             }
--
2.7.4

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

[Attila-Mihaly Balazs]

Sorry, forgot to CC Gerd in my previous email.

On Mon, Jun 6, 2016 at 6:39 PM, Attila-Mihaly Balazs <dify.ltd@gmail.com> 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");
>              } else {
>                  inet->host = g_strndup(vnc, hlen);
>              }
> --
> 2.7.4

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 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 :|

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

[Attila-Mihaly Balazs]

Hello,

I think that QEMU is a great project (in fact I've written by BS
thesis about it many years back - creating a sandbox to analyze
suspicious program behavior). Sadly I don't have the bandwidth to
follow up on this any further. I'm still convinced that binding any
listening socked to localhost by default is much better since most
people don't change the defaults (and - as you pointed out - this
should be done for other services that listen on TCP not just VNC).

I also take that this is a breaking change and I don't have any great
data to substantiate that it should be a priority. My worry comes from
sites like vncroulette.com which scan the entire IPv4 address space
for open VNC servers and anecdotally what I tended to observe was that
there were many QEMU instances in these scans.

Unfortunately vncroulette.com is now defunct but here is an outline if
someone wants to investigate this:

- buy from shodan.io the list of VNC servers (it currently knows about
~330k) - it would cost arount 100USD
- use a small Perl script for example to iterate over them and detect
ones which (a) don't have any authentication and (b) run QEMU.

Ie. something like:

use Net::VNC;

my $vnc = Net::VNC->new({hostname => '127.0.0.1', port => 5901});
$vnc->login;

if ($vnc->name =~ /^QEMU/) {
  print "Vulnerable QEMU VNC server at: ..."
};

This could lead to some more quantified data which hopefully can
convince people about the importance of this and similar changes.

Kind regards!
Attila Balazs (Grey Panther)

On Tue, Jun 7, 2016 at 11:42 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Di, 2016-06-07 at 20:51 +0300, Attila-Mihaly Balazs wrote:
>> >
>> > 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.
>> >
>>
>> Daniel, thank you for taking the time to consider this patch. You
>> raised a good point and I agree that it's better to have consistency.
>> I probably could submit a patch to change the meaning of "empty host"
>> to "bind to localhost" (instead of "bind to all interfaces") globally
>> (ie. for all code which uses InetSocketAddress) - and in my mind it
>> makes good security sense to do so.
>>
>> Do you think such a change is likely to be accepted?
>
> See other mail.  Changing defaults is always problematic.
>
> And in this specific case the upper management layers (libvirt,
> virt-manager, ...) already configure things strict by default (vnc) or
> prefer to not use tcp sockets in the first place (chardevs).
>
> So I'd tend to not change the qemu default here.
>
> cheers,
>   Gerd
>