From f3cf80e805bc22980733606df15917223f311f4b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 13 Mar 2015 18:43:09 +0100 Subject: [PATCH 1/9] vnc: Fix QMP change not to use funky error class Error classes are a leftover from the days of "rich" error objects. New code should always use ERROR_CLASS_GENERIC_ERROR. Commit 1d0d59f added a use of ERROR_CLASS_DEVICE_NOT_FOUND. Replace it. Signed-off-by: Gerd Hoffmann --- ui/vnc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/vnc.c b/ui/vnc.c index 6f9b718814..a950016aaa 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -3594,7 +3594,7 @@ void vnc_display_open(const char *id, Error **errp) dev = qdev_find_recursive(sysbus_get_default(), device_id); if (dev == NULL) { - error_set(errp, QERR_DEVICE_NOT_FOUND, device_id); + error_setg(errp, "Device '%s' not found", device_id); goto fail; } From a2f45bc02ae9be18119d2fb88042ef19e7e9247f Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 17 Mar 2015 13:42:53 +0000 Subject: [PATCH 2/9] ui: remove unused 'wiremode' variable in VncState struct Signed-off-by: Daniel P. Berrange Signed-off-by: Gerd Hoffmann --- 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 bc7032e695..a420ccbd1d 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 0f59f9b28e..de1cb342a4 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 36a2227fec..f9829c7824 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 d75950d7b1..1769d526f8 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; From 153130cd4fa236e29bb6243eebf9439b983ca266 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 17 Mar 2015 13:42:54 +0000 Subject: [PATCH 3/9] ui: replace printf() calls with VNC_DEBUG 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 Signed-off-by: Gerd Hoffmann --- ui/vnc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ui/vnc.c b/ui/vnc.c index a950016aaa..6f0b0ce3b9 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; } From d169f04b8b8424ad9c5377bb5391de2f760e3db1 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 17 Mar 2015 13:42:55 +0000 Subject: [PATCH 4/9] ui: report error if user requests VNC option that is unsupported If the VNC server is built without tls, sasl or websocket support and the user requests one of these features, they are just silently ignored. This is bad because it means the VNC server ends up running in a configuration that is less secure than the user asked for. It also leads to an tangled mass of preprocessor conditionals when configuring the VNC server. This ensures that the tls, sasl & websocket options are always processed and an error is reported back to the user if any of them were disabled at build time. Signed-off-by: Daniel P. Berrange Signed-off-by: Gerd Hoffmann --- ui/vnc.c | 51 +++++++++++++++++++++------------------------------ ui/vnc.h | 4 ++-- 2 files changed, 23 insertions(+), 32 deletions(-) diff --git a/ui/vnc.c b/ui/vnc.c index 6f0b0ce3b9..d5e60248af 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -3010,14 +3010,10 @@ static void vnc_connect(VncDisplay *vd, int csock, if (skipauth) { vs->auth = VNC_AUTH_NONE; -#ifdef CONFIG_VNC_TLS vs->subauth = VNC_AUTH_INVALID; -#endif } else { vs->auth = vd->auth; -#ifdef CONFIG_VNC_TLS vs->subauth = vd->subauth; -#endif } vs->lossy_rect = g_malloc0(VNC_STAT_ROWS * sizeof (*vs->lossy_rect)); @@ -3206,8 +3202,8 @@ static void vnc_display_close(VncDisplay *vs) } #endif /* CONFIG_VNC_WS */ vs->auth = VNC_AUTH_INVALID; -#ifdef CONFIG_VNC_TLS vs->subauth = VNC_AUTH_INVALID; +#ifdef CONFIG_VNC_TLS vs->tls.x509verify = 0; #endif } @@ -3332,15 +3328,13 @@ void vnc_display_open(const char *id, Error **errp) char *h; bool has_ipv4 = false; bool has_ipv6 = false; -#ifdef CONFIG_VNC_WS const char *websocket; -#endif -#ifdef CONFIG_VNC_TLS bool tls = false, x509 = false; +#ifdef CONFIG_VNC_TLS const char *path; #endif -#ifdef CONFIG_VNC_SASL bool sasl = false; +#ifdef CONFIG_VNC_SASL int saslErr; #endif #if defined(CONFIG_VNC_TLS) || defined(CONFIG_VNC_SASL) @@ -3404,11 +3398,15 @@ void vnc_display_open(const char *id, Error **errp) reverse = qemu_opt_get_bool(opts, "reverse", false); lock_key_sync = qemu_opt_get_bool(opts, "lock-key-sync", true); -#ifdef CONFIG_VNC_SASL sasl = qemu_opt_get_bool(opts, "sasl", false); -#endif -#ifdef CONFIG_VNC_TLS +#ifndef CONFIG_VNC_SASL + if (sasl) { + error_setg(errp, "VNC SASL auth requires cyrus-sasl support"); + goto fail; + } +#endif /* CONFIG_VNC_SASL */ tls = qemu_opt_get_bool(opts, "tls", false); +#ifdef CONFIG_VNC_TLS path = qemu_opt_get(opts, "x509"); if (!path) { path = qemu_opt_get(opts, "x509verify"); @@ -3424,7 +3422,12 @@ void vnc_display_open(const char *id, Error **errp) goto fail; } } -#endif +#else /* ! CONFIG_VNC_TLS */ + if (tls) { + error_setg(errp, "VNC TLS auth requires gnutls support"); + goto fail; + } +#endif /* ! CONFIG_VNC_TLS */ #if defined(CONFIG_VNC_TLS) || defined(CONFIG_VNC_SASL) acl = qemu_opt_get_bool(opts, "acl", false); #endif @@ -3446,14 +3449,16 @@ void vnc_display_open(const char *id, Error **errp) } vs->connections_limit = qemu_opt_get_number(opts, "connections", 32); - #ifdef CONFIG_VNC_WS websocket = qemu_opt_get(opts, "websocket"); if (websocket) { +#ifdef CONFIG_VNC_WS vs->ws_enabled = true; qemu_opt_set(wsopts, "port", websocket, &error_abort); - +#else /* ! CONFIG_VNC_WS */ + error_setg(errp, "Websockets protocol requires gnutls support"); + goto fail; +#endif /* ! CONFIG_VNC_WS */ } -#endif /* CONFIG_VNC_WS */ #ifdef CONFIG_VNC_JPEG vs->lossy = qemu_opt_get_bool(opts, "lossy", false); @@ -3518,7 +3523,6 @@ void vnc_display_open(const char *id, Error **errp) * NB2. the x509 schemes have option to validate a client cert dname */ if (password) { -#ifdef CONFIG_VNC_TLS if (tls) { vs->auth = VNC_AUTH_VENCRYPT; if (x509) { @@ -3529,16 +3533,11 @@ void vnc_display_open(const char *id, Error **errp) vs->subauth = VNC_AUTH_VENCRYPT_TLSVNC; } } else { -#endif /* CONFIG_VNC_TLS */ VNC_DEBUG("Initializing VNC server with password auth\n"); vs->auth = VNC_AUTH_VNC; -#ifdef CONFIG_VNC_TLS vs->subauth = VNC_AUTH_INVALID; } -#endif /* CONFIG_VNC_TLS */ -#ifdef CONFIG_VNC_SASL } else if (sasl) { -#ifdef CONFIG_VNC_TLS if (tls) { vs->auth = VNC_AUTH_VENCRYPT; if (x509) { @@ -3549,16 +3548,11 @@ void vnc_display_open(const char *id, Error **errp) vs->subauth = VNC_AUTH_VENCRYPT_TLSSASL; } } else { -#endif /* CONFIG_VNC_TLS */ VNC_DEBUG("Initializing VNC server with SASL auth\n"); vs->auth = VNC_AUTH_SASL; -#ifdef CONFIG_VNC_TLS vs->subauth = VNC_AUTH_INVALID; } -#endif /* CONFIG_VNC_TLS */ -#endif /* CONFIG_VNC_SASL */ } else { -#ifdef CONFIG_VNC_TLS if (tls) { vs->auth = VNC_AUTH_VENCRYPT; if (x509) { @@ -3569,13 +3563,10 @@ void vnc_display_open(const char *id, Error **errp) vs->subauth = VNC_AUTH_VENCRYPT_TLSNONE; } } else { -#endif VNC_DEBUG("Initializing VNC server with no auth\n"); vs->auth = VNC_AUTH_NONE; -#ifdef CONFIG_VNC_TLS vs->subauth = VNC_AUTH_INVALID; } -#endif } #ifdef CONFIG_VNC_SASL diff --git a/ui/vnc.h b/ui/vnc.h index 66a02986c7..90b25926ca 100644 --- a/ui/vnc.h +++ b/ui/vnc.h @@ -180,10 +180,10 @@ struct VncDisplay char *password; time_t expires; int auth; + int subauth; /* Used by VeNCrypt */ bool lossy; bool non_adaptive; #ifdef CONFIG_VNC_TLS - int subauth; /* Used by VeNCrypt */ VncDisplayTLS tls; #endif #ifdef CONFIG_VNC_SASL @@ -284,9 +284,9 @@ struct VncState int minor; int auth; + int subauth; /* Used by VeNCrypt */ char challenge[VNC_AUTH_CHALLENGE_SIZE]; #ifdef CONFIG_VNC_TLS - int subauth; /* Used by VeNCrypt */ VncStateTLS tls; #endif #ifdef CONFIG_VNC_SASL From 0dd72e1531f0ea1a62fd016702ea3b868d116bd8 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 17 Mar 2015 13:42:56 +0000 Subject: [PATCH 5/9] ui: split setup of VNC auth scheme into separate method The vnc_display_open method is quite long and complex, so move the VNC auth scheme decision logic into a separate method for clarity. Also update the comment to better describe what we are trying to achieve. Signed-off-by: Daniel P. Berrange Signed-off-by: Gerd Hoffmann --- ui/vnc.c | 153 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 91 insertions(+), 62 deletions(-) diff --git a/ui/vnc.c b/ui/vnc.c index d5e60248af..8edbb67a64 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -3314,6 +3314,96 @@ static QemuOptsList qemu_vnc_opts = { }, }; + +static void +vnc_display_setup_auth(VncDisplay *vs, + bool password, + bool sasl, + bool tls, + bool x509) +{ + /* + * We have a choice of 3 authentication options + * + * 1. none + * 2. vnc + * 3. sasl + * + * The channel can be run in 2 modes + * + * 1. clear + * 2. tls + * + * And TLS can use 2 types of credentials + * + * 1. anon + * 2. x509 + * + * We thus have 9 possible logical combinations + * + * 1. clear + none + * 2. clear + vnc + * 3. clear + sasl + * 4. tls + anon + none + * 5. tls + anon + vnc + * 6. tls + anon + sasl + * 7. tls + x509 + none + * 8. tls + x509 + vnc + * 9. tls + x509 + sasl + * + * These need to be mapped into the VNC auth schemes + * in an appropriate manner. In regular VNC, all the + * TLS options get mapped into VNC_AUTH_VENCRYPT + * sub-auth types. + */ + if (password) { + if (tls) { + vs->auth = VNC_AUTH_VENCRYPT; + if (x509) { + VNC_DEBUG("Initializing VNC server with x509 password auth\n"); + vs->subauth = VNC_AUTH_VENCRYPT_X509VNC; + } else { + VNC_DEBUG("Initializing VNC server with TLS password auth\n"); + vs->subauth = VNC_AUTH_VENCRYPT_TLSVNC; + } + } else { + VNC_DEBUG("Initializing VNC server with password auth\n"); + vs->auth = VNC_AUTH_VNC; + vs->subauth = VNC_AUTH_INVALID; + } + } else if (sasl) { + if (tls) { + vs->auth = VNC_AUTH_VENCRYPT; + if (x509) { + VNC_DEBUG("Initializing VNC server with x509 SASL auth\n"); + vs->subauth = VNC_AUTH_VENCRYPT_X509SASL; + } else { + VNC_DEBUG("Initializing VNC server with TLS SASL auth\n"); + vs->subauth = VNC_AUTH_VENCRYPT_TLSSASL; + } + } else { + VNC_DEBUG("Initializing VNC server with SASL auth\n"); + vs->auth = VNC_AUTH_SASL; + vs->subauth = VNC_AUTH_INVALID; + } + } else { + if (tls) { + vs->auth = VNC_AUTH_VENCRYPT; + if (x509) { + VNC_DEBUG("Initializing VNC server with x509 no auth\n"); + vs->subauth = VNC_AUTH_VENCRYPT_X509NONE; + } else { + VNC_DEBUG("Initializing VNC server with TLS no auth\n"); + vs->subauth = VNC_AUTH_VENCRYPT_TLSNONE; + } + } else { + VNC_DEBUG("Initializing VNC server with no auth\n"); + vs->auth = VNC_AUTH_NONE; + vs->subauth = VNC_AUTH_INVALID; + } + } +} + void vnc_display_open(const char *id, Error **errp) { VncDisplay *vs = vnc_display_find(id); @@ -3506,68 +3596,7 @@ void vnc_display_open(const char *id, Error **errp) } #endif - /* - * Combinations we support here: - * - * - no-auth (clear text, no auth) - * - password (clear text, weak auth) - * - sasl (encrypt, good auth *IF* using Kerberos via GSSAPI) - * - tls (encrypt, weak anonymous creds, no auth) - * - tls + password (encrypt, weak anonymous creds, weak auth) - * - tls + sasl (encrypt, weak anonymous creds, good auth) - * - tls + x509 (encrypt, good x509 creds, no auth) - * - tls + x509 + password (encrypt, good x509 creds, weak auth) - * - tls + x509 + sasl (encrypt, good x509 creds, good auth) - * - * NB1. TLS is a stackable auth scheme. - * NB2. the x509 schemes have option to validate a client cert dname - */ - if (password) { - if (tls) { - vs->auth = VNC_AUTH_VENCRYPT; - if (x509) { - VNC_DEBUG("Initializing VNC server with x509 password auth\n"); - vs->subauth = VNC_AUTH_VENCRYPT_X509VNC; - } else { - VNC_DEBUG("Initializing VNC server with TLS password auth\n"); - vs->subauth = VNC_AUTH_VENCRYPT_TLSVNC; - } - } else { - VNC_DEBUG("Initializing VNC server with password auth\n"); - vs->auth = VNC_AUTH_VNC; - vs->subauth = VNC_AUTH_INVALID; - } - } else if (sasl) { - if (tls) { - vs->auth = VNC_AUTH_VENCRYPT; - if (x509) { - VNC_DEBUG("Initializing VNC server with x509 SASL auth\n"); - vs->subauth = VNC_AUTH_VENCRYPT_X509SASL; - } else { - VNC_DEBUG("Initializing VNC server with TLS SASL auth\n"); - vs->subauth = VNC_AUTH_VENCRYPT_TLSSASL; - } - } else { - VNC_DEBUG("Initializing VNC server with SASL auth\n"); - vs->auth = VNC_AUTH_SASL; - vs->subauth = VNC_AUTH_INVALID; - } - } else { - if (tls) { - vs->auth = VNC_AUTH_VENCRYPT; - if (x509) { - VNC_DEBUG("Initializing VNC server with x509 no auth\n"); - vs->subauth = VNC_AUTH_VENCRYPT_X509NONE; - } else { - VNC_DEBUG("Initializing VNC server with TLS no auth\n"); - vs->subauth = VNC_AUTH_VENCRYPT_TLSNONE; - } - } else { - VNC_DEBUG("Initializing VNC server with no auth\n"); - vs->auth = VNC_AUTH_NONE; - vs->subauth = VNC_AUTH_INVALID; - } - } + vnc_display_setup_auth(vs, password, sasl, tls, x509); #ifdef CONFIG_VNC_SASL if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) { From f9148c8ae7b1515776699387b4d59864f302c77d Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 17 Mar 2015 13:42:57 +0000 Subject: [PATCH 6/9] ui: fix setup of VNC websockets auth scheme with TLS The way the websockets TLS code was integrated into the VNC server made it 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, since the browser clients cannot setup a nested TLS session over the main HTTPS connection, so it would not even get past auth. This patch causes us to decide our auth scheme separately for the main VNC server vs the websockets VNC server. We take account of the fact that if TLS is enabled, then the websockets client will use https, so setting up VeNCrypt is thus redundant as it would lead to nested TLS sessions. Signed-off-by: Daniel P. Berrange Signed-off-by: Gerd Hoffmann --- ui/vnc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++----- ui/vnc.h | 2 ++ 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/ui/vnc.c b/ui/vnc.c index 8edbb67a64..7b66f93595 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -3012,9 +3012,16 @@ static void vnc_connect(VncDisplay *vd, int csock, vs->auth = VNC_AUTH_NONE; vs->subauth = VNC_AUTH_INVALID; } else { - vs->auth = vd->auth; - vs->subauth = vd->subauth; + if (websocket) { + vs->auth = vd->ws_auth; + vs->subauth = VNC_AUTH_INVALID; + } else { + vs->auth = vd->auth; + vs->subauth = vd->subauth; + } } + VNC_DEBUG("Client sock=%d ws=%d auth=%d subauth=%d\n", + csock, websocket, vs->auth, vs->subauth); vs->lossy_rect = g_malloc0(VNC_STAT_ROWS * sizeof (*vs->lossy_rect)); for (i = 0; i < VNC_STAT_ROWS; ++i) { @@ -3028,7 +3035,7 @@ static void vnc_connect(VncDisplay *vd, int csock, if (websocket) { vs->websocket = 1; #ifdef CONFIG_VNC_TLS - if (vd->tls.x509cert) { + if (vd->ws_tls) { qemu_set_fd_handler2(vs->csock, NULL, vncws_tls_handshake_peek, NULL, vs); } else @@ -3320,7 +3327,8 @@ vnc_display_setup_auth(VncDisplay *vs, bool password, bool sasl, bool tls, - bool x509) + bool x509, + bool websocket) { /* * We have a choice of 3 authentication options @@ -3355,10 +3363,26 @@ vnc_display_setup_auth(VncDisplay *vs, * in an appropriate manner. In regular VNC, all the * TLS options get mapped into VNC_AUTH_VENCRYPT * sub-auth types. + * + * In websockets, the https:// protocol already provides + * TLS support, so there is no need to make use of the + * VeNCrypt extension. Furthermore, websockets browser + * clients could not use VeNCrypt even if they wanted to, + * as they cannot control when the TLS handshake takes + * place. Thus there is no option but to rely on https://, + * meaning combinations 4->6 and 7->9 will be mapped to + * VNC auth schemes in the same way as combos 1->3. + * + * Regardless of fact that we have a different mapping to + * VNC auth mechs for plain VNC vs websockets VNC, the end + * result has the same security characteristics. */ if (password) { if (tls) { vs->auth = VNC_AUTH_VENCRYPT; + if (websocket) { + vs->ws_tls = true; + } if (x509) { VNC_DEBUG("Initializing VNC server with x509 password auth\n"); vs->subauth = VNC_AUTH_VENCRYPT_X509VNC; @@ -3371,9 +3395,17 @@ vnc_display_setup_auth(VncDisplay *vs, vs->auth = VNC_AUTH_VNC; vs->subauth = VNC_AUTH_INVALID; } + if (websocket) { + vs->ws_auth = VNC_AUTH_VNC; + } else { + vs->ws_auth = VNC_AUTH_INVALID; + } } else if (sasl) { if (tls) { vs->auth = VNC_AUTH_VENCRYPT; + if (websocket) { + vs->ws_tls = true; + } if (x509) { VNC_DEBUG("Initializing VNC server with x509 SASL auth\n"); vs->subauth = VNC_AUTH_VENCRYPT_X509SASL; @@ -3386,9 +3418,17 @@ vnc_display_setup_auth(VncDisplay *vs, vs->auth = VNC_AUTH_SASL; vs->subauth = VNC_AUTH_INVALID; } + if (websocket) { + vs->ws_auth = VNC_AUTH_SASL; + } else { + vs->ws_auth = VNC_AUTH_INVALID; + } } else { if (tls) { vs->auth = VNC_AUTH_VENCRYPT; + if (websocket) { + vs->ws_tls = true; + } if (x509) { VNC_DEBUG("Initializing VNC server with x509 no auth\n"); vs->subauth = VNC_AUTH_VENCRYPT_X509NONE; @@ -3401,6 +3441,11 @@ vnc_display_setup_auth(VncDisplay *vs, vs->auth = VNC_AUTH_NONE; vs->subauth = VNC_AUTH_INVALID; } + if (websocket) { + vs->ws_auth = VNC_AUTH_NONE; + } else { + vs->ws_auth = VNC_AUTH_INVALID; + } } } @@ -3596,7 +3641,7 @@ void vnc_display_open(const char *id, Error **errp) } #endif - vnc_display_setup_auth(vs, password, sasl, tls, x509); + vnc_display_setup_auth(vs, password, sasl, tls, x509, websocket); #ifdef CONFIG_VNC_SASL if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) { diff --git a/ui/vnc.h b/ui/vnc.h index 90b25926ca..aac9156e79 100644 --- a/ui/vnc.h +++ b/ui/vnc.h @@ -181,6 +181,8 @@ struct VncDisplay time_t expires; int auth; int subauth; /* Used by VeNCrypt */ + int ws_auth; /* Used by websockets */ + bool ws_tls; /* Used by websockets */ bool lossy; bool non_adaptive; #ifdef CONFIG_VNC_TLS From 51941e4695c6f6c1f786bacef7e8c3a477570e04 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 17 Mar 2015 13:42:58 +0000 Subject: [PATCH 7/9] ui: enforce TLS when using websockets server When TLS is required, the primary VNC server considers it to be mandatory. ie the server admin decides whether or not TLS is used, and the client has to comply with this decision. The websockets server, however, treated it as optional, allowing non-TLS clients to connect to a server which had setup TLS. Thus enabling websockets lowers the security of the VNC server leaving the admin no way to enforce use of TLS. This removes the code that allows non-TLS fallback in the websockets server, so that if TLS is requested for VNC it is now mandatory for both the primary VNC server and the websockets VNC server. Signed-off-by: Daniel P. Berrange Signed-off-by: Gerd Hoffmann --- ui/vnc-ws.c | 31 +++++++------------------------ ui/vnc-ws.h | 2 +- ui/vnc.c | 2 +- 3 files changed, 9 insertions(+), 26 deletions(-) diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c index 1769d526f8..0fcce4e378 100644 --- a/ui/vnc-ws.c +++ b/ui/vnc-ws.c @@ -24,8 +24,6 @@ #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); @@ -53,34 +51,19 @@ 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; + if (!vs->tls.session) { + VNC_DEBUG("TLS Websocket setup\n"); + if (vnc_tls_client_setup(vs, vs->vd->tls.x509cert != NULL) < 0) { + return; + } + } 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); - } - } else { - qemu_set_fd_handler2(vs->csock, NULL, vncws_handshake_read, NULL, vs); - } -} #endif /* CONFIG_VNC_TLS */ void vncws_handshake_read(void *opaque) diff --git a/ui/vnc-ws.h b/ui/vnc-ws.h index 95c1b0aeae..ef229b7c0c 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 7b66f93595..9994de10c0 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -3036,7 +3036,7 @@ static void vnc_connect(VncDisplay *vd, int csock, vs->websocket = 1; #ifdef CONFIG_VNC_TLS if (vd->ws_tls) { - qemu_set_fd_handler2(vs->csock, NULL, vncws_tls_handshake_peek, + qemu_set_fd_handler2(vs->csock, NULL, vncws_tls_handshake_io, NULL, vs); } else #endif /* CONFIG_VNC_TLS */ From 7b45a00d05cc936d28e36b95932864e8cc095968 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 17 Mar 2015 13:42:59 +0000 Subject: [PATCH 8/9] ui: remove separate gnutls_session for websockets server The previous change to the auth scheme handling guarantees we can never have nested TLS sessions in the VNC websockets server. Thus we can remove the separate gnutls_session instance. Signed-off-by: Daniel P. Berrange Signed-off-by: Gerd Hoffmann --- ui/vnc-tls.c | 70 ++++++++++++++++++++++------------------------------ ui/vnc-ws.c | 4 +-- ui/vnc.c | 18 ++------------ ui/vnc.h | 3 --- 4 files changed, 33 insertions(+), 62 deletions(-) diff --git a/ui/vnc-tls.c b/ui/vnc-tls.c index de1cb342a4..eddd39b08e 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 0fcce4e378..5f9fcc42db 100644 --- a/ui/vnc-ws.c +++ b/ui/vnc-ws.c @@ -26,12 +26,12 @@ 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 { diff --git a/ui/vnc.c b/ui/vnc.c index 9994de10c0..cffb5b74b3 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 */ diff --git a/ui/vnc.h b/ui/vnc.h index aac9156e79..e19ac396f2 100644 --- a/ui/vnc.h +++ b/ui/vnc.h @@ -295,9 +295,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 */ From 4a48aaa9f52dbac148be24f591de2f28c58ccb5d Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 17 Mar 2015 13:43:00 +0000 Subject: [PATCH 9/9] ui: ensure VNC websockets server checks the ACL if requested If the x509verify option is requested, the VNC websockets server was failing to validate that the websockets client provided an x509 certificate matching the ACL rules. Signed-off-by: Daniel P. Berrange Signed-off-by: Gerd Hoffmann --- ui/vnc-ws.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c index 5f9fcc42db..85dbb7e6ae 100644 --- a/ui/vnc-ws.c +++ b/ui/vnc-ws.c @@ -45,6 +45,16 @@ static int vncws_start_tls_handshake(struct VncState *vs) return -1; } + if (vs->vd->tls.x509verify) { + if (vnc_tls_validate_certificate(vs) < 0) { + VNC_DEBUG("Client verification failed\n"); + vnc_client_error(vs); + return -1; + } else { + VNC_DEBUG("Client verification passed\n"); + } + } + VNC_DEBUG("Handshake done, switching to TLS data mode\n"); qemu_set_fd_handler2(vs->csock, NULL, vncws_handshake_read, NULL, vs);