[Qemu-devel] [PATCH 0/3] Misc fixes for VNC

[Qemu-devel] [PATCH 0/3] Misc fixes for VNC

[Daniel P. Berrange]

This is a small series of fixes for the VNC server, the most significant
of which is the last one affecting websockets + TLS integration. These
prepare the way for work I'm doing to refactor TLS handling into a module
that is reusable across QEMU.

Daniel P. Berrange (3):
  ui: remove unused 'wiremode' variable in VncState struct
  ui: replace printf() calls with VNC_DEBUG
  ui: fix VNC websockets TLS integration

 ui/vnc-auth-vencrypt.c |  1 -
 ui/vnc-tls.c           | 72 ++++++++++++++++++++----------------------------
 ui/vnc-tls.h           |  7 -----
 ui/vnc-ws.c            | 36 ++++++------------------
 ui/vnc-ws.h            |  2 +-
 ui/vnc.c               | 74 ++++++++++++++++++++++++++++++++------------------
 ui/vnc.h               |  9 ++++--
 7 files changed, 93 insertions(+), 108 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/3] ui: remove unused 'wiremode' variable in VncState struct

[Daniel P. Berrange]

Signed-off-by: Daniel P. Berrange <berrange@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;
-- 
2.1.0

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

[Daniel P. Berrange]

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>
---
 ui/vnc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 6f9b718..80dc63b 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;
     }
-- 
2.1.0

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

[Daniel P. Berrange]

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

Re: [Qemu-devel] [PATCH 1/3] ui: remove unused 'wiremode' variable in VncState struct

[Alex Bennée]

Daniel P. Berrange <berrange@redhat.com> writes:

> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

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

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 2/3] ui: replace printf() calls with VNC_DEBUG

[Alex Bennée]

Daniel P. Berrange <berrange@redhat.com> writes:

> 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>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  ui/vnc.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 6f9b718..80dc63b 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;
>      }

-- 
Alex Bennée

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

[Alex Bennée]

Daniel P. Berrange <berrange@redhat.com> writes:

> 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
<snip>
>
> 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.

I wonder if the mechanical changes to the tls field could be separated
from the logic changes to the handling of authentication and certificate
checking?

> @@ -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 */

I get we have added a bunch of exit cases earlier on that clean-up but
what happens when we do a clean shutdown? Have we just leaked?

Perhaps the tls.session cleanup code should be in a shared function?

>  }
>  
>  
> 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 */

-- 
Alex Bennée

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

[Daniel P. Berrange]

On Mon, Mar 16, 2015 at 01:17:16PM +0000, Alex Bennée wrote:
> 
> Daniel P. Berrange <berrange@redhat.com> writes:
> 
> > 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
> <snip>
> >
> > 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.
> 
> I wonder if the mechanical changes to the tls field could be separated
> from the logic changes to the handling of authentication and certificate
> checking?

They are rather intertwined, because the need for this duplicated
TLS field was a result of the way auth was mishandled. So cleaning
up one implies cleaning up the other & vica-verca.

> > @@ -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 */
> 
> I get we have added a bunch of exit cases earlier on that clean-up but
> what happens when we do a clean shutdown? Have we just leaked?

We deleted the vs->ws_tls field from the struct entirely, so there's
never any data to be leaked here.

> Perhaps the tls.session cleanup code should be in a shared function?

This is patch is a precursor to a major refactoring & cleanup of
the TLS code which will reduce code duplication significantly

   https://github.com/berrange/qemu/commits/qemu-io-channel-4

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 3/3] ui: fix VNC websockets TLS integration

[Gerd Hoffmann]

  Hi,

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

Hmm.  That is a problem for the QAPI, the auth scheme is linked to the
vnc server not the socket.

What is the point in having separate auth schemes for normal sockets and
websockets?  From a security point of view it IMHO doesn't buy you much
to have a better auch scheme on the normal sockets as the user/client
has the option to choose websockets ...

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

Except for the auth scheme.

cheers,
  Gerd

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

[Daniel P. Berrange]

On Tue, Mar 17, 2015 at 08:36:40AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >  - 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.
> 
> Hmm.  That is a problem for the QAPI, the auth scheme is linked to the
> vnc server not the socket.
> 
> What is the point in having separate auth schemes for normal sockets and
> websockets?  From a security point of view it IMHO doesn't buy you much
> to have a better auch scheme on the normal sockets as the user/client
> has the option to choose websockets ...

The problem is that the VeNCrypt auth scheme is not actally really
an auth scheme. VeNCrypt is a way to negotiate TLS session on the
VNC server, and then run one of the traditionl auth schemes over
that session. When using websockets, we cannot use VeNCrypt because
the browser websockets client can't do TLS negotigate part way
through the VNC protocol auth process. It has to have TLS on the
connection as a whole, hence the VNC websockets server will setup
TLS during the initial HTTP header phase, before the VNC protocol
even starts running.

Currently if you have VeNCrypt enabled (due to 'tls' flag to -vnc)
then the websockets server is useless because no client can connect.
The only solution to this is to not use VeNCrypt at all.

I could have just stuck with the 'auth' & 'subauth' fields in the
VncDisplay class, and translated them into something else in the
vnc_client_connect method when setting up VncState, but i figure
it was clearer to just add a 'ws_auth' field to VncDisplay
instead and avoid the translation step.

> >  - 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.
> 
> Except for the auth scheme.

When I say they are the same, I mean from a high level security
characteristics, not the low level protocol auth codes.

 eg if you -vnc 127.0.0.1:5901,websockets=5902,tls,x509,password

Then for normal VNC server you will get

  vs->auth = VNC_AUTH_VENCRYPT
  vs->subauth = VNC_AUTH_VENCRYPT_X509VNC

This gives a TLS handshake, with x509 certificates and the VNC password
auth scheme.

And for the websockets VNC server you will get

  vs->ws_auth = VNC_AUTH_VNC

combined with https:// requirement. This gives a TLS handshake with
x509 certificates and VNC password auth scheme.

So, yes, the VNC protocol auth numbers are diferent, but the actual
security characteristics, encryption setup and auth scheme *are*
identical.

If we made the VNC auth protocol codes the same in both cases, you
would actually be making the security setup *different*, because you
would have TLS running over TLS giving double-encryption in the
websockets case. Of course you can't do that because the browser has
no way to achieve that, hence why the current QEMU impl is unusuably
broken for websockets + TLS.


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 3/3] ui: fix VNC websockets TLS integration

[Daniel P. Berrange]

On Tue, Mar 17, 2015 at 08:36:40AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >  - 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.
> 
> Hmm.  That is a problem for the QAPI, the auth scheme is linked to the
> vnc server not the socket.

It seems straightforward enough to just do this:

diff --git a/qapi-schema.json b/qapi-schema.json
index d7c3eec..3362956 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -808,6 +808,7 @@
             'clients'   : ['VncClientInfo'],
             'auth'      : 'VncPrimaryAuth',
             '*vencrypt' : 'VncVencryptSubAuth',
+            '*ws-auth'  : 'VncPrimaryAuth',
             '*display'  : 'str' } }

And document that 'ws-auth' is used if server->websocket == true

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 3/3] ui: fix VNC websockets TLS integration

[Gerd Hoffmann]

  Hi,

> The problem is that the VeNCrypt auth scheme is not actally really
> an auth scheme. VeNCrypt is a way to negotiate TLS session on the
> VNC server, and then run one of the traditionl auth schemes over
> that session. When using websockets, we cannot use VeNCrypt because
> the browser websockets client can't do TLS negotigate part way
> through the VNC protocol auth process. It has to have TLS on the
> connection as a whole, hence the VNC websockets server will setup
> TLS during the initial HTTP header phase, before the VNC protocol
> even starts running.

Understood.

> I could have just stuck with the 'auth' & 'subauth' fields in the
> VncDisplay class, and translated them into something else in the
> vnc_client_connect method when setting up VncState, but i figure
> it was clearer to just add a 'ws_auth' field to VncDisplay
> instead and avoid the translation step.

> When I say they are the same, I mean from a high level security
> characteristics, not the low level protocol auth codes.
> 
>  eg if you -vnc 127.0.0.1:5901,websockets=5902,tls,x509,password
> 
> Then for normal VNC server you will get
> 
>   vs->auth = VNC_AUTH_VENCRYPT
>   vs->subauth = VNC_AUTH_VENCRYPT_X509VNC
> 
> This gives a TLS handshake, with x509 certificates and the VNC password
> auth scheme.
> 
> And for the websockets VNC server you will get
> 
>   vs->ws_auth = VNC_AUTH_VNC
> 
> combined with https:// requirement. This gives a TLS handshake with
> x509 certificates and VNC password auth scheme.

Ok, so there basically is a fixed mapping from auth+subauth to ws_auth
+ws_tls, correct?

I think we should have a function setting ws_auth+ws_tls that way then,
to make clear how this works, with a comment explaining things (which
you can probably largely cut+paste from your mail ;)

> So, yes, the VNC protocol auth numbers are diferent, but the actual
> security characteristics, encryption setup and auth scheme *are*
> identical.

I guess we can live with the current QAPI schema then?

cheers,
  Gerd

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

[Gerd Hoffmann]

On Di, 2015-03-17 at 10:33 +0000, Daniel P. Berrange wrote:
> On Tue, Mar 17, 2015 at 08:36:40AM +0100, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > >  - 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.
> > 
> > Hmm.  That is a problem for the QAPI, the auth scheme is linked to the
> > vnc server not the socket.
> 
> It seems straightforward enough to just do this:
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d7c3eec..3362956 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -808,6 +808,7 @@
>              'clients'   : ['VncClientInfo'],
>              'auth'      : 'VncPrimaryAuth',
>              '*vencrypt' : 'VncVencryptSubAuth',
> +            '*ws-auth'  : 'VncPrimaryAuth',
>              '*display'  : 'str' } }
> 
> And document that 'ws-auth' is used if server->websocket == true

When doing it this way we probably want add '*ws-tls' : 'bool' too.

I'm fine either way (adding both or -- given the fixed scheme mapping we
have -- none).  Pick whatever suits libvirt best.

cheers,
  Gerd

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

[Daniel P. Berrange]

On Tue, Mar 17, 2015 at 11:50:46AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > The problem is that the VeNCrypt auth scheme is not actally really
> > an auth scheme. VeNCrypt is a way to negotiate TLS session on the
> > VNC server, and then run one of the traditionl auth schemes over
> > that session. When using websockets, we cannot use VeNCrypt because
> > the browser websockets client can't do TLS negotigate part way
> > through the VNC protocol auth process. It has to have TLS on the
> > connection as a whole, hence the VNC websockets server will setup
> > TLS during the initial HTTP header phase, before the VNC protocol
> > even starts running.
> 
> Understood.
> 
> > I could have just stuck with the 'auth' & 'subauth' fields in the
> > VncDisplay class, and translated them into something else in the
> > vnc_client_connect method when setting up VncState, but i figure
> > it was clearer to just add a 'ws_auth' field to VncDisplay
> > instead and avoid the translation step.
> 
> > When I say they are the same, I mean from a high level security
> > characteristics, not the low level protocol auth codes.
> > 
> >  eg if you -vnc 127.0.0.1:5901,websockets=5902,tls,x509,password
> > 
> > Then for normal VNC server you will get
> > 
> >   vs->auth = VNC_AUTH_VENCRYPT
> >   vs->subauth = VNC_AUTH_VENCRYPT_X509VNC
> > 
> > This gives a TLS handshake, with x509 certificates and the VNC password
> > auth scheme.
> > 
> > And for the websockets VNC server you will get
> > 
> >   vs->ws_auth = VNC_AUTH_VNC
> > 
> > combined with https:// requirement. This gives a TLS handshake with
> > x509 certificates and VNC password auth scheme.
> 
> Ok, so there basically is a fixed mapping from auth+subauth to ws_auth
> +ws_tls, correct?

Yes, that's correct.

> I think we should have a function setting ws_auth+ws_tls that way then,
> to make clear how this works, with a comment explaining things (which
> you can probably largely cut+paste from your mail ;)

Ok, I'll separate the code into a standalone function and try to split
this change up a bit more so it only does one thing at a time.

> > So, yes, the VNC protocol auth numbers are diferent, but the actual
> > security characteristics, encryption setup and auth scheme *are*
> > identical.
> 
> I guess we can live with the current QAPI schema then?

I thing thats probably the simplest - fwiw, libvirt won't use any extra
info even if we provided it & we can always add it later if required.

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 3/3] ui: fix VNC websockets TLS integration

[Daniel P. Berrange]

On Mon, Mar 16, 2015 at 01:35:02PM +0000, Daniel P. Berrange wrote:
> On Mon, Mar 16, 2015 at 01:17:16PM +0000, Alex Bennée wrote:
> > 
> > Daniel P. Berrange <berrange@redhat.com> writes:
> > 
> > > 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
> > <snip>
> > >
> > > 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.
> > 
> > I wonder if the mechanical changes to the tls field could be separated
> > from the logic changes to the handling of authentication and certificate
> > checking?
> 
> They are rather intertwined, because the need for this duplicated
> TLS field was a result of the way auth was mishandled. So cleaning
> up one implies cleaning up the other & vica-verca.

I've actually realized I can split it, if I do the auth scheme clean
up first.


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