* [Qemu-devel] [PATCH v2 1/8] ui: remove unused 'wiremode' variable in VncState struct
2015-03-17 13:42 [Qemu-devel] [PATCH v2 0/8] Misc fixes for VNC Daniel P. Berrange
@ 2015-03-17 13:42 ` Daniel P. Berrange
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 2/8] ui: replace printf() calls with VNC_DEBUG Daniel P. Berrange
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrange @ 2015-03-17 13:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
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 bc7032e..a420ccb 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 0f59f9b..de1cb34 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 36a2227..f9829c7 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 d75950d..1769d52 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;
--
2.1.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2 2/8] ui: replace printf() calls with VNC_DEBUG
2015-03-17 13:42 [Qemu-devel] [PATCH v2 0/8] Misc fixes for VNC Daniel P. Berrange
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 1/8] ui: remove unused 'wiremode' variable in VncState struct Daniel P. Berrange
@ 2015-03-17 13:42 ` Daniel P. Berrange
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 3/8] ui: report error if user requests VNC option that is unsupported Daniel P. Berrange
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrange @ 2015-03-17 13:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
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 <berrange@redhat.com>
---
ui/vnc.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 6f9b718..80dc63b 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;
}
--
2.1.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2 3/8] ui: report error if user requests VNC option that is unsupported
2015-03-17 13:42 [Qemu-devel] [PATCH v2 0/8] Misc fixes for VNC Daniel P. Berrange
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 1/8] ui: remove unused 'wiremode' variable in VncState struct Daniel P. Berrange
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 2/8] ui: replace printf() calls with VNC_DEBUG Daniel P. Berrange
@ 2015-03-17 13:42 ` Daniel P. Berrange
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 4/8] ui: split setup of VNC auth scheme into separate method Daniel P. Berrange
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrange @ 2015-03-17 13:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
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 <berrange@redhat.com>
---
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 80dc63b..903ed8b 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 66a0298..90b2592 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
--
2.1.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2 4/8] ui: split setup of VNC auth scheme into separate method
2015-03-17 13:42 [Qemu-devel] [PATCH v2 0/8] Misc fixes for VNC Daniel P. Berrange
` (2 preceding siblings ...)
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 3/8] ui: report error if user requests VNC option that is unsupported Daniel P. Berrange
@ 2015-03-17 13:42 ` Daniel P. Berrange
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 5/8] ui: fix setup of VNC websockets auth scheme with TLS Daniel P. Berrange
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrange @ 2015-03-17 13:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
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 <berrange@redhat.com>
---
ui/vnc.c | 153 +++++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 91 insertions(+), 62 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 903ed8b..a7081b3 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) {
--
2.1.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2 5/8] ui: fix setup of VNC websockets auth scheme with TLS
2015-03-17 13:42 [Qemu-devel] [PATCH v2 0/8] Misc fixes for VNC Daniel P. Berrange
` (3 preceding siblings ...)
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 4/8] ui: split setup of VNC auth scheme into separate method Daniel P. Berrange
@ 2015-03-17 13:42 ` Daniel P. Berrange
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 6/8] ui: enforce TLS when using websockets server Daniel P. Berrange
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrange @ 2015-03-17 13:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
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 <berrange@redhat.com>
---
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 a7081b3..caa47f0 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 90b2592..aac9156 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
--
2.1.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2 6/8] ui: enforce TLS when using websockets server
2015-03-17 13:42 [Qemu-devel] [PATCH v2 0/8] Misc fixes for VNC Daniel P. Berrange
` (4 preceding siblings ...)
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 5/8] ui: fix setup of VNC websockets auth scheme with TLS Daniel P. Berrange
@ 2015-03-17 13:42 ` Daniel P. Berrange
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 7/8] ui: remove separate gnutls_session for " Daniel P. Berrange
2015-03-17 13:43 ` [Qemu-devel] [PATCH v2 8/8] ui: ensure VNC websockets server checks the ACL if requested Daniel P. Berrange
7 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrange @ 2015-03-17 13:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
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 <berrange@redhat.com>
---
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 1769d52..0fcce4e 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,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 caa47f0..ed01cef 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 */
--
2.1.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2 7/8] ui: remove separate gnutls_session for websockets server
2015-03-17 13:42 [Qemu-devel] [PATCH v2 0/8] Misc fixes for VNC Daniel P. Berrange
` (5 preceding siblings ...)
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 6/8] ui: enforce TLS when using websockets server Daniel P. Berrange
@ 2015-03-17 13:42 ` Daniel P. Berrange
2015-03-17 13:43 ` [Qemu-devel] [PATCH v2 8/8] ui: ensure VNC websockets server checks the ACL if requested Daniel P. Berrange
7 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrange @ 2015-03-17 13:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
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 <berrange@redhat.com>
---
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 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 0fcce4e..5f9fcc4 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 ed01cef..7f45daf 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 aac9156..e19ac39 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 */
--
2.1.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2 8/8] ui: ensure VNC websockets server checks the ACL if requested
2015-03-17 13:42 [Qemu-devel] [PATCH v2 0/8] Misc fixes for VNC Daniel P. Berrange
` (6 preceding siblings ...)
2015-03-17 13:42 ` [Qemu-devel] [PATCH v2 7/8] ui: remove separate gnutls_session for " Daniel P. Berrange
@ 2015-03-17 13:43 ` Daniel P. Berrange
7 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrange @ 2015-03-17 13:43 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann
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 <berrange@redhat.com>
---
ui/vnc-ws.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/ui/vnc-ws.c b/ui/vnc-ws.c
index 5f9fcc4..85dbb7e 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);
--
2.1.0
^ permalink raw reply related [flat|nested] 9+ messages in thread