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

["Alex Bennée" <alex.bennee@linaro.org>]

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