From patchwork Mon Mar 16 12:36:04 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Daniel_P=2E_Berrang=C3=A9?= X-Patchwork-Id: 450537 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4E1251400B7 for ; Mon, 16 Mar 2015 23:36:53 +1100 (AEDT) Received: from localhost ([::1]:49021 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXUGJ-0005sl-I4 for incoming@patchwork.ozlabs.org; Mon, 16 Mar 2015 08:36:51 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41221) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXUFr-0005Do-It for qemu-devel@nongnu.org; Mon, 16 Mar 2015 08:36:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXUFl-0003jd-44 for qemu-devel@nongnu.org; Mon, 16 Mar 2015 08:36:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58998) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXUFk-0003jY-Ti for qemu-devel@nongnu.org; Mon, 16 Mar 2015 08:36:17 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 8A0AEC79C1 for ; Mon, 16 Mar 2015 12:36:16 +0000 (UTC) Received: from localhost.localdomain.com (vpn1-6-75.ams2.redhat.com [10.36.6.75]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2GCa94N003000; Mon, 16 Mar 2015 08:36:14 -0400 From: "Daniel P. Berrange" To: qemu-devel@nongnu.org Date: Mon, 16 Mar 2015 12:36:04 +0000 Message-Id: <1426509364-19513-4-git-send-email-berrange@redhat.com> In-Reply-To: <1426509364-19513-1-git-send-email-berrange@redhat.com> References: <1426509364-19513-1-git-send-email-berrange@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Gerd Hoffmann Subject: [Qemu-devel] [PATCH 3/3] ui: fix VNC websockets TLS integration X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- 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 */