Daniel P. Berrange <berra...@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
