[Qemu-devel] [PULL for-2.3 0/9] vnc patch queue.

[Qemu-devel] [PULL for-2.3 0/9] vnc patch queue.

[Gerd Hoffmann]

  Hi,

One QMP fix from Markus, needs to go into 2.3 because it is a API fix.
A bunch of websocket fixes from Daniel, with some cleanup work.  The
problems fixed are serious enouth that I don't want delay this to 2.4
even though there are some non-trivial changes in the vnc websockets
code.

please pull,
  Gerd

The following changes since commit 5a4992834daec85c3913654903fb9f4f954e585a:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-cov-model-2015-03-17' into staging (2015-03-17 11:43:00 +0000)

are available in the git repository at:


  git://git.kraxel.org/qemu tags/pull-vnc-20150318-1

for you to fetch changes up to 4a48aaa9f52dbac148be24f591de2f28c58ccb5d:

  ui: ensure VNC websockets server checks the ACL if requested (2015-03-18 09:25:14 +0100)

----------------------------------------------------------------
vnc: fix websockets & QMP.

----------------------------------------------------------------
Daniel P. Berrange (8):
      ui: remove unused 'wiremode' variable in VncState struct
      ui: replace printf() calls with VNC_DEBUG
      ui: report error if user requests VNC option that is unsupported
      ui: split setup of VNC auth scheme into separate method
      ui: fix setup of VNC websockets auth scheme with TLS
      ui: enforce TLS when using websockets server
      ui: remove separate gnutls_session for websockets server
      ui: ensure VNC websockets server checks the ACL if requested

Markus Armbruster (1):
      vnc: Fix QMP change not to use funky error class

 ui/vnc-auth-vencrypt.c |   1 -
 ui/vnc-tls.c           |  72 +++++-------
 ui/vnc-tls.h           |   7 --
 ui/vnc-ws.c            |  46 ++++----
 ui/vnc-ws.h            |   2 +-
 ui/vnc.c               | 289 +++++++++++++++++++++++++++++--------------------
 ui/vnc.h               |   9 +-
 7 files changed, 223 insertions(+), 203 deletions(-)

[Qemu-devel] [PULL 1/9] vnc: Fix QMP change not to use funky error class

[Gerd Hoffmann]

From: Markus Armbruster <armbru@redhat.com>

Error classes are a leftover from the days of "rich" error objects.
New code should always use ERROR_CLASS_GENERIC_ERROR.  Commit 1d0d59f
added a use of ERROR_CLASS_DEVICE_NOT_FOUND.  Replace it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 6f9b718..a950016 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3594,7 +3594,7 @@ void vnc_display_open(const char *id, Error **errp)
 
         dev = qdev_find_recursive(sysbus_get_default(), device_id);
         if (dev == NULL) {
-            error_set(errp, QERR_DEVICE_NOT_FOUND, device_id);
+            error_setg(errp, "Device '%s' not found", device_id);
             goto fail;
         }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 2/9] ui: remove unused 'wiremode' variable in VncState struct

[Gerd Hoffmann]

From: "Daniel P. Berrange" <berrange@redhat.com>

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc-auth-vencrypt.c | 1 -
 ui/vnc-tls.c           | 2 --
 ui/vnc-tls.h           | 7 -------
 ui/vnc-ws.c            | 1 -
 4 files changed, 11 deletions(-)

diff --git a/ui/vnc-auth-vencrypt.c b/ui/vnc-auth-vencrypt.c
index bc7032e..a420ccb 100644
--- a/ui/vnc-auth-vencrypt.c
+++ b/ui/vnc-auth-vencrypt.c
@@ -93,7 +93,6 @@ static int vnc_start_vencrypt_handshake(struct VncState *vs) {
     }
 
     VNC_DEBUG("Handshake done, switching to TLS data mode\n");
-    vs->tls.wiremode = VNC_WIREMODE_TLS;
     qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, vnc_client_write, vs);
 
     start_auth_vencrypt_subauth(vs);
diff --git a/ui/vnc-tls.c b/ui/vnc-tls.c
index 0f59f9b..de1cb34 100644
--- a/ui/vnc-tls.c
+++ b/ui/vnc-tls.c
@@ -421,14 +421,12 @@ void vnc_tls_client_cleanup(struct VncState *vs)
         gnutls_deinit(vs->tls.session);
         vs->tls.session = NULL;
     }
-    vs->tls.wiremode = VNC_WIREMODE_CLEAR;
     g_free(vs->tls.dname);
 #ifdef CONFIG_VNC_WS
     if (vs->ws_tls.session) {
         gnutls_deinit(vs->ws_tls.session);
         vs->ws_tls.session = NULL;
     }
-    vs->ws_tls.wiremode = VNC_WIREMODE_CLEAR;
     g_free(vs->ws_tls.dname);
 #endif /* CONFIG_VNC_WS */
 }
diff --git a/ui/vnc-tls.h b/ui/vnc-tls.h
index 36a2227..f9829c7 100644
--- a/ui/vnc-tls.h
+++ b/ui/vnc-tls.h
@@ -33,11 +33,6 @@
 
 #include "qemu/acl.h"
 
-enum {
-    VNC_WIREMODE_CLEAR,
-    VNC_WIREMODE_TLS,
-};
-
 typedef struct VncDisplayTLS VncDisplayTLS;
 typedef struct VncStateTLS VncStateTLS;
 
@@ -55,8 +50,6 @@ struct VncDisplayTLS {
 
 /* Per client state */
 struct VncStateTLS {
-    /* Whether data is being TLS encrypted yet */
-    int wiremode;
     gnutls_session_t session;
 
     /* Client's Distinguished Name from the x509 cert */
diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index d75950d..1769d52 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -48,7 +48,6 @@ static int vncws_start_tls_handshake(struct VncState *vs)
     }
 
     VNC_DEBUG("Handshake done, switching to TLS data mode\n");
-    vs->ws_tls.wiremode = VNC_WIREMODE_TLS;
     qemu_set_fd_handler2(vs->csock, NULL, vncws_handshake_read, NULL, vs);
 
     return 0;
-- 
1.8.3.1

[Qemu-devel] [PULL 3/9] ui: replace printf() calls with VNC_DEBUG

[Gerd Hoffmann]

From: "Daniel P. Berrange" <berrange@redhat.com>

Handling of VNC audio messages results in printfs to the console.
This is of no use to anyone in production, so should be using the
normal VNC_DEBUG macro instead.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index a950016..6f0b0ce 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2400,34 +2400,34 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
                 case 4: vs->as.fmt = AUD_FMT_U32; break;
                 case 5: vs->as.fmt = AUD_FMT_S32; break;
                 default:
-                    printf("Invalid audio format %d\n", read_u8(data, 4));
+                    VNC_DEBUG("Invalid audio format %d\n", read_u8(data, 4));
                     vnc_client_error(vs);
                     break;
                 }
                 vs->as.nchannels = read_u8(data, 5);
                 if (vs->as.nchannels != 1 && vs->as.nchannels != 2) {
-                    printf("Invalid audio channel coount %d\n",
-                           read_u8(data, 5));
+                    VNC_DEBUG("Invalid audio channel coount %d\n",
+                              read_u8(data, 5));
                     vnc_client_error(vs);
                     break;
                 }
                 vs->as.freq = read_u32(data, 6);
                 break;
             default:
-                printf ("Invalid audio message %d\n", read_u8(data, 4));
+                VNC_DEBUG("Invalid audio message %d\n", read_u8(data, 4));
                 vnc_client_error(vs);
                 break;
             }
             break;
 
         default:
-            printf("Msg: %d\n", read_u16(data, 0));
+            VNC_DEBUG("Msg: %d\n", read_u16(data, 0));
             vnc_client_error(vs);
             break;
         }
         break;
     default:
-        printf("Msg: %d\n", data[0]);
+        VNC_DEBUG("Msg: %d\n", data[0]);
         vnc_client_error(vs);
         break;
     }
-- 
1.8.3.1

[Qemu-devel] [PULL 4/9] ui: report error if user requests VNC option that is unsupported

[Gerd Hoffmann]

From: "Daniel P. Berrange" <berrange@redhat.com>

If the VNC server is built without tls, sasl or websocket support
and the user requests one of these features, they are just silently
ignored. This is bad because it means the VNC server ends up running
in a configuration that is less secure than the user asked for.
It also leads to an tangled mass of preprocessor conditionals when
configuring the VNC server.

This ensures that the tls, sasl & websocket options are always
processed and an error is reported back to the user if any of
them were disabled at build time.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 51 +++++++++++++++++++++------------------------------
 ui/vnc.h |  4 ++--
 2 files changed, 23 insertions(+), 32 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 6f0b0ce..d5e6024 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3010,14 +3010,10 @@ static void vnc_connect(VncDisplay *vd, int csock,
 
     if (skipauth) {
 	vs->auth = VNC_AUTH_NONE;
-#ifdef CONFIG_VNC_TLS
 	vs->subauth = VNC_AUTH_INVALID;
-#endif
     } else {
 	vs->auth = vd->auth;
-#ifdef CONFIG_VNC_TLS
 	vs->subauth = vd->subauth;
-#endif
     }
 
     vs->lossy_rect = g_malloc0(VNC_STAT_ROWS * sizeof (*vs->lossy_rect));
@@ -3206,8 +3202,8 @@ static void vnc_display_close(VncDisplay *vs)
     }
 #endif /* CONFIG_VNC_WS */
     vs->auth = VNC_AUTH_INVALID;
-#ifdef CONFIG_VNC_TLS
     vs->subauth = VNC_AUTH_INVALID;
+#ifdef CONFIG_VNC_TLS
     vs->tls.x509verify = 0;
 #endif
 }
@@ -3332,15 +3328,13 @@ void vnc_display_open(const char *id, Error **errp)
     char *h;
     bool has_ipv4 = false;
     bool has_ipv6 = false;
-#ifdef CONFIG_VNC_WS
     const char *websocket;
-#endif
-#ifdef CONFIG_VNC_TLS
     bool tls = false, x509 = false;
+#ifdef CONFIG_VNC_TLS
     const char *path;
 #endif
-#ifdef CONFIG_VNC_SASL
     bool sasl = false;
+#ifdef CONFIG_VNC_SASL
     int saslErr;
 #endif
 #if defined(CONFIG_VNC_TLS) || defined(CONFIG_VNC_SASL)
@@ -3404,11 +3398,15 @@ void vnc_display_open(const char *id, Error **errp)
 
     reverse = qemu_opt_get_bool(opts, "reverse", false);
     lock_key_sync = qemu_opt_get_bool(opts, "lock-key-sync", true);
-#ifdef CONFIG_VNC_SASL
     sasl = qemu_opt_get_bool(opts, "sasl", false);
-#endif
-#ifdef CONFIG_VNC_TLS
+#ifndef CONFIG_VNC_SASL
+    if (sasl) {
+        error_setg(errp, "VNC SASL auth requires cyrus-sasl support");
+        goto fail;
+    }
+#endif /* CONFIG_VNC_SASL */
     tls  = qemu_opt_get_bool(opts, "tls", false);
+#ifdef CONFIG_VNC_TLS
     path = qemu_opt_get(opts, "x509");
     if (!path) {
         path = qemu_opt_get(opts, "x509verify");
@@ -3424,7 +3422,12 @@ void vnc_display_open(const char *id, Error **errp)
             goto fail;
         }
     }
-#endif
+#else /* ! CONFIG_VNC_TLS */
+    if (tls) {
+        error_setg(errp, "VNC TLS auth requires gnutls support");
+        goto fail;
+    }
+#endif /* ! CONFIG_VNC_TLS */
 #if defined(CONFIG_VNC_TLS) || defined(CONFIG_VNC_SASL)
     acl = qemu_opt_get_bool(opts, "acl", false);
 #endif
@@ -3446,14 +3449,16 @@ void vnc_display_open(const char *id, Error **errp)
     }
     vs->connections_limit = qemu_opt_get_number(opts, "connections", 32);
 
- #ifdef CONFIG_VNC_WS
     websocket = qemu_opt_get(opts, "websocket");
     if (websocket) {
+#ifdef CONFIG_VNC_WS
         vs->ws_enabled = true;
         qemu_opt_set(wsopts, "port", websocket, &error_abort);
-
+#else /* ! CONFIG_VNC_WS */
+        error_setg(errp, "Websockets protocol requires gnutls support");
+        goto fail;
+#endif /* ! CONFIG_VNC_WS */
     }
-#endif /* CONFIG_VNC_WS */
 
 #ifdef CONFIG_VNC_JPEG
     vs->lossy = qemu_opt_get_bool(opts, "lossy", false);
@@ -3518,7 +3523,6 @@ void vnc_display_open(const char *id, Error **errp)
      * NB2. the x509 schemes have option to validate a client cert dname
      */
     if (password) {
-#ifdef CONFIG_VNC_TLS
         if (tls) {
             vs->auth = VNC_AUTH_VENCRYPT;
             if (x509) {
@@ -3529,16 +3533,11 @@ void vnc_display_open(const char *id, Error **errp)
                 vs->subauth = VNC_AUTH_VENCRYPT_TLSVNC;
             }
         } else {
-#endif /* CONFIG_VNC_TLS */
             VNC_DEBUG("Initializing VNC server with password auth\n");
             vs->auth = VNC_AUTH_VNC;
-#ifdef CONFIG_VNC_TLS
             vs->subauth = VNC_AUTH_INVALID;
         }
-#endif /* CONFIG_VNC_TLS */
-#ifdef CONFIG_VNC_SASL
     } else if (sasl) {
-#ifdef CONFIG_VNC_TLS
         if (tls) {
             vs->auth = VNC_AUTH_VENCRYPT;
             if (x509) {
@@ -3549,16 +3548,11 @@ void vnc_display_open(const char *id, Error **errp)
                 vs->subauth = VNC_AUTH_VENCRYPT_TLSSASL;
             }
         } else {
-#endif /* CONFIG_VNC_TLS */
             VNC_DEBUG("Initializing VNC server with SASL auth\n");
             vs->auth = VNC_AUTH_SASL;
-#ifdef CONFIG_VNC_TLS
             vs->subauth = VNC_AUTH_INVALID;
         }
-#endif /* CONFIG_VNC_TLS */
-#endif /* CONFIG_VNC_SASL */
     } else {
-#ifdef CONFIG_VNC_TLS
         if (tls) {
             vs->auth = VNC_AUTH_VENCRYPT;
             if (x509) {
@@ -3569,13 +3563,10 @@ void vnc_display_open(const char *id, Error **errp)
                 vs->subauth = VNC_AUTH_VENCRYPT_TLSNONE;
             }
         } else {
-#endif
             VNC_DEBUG("Initializing VNC server with no auth\n");
             vs->auth = VNC_AUTH_NONE;
-#ifdef CONFIG_VNC_TLS
             vs->subauth = VNC_AUTH_INVALID;
         }
-#endif
     }
 
 #ifdef CONFIG_VNC_SASL
diff --git a/ui/vnc.h b/ui/vnc.h
index 66a0298..90b2592 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -180,10 +180,10 @@ struct VncDisplay
     char *password;
     time_t expires;
     int auth;
+    int subauth; /* Used by VeNCrypt */
     bool lossy;
     bool non_adaptive;
 #ifdef CONFIG_VNC_TLS
-    int subauth; /* Used by VeNCrypt */
     VncDisplayTLS tls;
 #endif
 #ifdef CONFIG_VNC_SASL
@@ -284,9 +284,9 @@ struct VncState
     int minor;
 
     int auth;
+    int subauth; /* Used by VeNCrypt */
     char challenge[VNC_AUTH_CHALLENGE_SIZE];
 #ifdef CONFIG_VNC_TLS
-    int subauth; /* Used by VeNCrypt */
     VncStateTLS tls;
 #endif
 #ifdef CONFIG_VNC_SASL
-- 
1.8.3.1

[Qemu-devel] [PULL 5/9] ui: split setup of VNC auth scheme into separate method

[Gerd Hoffmann]

From: "Daniel P. Berrange" <berrange@redhat.com>

The vnc_display_open method is quite long and complex, so
move the VNC auth scheme decision logic into a separate
method for clarity.

Also update the comment to better describe what we are
trying to achieve.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 153 +++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 91 insertions(+), 62 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index d5e6024..8edbb67 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3314,6 +3314,96 @@ static QemuOptsList qemu_vnc_opts = {
     },
 };
 
+
+static void
+vnc_display_setup_auth(VncDisplay *vs,
+                       bool password,
+                       bool sasl,
+                       bool tls,
+                       bool x509)
+{
+    /*
+     * We have a choice of 3 authentication options
+     *
+     *   1. none
+     *   2. vnc
+     *   3. sasl
+     *
+     * The channel can be run in 2 modes
+     *
+     *   1. clear
+     *   2. tls
+     *
+     * And TLS can use 2 types of credentials
+     *
+     *   1. anon
+     *   2. x509
+     *
+     * We thus have 9 possible logical combinations
+     *
+     *   1. clear + none
+     *   2. clear + vnc
+     *   3. clear + sasl
+     *   4. tls + anon + none
+     *   5. tls + anon + vnc
+     *   6. tls + anon + sasl
+     *   7. tls + x509 + none
+     *   8. tls + x509 + vnc
+     *   9. tls + x509 + sasl
+     *
+     * These need to be mapped into the VNC auth schemes
+     * in an appropriate manner. In regular VNC, all the
+     * TLS options get mapped into VNC_AUTH_VENCRYPT
+     * sub-auth types.
+     */
+    if (password) {
+        if (tls) {
+            vs->auth = VNC_AUTH_VENCRYPT;
+            if (x509) {
+                VNC_DEBUG("Initializing VNC server with x509 password auth\n");
+                vs->subauth = VNC_AUTH_VENCRYPT_X509VNC;
+            } else {
+                VNC_DEBUG("Initializing VNC server with TLS password auth\n");
+                vs->subauth = VNC_AUTH_VENCRYPT_TLSVNC;
+            }
+        } else {
+            VNC_DEBUG("Initializing VNC server with password auth\n");
+            vs->auth = VNC_AUTH_VNC;
+            vs->subauth = VNC_AUTH_INVALID;
+        }
+    } else if (sasl) {
+        if (tls) {
+            vs->auth = VNC_AUTH_VENCRYPT;
+            if (x509) {
+                VNC_DEBUG("Initializing VNC server with x509 SASL auth\n");
+                vs->subauth = VNC_AUTH_VENCRYPT_X509SASL;
+            } else {
+                VNC_DEBUG("Initializing VNC server with TLS SASL auth\n");
+                vs->subauth = VNC_AUTH_VENCRYPT_TLSSASL;
+            }
+        } else {
+            VNC_DEBUG("Initializing VNC server with SASL auth\n");
+            vs->auth = VNC_AUTH_SASL;
+            vs->subauth = VNC_AUTH_INVALID;
+        }
+    } else {
+        if (tls) {
+            vs->auth = VNC_AUTH_VENCRYPT;
+            if (x509) {
+                VNC_DEBUG("Initializing VNC server with x509 no auth\n");
+                vs->subauth = VNC_AUTH_VENCRYPT_X509NONE;
+            } else {
+                VNC_DEBUG("Initializing VNC server with TLS no auth\n");
+                vs->subauth = VNC_AUTH_VENCRYPT_TLSNONE;
+            }
+        } else {
+            VNC_DEBUG("Initializing VNC server with no auth\n");
+            vs->auth = VNC_AUTH_NONE;
+            vs->subauth = VNC_AUTH_INVALID;
+        }
+    }
+}
+
 void vnc_display_open(const char *id, Error **errp)
 {
     VncDisplay *vs = vnc_display_find(id);
@@ -3506,68 +3596,7 @@ void vnc_display_open(const char *id, Error **errp)
     }
 #endif
 
-    /*
-     * Combinations we support here:
-     *
-     *  - no-auth                (clear text, no auth)
-     *  - password               (clear text, weak auth)
-     *  - sasl                   (encrypt, good auth *IF* using Kerberos via GSSAPI)
-     *  - tls                    (encrypt, weak anonymous creds, no auth)
-     *  - tls + password         (encrypt, weak anonymous creds, weak auth)
-     *  - tls + sasl             (encrypt, weak anonymous creds, good auth)
-     *  - tls + x509             (encrypt, good x509 creds, no auth)
-     *  - tls + x509 + password  (encrypt, good x509 creds, weak auth)
-     *  - tls + x509 + sasl      (encrypt, good x509 creds, good auth)
-     *
-     * NB1. TLS is a stackable auth scheme.
-     * NB2. the x509 schemes have option to validate a client cert dname
-     */
-    if (password) {
-        if (tls) {
-            vs->auth = VNC_AUTH_VENCRYPT;
-            if (x509) {
-                VNC_DEBUG("Initializing VNC server with x509 password auth\n");
-                vs->subauth = VNC_AUTH_VENCRYPT_X509VNC;
-            } else {
-                VNC_DEBUG("Initializing VNC server with TLS password auth\n");
-                vs->subauth = VNC_AUTH_VENCRYPT_TLSVNC;
-            }
-        } else {
-            VNC_DEBUG("Initializing VNC server with password auth\n");
-            vs->auth = VNC_AUTH_VNC;
-            vs->subauth = VNC_AUTH_INVALID;
-        }
-    } else if (sasl) {
-        if (tls) {
-            vs->auth = VNC_AUTH_VENCRYPT;
-            if (x509) {
-                VNC_DEBUG("Initializing VNC server with x509 SASL auth\n");
-                vs->subauth = VNC_AUTH_VENCRYPT_X509SASL;
-            } else {
-                VNC_DEBUG("Initializing VNC server with TLS SASL auth\n");
-                vs->subauth = VNC_AUTH_VENCRYPT_TLSSASL;
-            }
-        } else {
-            VNC_DEBUG("Initializing VNC server with SASL auth\n");
-            vs->auth = VNC_AUTH_SASL;
-            vs->subauth = VNC_AUTH_INVALID;
-        }
-    } else {
-        if (tls) {
-            vs->auth = VNC_AUTH_VENCRYPT;
-            if (x509) {
-                VNC_DEBUG("Initializing VNC server with x509 no auth\n");
-                vs->subauth = VNC_AUTH_VENCRYPT_X509NONE;
-            } else {
-                VNC_DEBUG("Initializing VNC server with TLS no auth\n");
-                vs->subauth = VNC_AUTH_VENCRYPT_TLSNONE;
-            }
-        } else {
-            VNC_DEBUG("Initializing VNC server with no auth\n");
-            vs->auth = VNC_AUTH_NONE;
-            vs->subauth = VNC_AUTH_INVALID;
-        }
-    }
+    vnc_display_setup_auth(vs, password, sasl, tls, x509);
 
 #ifdef CONFIG_VNC_SASL
     if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
-- 
1.8.3.1

[Qemu-devel] [PULL 6/9] ui: fix setup of VNC websockets auth scheme with TLS

[Gerd Hoffmann]

From: "Daniel P. Berrange" <berrange@redhat.com>

The way the websockets TLS code was integrated into the VNC server
made it essentially useless. The only time that the websockets TLS
support could be used is if the primary VNC server had its existing
TLS support disabled. ie QEMU had to be launched with:

  # qemu -vnc localhost:1,websockets=5902,x509=/path/to/certs

Note the absence of the 'tls' flag. This is already a bug, because
the docs indicate that 'x509' is ignored unless 'tls' is given.

If the primary VNC server had TLS turned on via the 'tls' flag,
then this prevented the websockets TLS support from being used,
because it activates the VeNCrypt auth which would have resulted
in TLS being run over a TLS session. Of course no websockets VNC
client supported VeNCrypt so in practice, since the browser clients
cannot setup a nested TLS session over the main HTTPS connection,
so it would not even get past auth.

This patch causes us to decide our auth scheme separately for the
main VNC server vs the websockets VNC server. We take account of
the fact that if TLS is enabled, then the websockets client will
use https, so setting up VeNCrypt is thus redundant as it would
lead to nested TLS sessions.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 ui/vnc.h |  2 ++
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 8edbb67..7b66f93 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3012,9 +3012,16 @@ static void vnc_connect(VncDisplay *vd, int csock,
 	vs->auth = VNC_AUTH_NONE;
 	vs->subauth = VNC_AUTH_INVALID;
     } else {
-	vs->auth = vd->auth;
-	vs->subauth = vd->subauth;
+        if (websocket) {
+            vs->auth = vd->ws_auth;
+            vs->subauth = VNC_AUTH_INVALID;
+        } else {
+            vs->auth = vd->auth;
+            vs->subauth = vd->subauth;
+        }
     }
+    VNC_DEBUG("Client sock=%d ws=%d auth=%d subauth=%d\n",
+              csock, websocket, vs->auth, vs->subauth);
 
     vs->lossy_rect = g_malloc0(VNC_STAT_ROWS * sizeof (*vs->lossy_rect));
     for (i = 0; i < VNC_STAT_ROWS; ++i) {
@@ -3028,7 +3035,7 @@ static void vnc_connect(VncDisplay *vd, int csock,
     if (websocket) {
         vs->websocket = 1;
 #ifdef CONFIG_VNC_TLS
-        if (vd->tls.x509cert) {
+        if (vd->ws_tls) {
             qemu_set_fd_handler2(vs->csock, NULL, vncws_tls_handshake_peek,
                                  NULL, vs);
         } else
@@ -3320,7 +3327,8 @@ vnc_display_setup_auth(VncDisplay *vs,
                        bool password,
                        bool sasl,
                        bool tls,
-                       bool x509)
+                       bool x509,
+                       bool websocket)
 {
     /*
      * We have a choice of 3 authentication options
@@ -3355,10 +3363,26 @@ vnc_display_setup_auth(VncDisplay *vs,
      * in an appropriate manner. In regular VNC, all the
      * TLS options get mapped into VNC_AUTH_VENCRYPT
      * sub-auth types.
+     *
+     * In websockets, the https:// protocol already provides
+     * TLS support, so there is no need to make use of the
+     * VeNCrypt extension. Furthermore, websockets browser
+     * clients could not use VeNCrypt even if they wanted to,
+     * as they cannot control when the TLS handshake takes
+     * place. Thus there is no option but to rely on https://,
+     * meaning combinations 4->6 and 7->9 will be mapped to
+     * VNC auth schemes in the same way as combos 1->3.
+     *
+     * Regardless of fact that we have a different mapping to
+     * VNC auth mechs for plain VNC vs websockets VNC, the end
+     * result has the same security characteristics.
      */
     if (password) {
         if (tls) {
             vs->auth = VNC_AUTH_VENCRYPT;
+            if (websocket) {
+                vs->ws_tls = true;
+            }
             if (x509) {
                 VNC_DEBUG("Initializing VNC server with x509 password auth\n");
                 vs->subauth = VNC_AUTH_VENCRYPT_X509VNC;
@@ -3371,9 +3395,17 @@ vnc_display_setup_auth(VncDisplay *vs,
             vs->auth = VNC_AUTH_VNC;
             vs->subauth = VNC_AUTH_INVALID;
         }
+        if (websocket) {
+            vs->ws_auth = VNC_AUTH_VNC;
+        } else {
+            vs->ws_auth = VNC_AUTH_INVALID;
+        }
     } else if (sasl) {
         if (tls) {
             vs->auth = VNC_AUTH_VENCRYPT;
+            if (websocket) {
+                vs->ws_tls = true;
+            }
             if (x509) {
                 VNC_DEBUG("Initializing VNC server with x509 SASL auth\n");
                 vs->subauth = VNC_AUTH_VENCRYPT_X509SASL;
@@ -3386,9 +3418,17 @@ vnc_display_setup_auth(VncDisplay *vs,
             vs->auth = VNC_AUTH_SASL;
             vs->subauth = VNC_AUTH_INVALID;
         }
+        if (websocket) {
+            vs->ws_auth = VNC_AUTH_SASL;
+        } else {
+            vs->ws_auth = VNC_AUTH_INVALID;
+        }
     } else {
         if (tls) {
             vs->auth = VNC_AUTH_VENCRYPT;
+            if (websocket) {
+                vs->ws_tls = true;
+            }
             if (x509) {
                 VNC_DEBUG("Initializing VNC server with x509 no auth\n");
                 vs->subauth = VNC_AUTH_VENCRYPT_X509NONE;
@@ -3401,6 +3441,11 @@ vnc_display_setup_auth(VncDisplay *vs,
             vs->auth = VNC_AUTH_NONE;
             vs->subauth = VNC_AUTH_INVALID;
         }
+        if (websocket) {
+            vs->ws_auth = VNC_AUTH_NONE;
+        } else {
+            vs->ws_auth = VNC_AUTH_INVALID;
+        }
     }
 }
 
@@ -3596,7 +3641,7 @@ void vnc_display_open(const char *id, Error **errp)
     }
 #endif
 
-    vnc_display_setup_auth(vs, password, sasl, tls, x509);
+    vnc_display_setup_auth(vs, password, sasl, tls, x509, websocket);
 
 #ifdef CONFIG_VNC_SASL
     if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
diff --git a/ui/vnc.h b/ui/vnc.h
index 90b2592..aac9156 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -181,6 +181,8 @@ struct VncDisplay
     time_t expires;
     int auth;
     int subauth; /* Used by VeNCrypt */
+    int ws_auth; /* Used by websockets */
+    bool ws_tls; /* Used by websockets */
     bool lossy;
     bool non_adaptive;
 #ifdef CONFIG_VNC_TLS
-- 
1.8.3.1

[Qemu-devel] [PULL 7/9] ui: enforce TLS when using websockets server

[Gerd Hoffmann]

From: "Daniel P. Berrange" <berrange@redhat.com>

When TLS is required, the primary VNC server considers it to be
mandatory. ie the server admin decides whether or not TLS is used,
and the client has to comply with this decision. The websockets
server, however, treated it as optional, allowing non-TLS clients
to connect to a server which had setup TLS. Thus enabling websockets
lowers the security of the VNC server leaving the admin no way to
enforce use of TLS.

This removes the code that allows non-TLS fallback in the websockets
server, so that if TLS is requested for VNC it is now mandatory for
both the primary VNC server and the websockets VNC server.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc-ws.c | 31 +++++++------------------------
 ui/vnc-ws.h |  2 +-
 ui/vnc.c    |  2 +-
 3 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index 1769d52..0fcce4e 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -24,8 +24,6 @@
 #ifdef CONFIG_VNC_TLS
 #include "qemu/sockets.h"
 
-static void vncws_tls_handshake_io(void *opaque);
-
 static int vncws_start_tls_handshake(struct VncState *vs)
 {
     int ret = gnutls_handshake(vs->ws_tls.session);
@@ -53,33 +51,18 @@ static int vncws_start_tls_handshake(struct VncState *vs)
     return 0;
 }
 
-static void vncws_tls_handshake_io(void *opaque)
+void vncws_tls_handshake_io(void *opaque)
 {
     struct VncState *vs = (struct VncState *)opaque;
 
-    VNC_DEBUG("Handshake IO continue\n");
-    vncws_start_tls_handshake(vs);
-}
-
-void vncws_tls_handshake_peek(void *opaque)
-{
-    VncState *vs = opaque;
-    long ret;
-
-    if (!vs->ws_tls.session) {
-        char peek[4];
-        ret = qemu_recv(vs->csock, peek, sizeof(peek), MSG_PEEK);
-        if (ret && (strncmp(peek, "\x16", 1) == 0
-                    || strncmp(peek, "\x80", 1) == 0)) {
-            VNC_DEBUG("TLS Websocket connection recognized");
-            vnc_tls_client_setup(vs, 1);
-            vncws_start_tls_handshake(vs);
-        } else {
-            vncws_handshake_read(vs);
+    if (!vs->tls.session) {
+        VNC_DEBUG("TLS Websocket setup\n");
+        if (vnc_tls_client_setup(vs, vs->vd->tls.x509cert != NULL) < 0) {
+            return;
         }
-    } else {
-        qemu_set_fd_handler2(vs->csock, NULL, vncws_handshake_read, NULL, vs);
     }
+    VNC_DEBUG("Handshake IO continue\n");
+    vncws_start_tls_handshake(vs);
 }
 #endif /* CONFIG_VNC_TLS */
 
diff --git a/ui/vnc-ws.h b/ui/vnc-ws.h
index 95c1b0a..ef229b7 100644
--- a/ui/vnc-ws.h
+++ b/ui/vnc-ws.h
@@ -75,7 +75,7 @@ enum {
 };
 
 #ifdef CONFIG_VNC_TLS
-void vncws_tls_handshake_peek(void *opaque);
+void vncws_tls_handshake_io(void *opaque);
 #endif /* CONFIG_VNC_TLS */
 void vncws_handshake_read(void *opaque);
 long vnc_client_write_ws(VncState *vs);
diff --git a/ui/vnc.c b/ui/vnc.c
index 7b66f93..9994de1 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3036,7 +3036,7 @@ static void vnc_connect(VncDisplay *vd, int csock,
         vs->websocket = 1;
 #ifdef CONFIG_VNC_TLS
         if (vd->ws_tls) {
-            qemu_set_fd_handler2(vs->csock, NULL, vncws_tls_handshake_peek,
+            qemu_set_fd_handler2(vs->csock, NULL, vncws_tls_handshake_io,
                                  NULL, vs);
         } else
 #endif /* CONFIG_VNC_TLS */
-- 
1.8.3.1

[Qemu-devel] [PULL 8/9] ui: remove separate gnutls_session for websockets server

[Gerd Hoffmann]

From: "Daniel P. Berrange" <berrange@redhat.com>

The previous change to the auth scheme handling guarantees we
can never have nested TLS sessions in the VNC websockets server.
Thus we can remove the separate gnutls_session instance.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc-tls.c | 70 +++++++++++++++++++++++++-----------------------------------
 ui/vnc-ws.c  |  4 ++--
 ui/vnc.c     | 18 ++--------------
 ui/vnc.h     |  3 ---
 4 files changed, 33 insertions(+), 62 deletions(-)

diff --git a/ui/vnc-tls.c b/ui/vnc-tls.c
index de1cb34..eddd39b 100644
--- a/ui/vnc-tls.c
+++ b/ui/vnc-tls.c
@@ -334,82 +334,77 @@ static int vnc_set_gnutls_priority(gnutls_session_t s, int x509)
 
 int vnc_tls_client_setup(struct VncState *vs,
                          int needX509Creds) {
-    VncStateTLS *tls;
-
     VNC_DEBUG("Do TLS setup\n");
-#ifdef CONFIG_VNC_WS
-    if (vs->websocket) {
-        tls = &vs->ws_tls;
-    } else
-#endif /* CONFIG_VNC_WS */
-    {
-        tls = &vs->tls;
-    }
     if (vnc_tls_initialize() < 0) {
         VNC_DEBUG("Failed to init TLS\n");
         vnc_client_error(vs);
         return -1;
     }
-    if (tls->session == NULL) {
-        if (gnutls_init(&tls->session, GNUTLS_SERVER) < 0) {
+    if (vs->tls.session == NULL) {
+        if (gnutls_init(&vs->tls.session, GNUTLS_SERVER) < 0) {
             vnc_client_error(vs);
             return -1;
         }
 
-        if (gnutls_set_default_priority(tls->session) < 0) {
-            gnutls_deinit(tls->session);
-            tls->session = NULL;
+        if (gnutls_set_default_priority(vs->tls.session) < 0) {
+            gnutls_deinit(vs->tls.session);
+            vs->tls.session = NULL;
             vnc_client_error(vs);
             return -1;
         }
 
-        if (vnc_set_gnutls_priority(tls->session, needX509Creds) < 0) {
-            gnutls_deinit(tls->session);
-            tls->session = NULL;
+        if (vnc_set_gnutls_priority(vs->tls.session, needX509Creds) < 0) {
+            gnutls_deinit(vs->tls.session);
+            vs->tls.session = NULL;
             vnc_client_error(vs);
             return -1;
         }
 
         if (needX509Creds) {
-            gnutls_certificate_server_credentials x509_cred = vnc_tls_initialize_x509_cred(vs->vd);
+            gnutls_certificate_server_credentials x509_cred =
+                vnc_tls_initialize_x509_cred(vs->vd);
             if (!x509_cred) {
-                gnutls_deinit(tls->session);
-                tls->session = NULL;
+                gnutls_deinit(vs->tls.session);
+                vs->tls.session = NULL;
                 vnc_client_error(vs);
                 return -1;
             }
-            if (gnutls_credentials_set(tls->session, GNUTLS_CRD_CERTIFICATE, x509_cred) < 0) {
-                gnutls_deinit(tls->session);
-                tls->session = NULL;
+            if (gnutls_credentials_set(vs->tls.session,
+                                       GNUTLS_CRD_CERTIFICATE, x509_cred) < 0) {
+                gnutls_deinit(vs->tls.session);
+                vs->tls.session = NULL;
                 gnutls_certificate_free_credentials(x509_cred);
                 vnc_client_error(vs);
                 return -1;
             }
             if (vs->vd->tls.x509verify) {
                 VNC_DEBUG("Requesting a client certificate\n");
-                gnutls_certificate_server_set_request (tls->session, GNUTLS_CERT_REQUEST);
+                gnutls_certificate_server_set_request(vs->tls.session,
+                                                      GNUTLS_CERT_REQUEST);
             }
 
         } else {
-            gnutls_anon_server_credentials_t anon_cred = vnc_tls_initialize_anon_cred();
+            gnutls_anon_server_credentials_t anon_cred =
+                vnc_tls_initialize_anon_cred();
             if (!anon_cred) {
-                gnutls_deinit(tls->session);
-                tls->session = NULL;
+                gnutls_deinit(vs->tls.session);
+                vs->tls.session = NULL;
                 vnc_client_error(vs);
                 return -1;
             }
-            if (gnutls_credentials_set(tls->session, GNUTLS_CRD_ANON, anon_cred) < 0) {
-                gnutls_deinit(tls->session);
-                tls->session = NULL;
+            if (gnutls_credentials_set(vs->tls.session,
+                                       GNUTLS_CRD_ANON, anon_cred) < 0) {
+                gnutls_deinit(vs->tls.session);
+                vs->tls.session = NULL;
                 gnutls_anon_free_server_credentials(anon_cred);
                 vnc_client_error(vs);
                 return -1;
             }
         }
 
-        gnutls_transport_set_ptr(tls->session, (gnutls_transport_ptr_t)vs);
-        gnutls_transport_set_push_function(tls->session, vnc_tls_push);
-        gnutls_transport_set_pull_function(tls->session, vnc_tls_pull);
+        gnutls_transport_set_ptr(vs->tls.session, (gnutls_transport_ptr_t)vs);
+        gnutls_transport_set_push_function(vs->tls.session, vnc_tls_push);
+        gnutls_transport_set_pull_function(vs->tls.session, vnc_tls_pull);
     }
     return 0;
 }
@@ -422,13 +417,6 @@ void vnc_tls_client_cleanup(struct VncState *vs)
         vs->tls.session = NULL;
     }
     g_free(vs->tls.dname);
-#ifdef CONFIG_VNC_WS
-    if (vs->ws_tls.session) {
-        gnutls_deinit(vs->ws_tls.session);
-        vs->ws_tls.session = NULL;
-    }
-    g_free(vs->ws_tls.dname);
-#endif /* CONFIG_VNC_WS */
 }
 
 
diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index 0fcce4e..5f9fcc4 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -26,12 +26,12 @@
 
 static int vncws_start_tls_handshake(struct VncState *vs)
 {
-    int ret = gnutls_handshake(vs->ws_tls.session);
+    int ret = gnutls_handshake(vs->tls.session);
 
     if (ret < 0) {
         if (!gnutls_error_is_fatal(ret)) {
             VNC_DEBUG("Handshake interrupted (blocking)\n");
-            if (!gnutls_record_get_direction(vs->ws_tls.session)) {
+            if (!gnutls_record_get_direction(vs->tls.session)) {
                 qemu_set_fd_handler(vs->csock, vncws_tls_handshake_io,
                                     NULL, vs);
             } else {
diff --git a/ui/vnc.c b/ui/vnc.c
index 9994de1..cffb5b7 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1343,15 +1343,8 @@ long vnc_client_write_buf(VncState *vs, const uint8_t *data, size_t datalen)
     if (vs->tls.session) {
         ret = vnc_client_write_tls(&vs->tls.session, data, datalen);
     } else {
-#ifdef CONFIG_VNC_WS
-        if (vs->ws_tls.session) {
-            ret = vnc_client_write_tls(&vs->ws_tls.session, data, datalen);
-        } else
-#endif /* CONFIG_VNC_WS */
 #endif /* CONFIG_VNC_TLS */
-        {
-            ret = send(vs->csock, (const void *)data, datalen, 0);
-        }
+        ret = send(vs->csock, (const void *)data, datalen, 0);
 #ifdef CONFIG_VNC_TLS
     }
 #endif /* CONFIG_VNC_TLS */
@@ -1491,15 +1484,8 @@ long vnc_client_read_buf(VncState *vs, uint8_t *data, size_t datalen)
     if (vs->tls.session) {
         ret = vnc_client_read_tls(&vs->tls.session, data, datalen);
     } else {
-#ifdef CONFIG_VNC_WS
-        if (vs->ws_tls.session) {
-            ret = vnc_client_read_tls(&vs->ws_tls.session, data, datalen);
-        } else
-#endif /* CONFIG_VNC_WS */
 #endif /* CONFIG_VNC_TLS */
-        {
-            ret = qemu_recv(vs->csock, data, datalen, 0);
-        }
+        ret = qemu_recv(vs->csock, data, datalen, 0);
 #ifdef CONFIG_VNC_TLS
     }
 #endif /* CONFIG_VNC_TLS */
diff --git a/ui/vnc.h b/ui/vnc.h
index aac9156..e19ac39 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -295,9 +295,6 @@ struct VncState
     VncStateSASL sasl;
 #endif
 #ifdef CONFIG_VNC_WS
-#ifdef CONFIG_VNC_TLS
-    VncStateTLS ws_tls;
-#endif /* CONFIG_VNC_TLS */
     bool encode_ws;
     bool websocket;
 #endif /* CONFIG_VNC_WS */
-- 
1.8.3.1

[Qemu-devel] [PULL 9/9] ui: ensure VNC websockets server checks the ACL if requested

[Gerd Hoffmann]

From: "Daniel P. Berrange" <berrange@redhat.com>

If the x509verify option is requested, the VNC websockets server
was failing to validate that the websockets client provided an
x509 certificate matching the ACL rules.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc-ws.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index 5f9fcc4..85dbb7e 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -45,6 +45,16 @@ static int vncws_start_tls_handshake(struct VncState *vs)
         return -1;
     }
 
+    if (vs->vd->tls.x509verify) {
+        if (vnc_tls_validate_certificate(vs) < 0) {
+            VNC_DEBUG("Client verification failed\n");
+            vnc_client_error(vs);
+            return -1;
+        } else {
+            VNC_DEBUG("Client verification passed\n");
+        }
+    }
+
     VNC_DEBUG("Handshake done, switching to TLS data mode\n");
     qemu_set_fd_handler2(vs->csock, NULL, vncws_handshake_read, NULL, vs);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PULL for-2.3 0/9] vnc patch queue.

[Peter Maydell]

On 18 March 2015 at 13:17, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> One QMP fix from Markus, needs to go into 2.3 because it is a API fix.
> A bunch of websocket fixes from Daniel, with some cleanup work.  The
> problems fixed are serious enouth that I don't want delay this to 2.4
> even though there are some non-trivial changes in the vnc websockets
> code.
>
> please pull,
>   Gerd
>
> The following changes since commit 5a4992834daec85c3913654903fb9f4f954e585a:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-cov-model-2015-03-17' into staging (2015-03-17 11:43:00 +0000)
>
> are available in the git repository at:
>
>
>   git://git.kraxel.org/qemu tags/pull-vnc-20150318-1
>
> for you to fetch changes up to 4a48aaa9f52dbac148be24f591de2f28c58ccb5d:
>
>   ui: ensure VNC websockets server checks the ACL if requested (2015-03-18 09:25:14 +0100)
>
> ----------------------------------------------------------------
> vnc: fix websockets & QMP.

Applied, thanks.

-- PMM