[Qemu-devel] [PATCH 3/3] ui: fix VNC websockets TLS integration

["Daniel P. Berrange" <berrange@redhat.com>]

The way the websockets TLS code was integrated into the VNC server
made it insecure and 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 it would not even get
past auth.

Second, the enablement of TLS on the server is supposed to prevent
any unencrypted clients from connecting. The way the websockets
code implemented TLS though, it would happily allow non-TLS clients
to connect to a TLS enabled server.

Third, the websockets TLS server code never checked the x509 client
cert whitelist after the TLS handshake completed, so it allowed any
client to connect, just relying on the weak VNC password.

With this patch applied a number of things change

 - TLS is not activated for websockets unless the 'tls' flag is
   actually given.
 - Non-TLS websockets connections are dropped if TLS is active
 - The client certificate is validated after handshake completes
   if the 'x509verify' flag is given
 - Separate VNC auth scheme is tracked for websockets server,
   since it makes no sense to try to use VeNCrypt over a TLS
   enabled websockets connection.
 - The separate "VncDisplayTLS ws_tls" field is dropped, since
   the auth setup ensures we can never have multiple TLS sessions.

This ensures that when TLS is activated for websockets, it has
exactly the same security characteristics as when activated for
the primary VNC socket.

Eliminating these difference in behaviour is also important to
prepare for future the refactoring work on TLS work, which will
ensure identical code paths are taken for TLS handshakes in both
websockets and non-websockets scenarios.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 ui/vnc-tls.c | 70 +++++++++++++++++++++++++-----------------------------------
 ui/vnc-ws.c  | 35 ++++++++----------------------
 ui/vnc-ws.h  |  2 +-
 ui/vnc.c     | 62 ++++++++++++++++++++++++++++++++++++-----------------
 ui/vnc.h     |  9 +++++---
 5 files changed, 87 insertions(+), 91 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 1769d52..5f9fcc4 100644
--- a/ui/vnc-ws.c
+++ b/ui/vnc-ws.c
@@ -24,16 +24,14 @@
 #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);
+    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 {
@@ -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 80dc63b..90684f1 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 */
@@ -3014,11 +3000,29 @@ static void vnc_connect(VncDisplay *vd, int csock,
 	vs->subauth = VNC_AUTH_INVALID;
 #endif
     } else {
-	vs->auth = vd->auth;
+#ifdef CONFIG_VNC_WS
+        if (websocket) {
+            vs->auth = vd->ws_auth;
+#ifdef CONFIG_VNC_TLS
+            vs->subauth = VNC_AUTH_INVALID;
+#endif
+        } else {
+#endif
+            vs->auth = vd->auth;
 #ifdef CONFIG_VNC_TLS
-	vs->subauth = vd->subauth;
+            vs->subauth = vd->subauth;
+#endif
+#ifdef CONFIG_VNC_WS
+        }
 #endif
     }
+#ifdef CONFIG_VNC_TLS
+    VNC_DEBUG("Client sock=%d ws=%d auth=%d subauth=%d\n",
+              csock, websocket, vs->auth, vs->subauth);
+#else
+    VNC_DEBUG("Client sock=%d ws=%d auth=%d\n",
+              csock, websocket, vs->auth);
+#endif
 
     vs->lossy_rect = g_malloc0(VNC_STAT_ROWS * sizeof (*vs->lossy_rect));
     for (i = 0; i < VNC_STAT_ROWS; ++i) {
@@ -3032,8 +3036,8 @@ static void vnc_connect(VncDisplay *vd, int csock,
     if (websocket) {
         vs->websocket = 1;
 #ifdef CONFIG_VNC_TLS
-        if (vd->tls.x509cert) {
-            qemu_set_fd_handler2(vs->csock, NULL, vncws_tls_handshake_peek,
+        if (vd->ws_tls) {
+            qemu_set_fd_handler2(vs->csock, NULL, vncws_tls_handshake_io,
                                  NULL, vs);
         } else
 #endif /* CONFIG_VNC_TLS */
@@ -3577,6 +3581,24 @@ void vnc_display_open(const char *id, Error **errp)
         }
 #endif
     }
+#ifdef CONFIG_VNC_WS
+    if (websocket) {
+        if (password) {
+            vs->ws_auth = VNC_AUTH_VNC;
+#ifdef CONFIG_VNC_SASL
+        } else if (sasl) {
+            vs->ws_auth = VNC_AUTH_SASL;
+#endif
+        } else {
+            vs->ws_auth = VNC_AUTH_NONE;
+        }
+#ifdef CONFIG_VNC_TLS
+        if (tls) {
+            vs->ws_tls = true;
+        }
+#endif
+    }
+#endif
 
 #ifdef CONFIG_VNC_SASL
     if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
diff --git a/ui/vnc.h b/ui/vnc.h
index 66a0298..fc4ac9e 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -180,6 +180,12 @@ struct VncDisplay
     char *password;
     time_t expires;
     int auth;
+#ifdef CONFIG_VNC_WS
+    int ws_auth;
+#ifdef CONFIG_VNC_TLS
+    bool ws_tls;
+#endif
+#endif
     bool lossy;
     bool non_adaptive;
 #ifdef CONFIG_VNC_TLS
@@ -293,9 +299,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 */
-- 
2.1.0