Misc fixes for VNC

Signed-off-by Daniel P. Berrange <[email protected]>

---
 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(-)

ui/vnc-auth-vencrypt.c +0 -1

index bc7032e..a420ccb 100644
@@ -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);

ui/vnc-tls.c +0 -2

index 0f59f9b..de1cb34 100644
@@ -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 */
 }

ui/vnc-tls.h +0 -7

index 36a2227..f9829c7 100644
@@ -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 */

ui/vnc-ws.c +0 -1

index d75950d..1769d52 100644
@@ -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;

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 <[email protected]>

---
 ui/vnc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

ui/vnc.c +6 -6

index 6f9b718..80dc63b 100644
@@ -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;
     }

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

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.

(quoted diff) +0 -0

 }
 
 

ui/vnc-ws.c +9 -26

index 1769d52..5f9fcc4 100644
@@ -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 */
 

ui/vnc-ws.h +1 -1

index 95c1b0a..ef229b7 100644
@@ -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);

ui/vnc.c +42 -20

index 80dc63b..90684f1 100644
@@ -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) {

ui/vnc.h +6 -3

index 66a0298..fc4ac9e 100644
@@ -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 */

Quoted 3 lines · 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

<snip>

Quoted 13 lines

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?

Quoted 6 lines

ui/vnc-tls.c +0 -2

@@ -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) {

ui/vnc-tls.c void vnc_tls_client_cleanup(struct VncState *vs) · line 431

Quoted 5 lines

ui/vnc-tls.c +0 -5

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

Quoted 21 lines · Alex Bennée

Quoted 3 lines · 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

<snip>

Quoted 13 lines

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.

Quoted 3 lines

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.

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

Hi,

Quoted 3 lines

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

Quoted 6 lines

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

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.

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.

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

Hi,

Quoted 9 lines

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.

Quoted 21 lines

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

Quoted 4 lines

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

Quoted 3 lines

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?

Hi,

Quoted 3 lines

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

Quoted 8 lines · Gerd Hoffmann

Hi,

Quoted 3 lines

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

qapi-schema.json +1 -0

index d7c3eec..3362956 100644
@@ -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