[gtk-vnc-devel] PATCH: Fix handling of TLS

["Daniel P. Berrange" <berrange redhat com>]

For some reason I can't now believe, when I wrote the TLS support for GTK-VNC
I made it do the IO yield inside our gvnc_tls_push/pull functions. We were
lucky and this worked before. Now that we have interruptable sleeps though,
we could get interrupted while in our push/pull functions, and then call
back into more GNU TLS APIs. This is totally disasterous because they're
not intended to be re-entrant safe in this way. The fix is trivial, just
do the IO yield based on the gnutls_read/gnutls_write function return status.
In doing this we also need to yeild if the gnutls_handshake function blocks.
The attached patch fixes this, and removes the hardcoded 'foo' for the cert
hostname check. So TLS now works correctly & I've confirmed Anthony's patch
for interruptable waits is working as planned.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

diff -r f5d4e8c6c0bf src/gvnc.c
--- a/src/gvnc.c	Fri Jul 20 16:53:53 2007 -0400
+++ b/src/gvnc.c	Thu Jul 26 12:34:40 2007 -0400
@@ -129,10 +129,12 @@ struct gvnc
 #if DEBUG
 #define GVNC_DEBUG(fmt, ...) do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
 
+#if DEBUG == 2
 static void debug_log(int level, const char* str)
 {
 	GVNC_DEBUG("%d %s", level, str);
 }
+#endif
 
 #else
 #define GVNC_DEBUG(fmt, ...) do { } while (0)
@@ -278,26 +280,29 @@ static int gvnc_read(struct gvnc *gvnc, 
 			if (gvnc->tls_session) {
 				ret = gnutls_read(gvnc->tls_session, gvnc->read_buffer, 4096);
 				if (ret < 0) {
+					if (ret == GNUTLS_E_AGAIN)
+						errno = EAGAIN;
+					else
+						errno = EIO;
+					ret = -1;
+				}
+			} else
+				ret = read(fd, gvnc->read_buffer, 4096);
+
+			if (ret == -1) {
+				switch (errno) {
+				case EAGAIN:
+					if (gvnc->wait_interruptable) {
+						if (!g_io_wait_interruptable(&gvnc->wait,
+									     gvnc->channel, G_IO_IN))
+							return -EAGAIN;
+					} else
+						g_io_wait(gvnc->channel, G_IO_IN);
+				case EINTR:
+					continue;
+				default:
 					gvnc->has_error = TRUE;
-					return ret;
-				}
-			} else {
-				ret = read(fd, gvnc->read_buffer, 4096);
-				if (ret == -1) {
-					switch (errno) {
-					case EAGAIN:
-						if (gvnc->wait_interruptable) {
-							if (!g_io_wait_interruptable(&gvnc->wait,
-										     gvnc->channel, G_IO_IN))
-								return -EAGAIN;
-						} else
-							g_io_wait(gvnc->channel, G_IO_IN);
-					case EINTR:
-						continue;
-					default:
-						gvnc->has_error = TRUE;
-						return -errno;
-					}
+					return -errno;
 				}
 			}
 			if (ret == 0) {
@@ -332,23 +337,25 @@ static void gvnc_flush(struct gvnc *gvnc
 					   gvnc->write_buffer+offset,
 					   gvnc->write_offset-offset);
 			if (ret < 0) {
-				gvnc->has_error = TRUE;
-				return;
+				if (ret == GNUTLS_E_AGAIN)
+					errno = EAGAIN;
+				else
+					errno = EIO;
+				ret = -1;
 			}
-		} else {
+		} else
 			ret = write(fd,
 				    gvnc->write_buffer+offset,
 				    gvnc->write_offset-offset);
-			if (ret == -1) {
-				switch (errno) {
-				case EAGAIN:
-					g_io_wait(gvnc->channel, G_IO_OUT);
-				case EINTR:
-					continue;
-				default:
-					gvnc->has_error = TRUE;
-					return;
-				}
+		if (ret == -1) {
+			switch (errno) {
+			case EAGAIN:
+				g_io_wait(gvnc->channel, G_IO_OUT);
+			case EINTR:
+				continue;
+			default:
+				gvnc->has_error = TRUE;
+				return;
 			}
 		}
 		if (ret == 0) {
@@ -388,25 +395,15 @@ static ssize_t gvnc_tls_push(gnutls_tran
 	struct gvnc *gvnc = (struct gvnc *)transport;
 	int fd = g_io_channel_unix_get_fd(gvnc->channel);
 	int ret;
-	size_t sent = 0;
-
-	while (sent < len) {
-		ret = write(fd, data+sent, len-sent);
-		if (ret == -1 && errno == EINTR)
-			continue;
-		if (ret == -1 && errno == EAGAIN) {
-			g_io_wait(gvnc->channel, G_IO_OUT);
-			continue;
-		}
-		if (ret <= 0) {
-			gvnc->has_error = TRUE;
-			return -1;
-		} else {
-			sent += ret;
-		}
-	}
-
-	return len;
+
+ retry:
+	ret = write(fd, data, len);
+	if (ret < 0) {
+		if (errno == EINTR)
+			goto retry;
+		return -1;
+	}
+	return ret;
 }
 
 
@@ -416,25 +413,15 @@ static ssize_t gvnc_tls_pull(gnutls_tran
 	struct gvnc *gvnc = (struct gvnc *)transport;
 	int fd = g_io_channel_unix_get_fd(gvnc->channel);
 	int ret;
-	size_t got = 0;
-
-	while (got < len) {
-		ret = read(fd, data+got, len-got);
-		if (ret == -1 && errno == EINTR)
-			continue;
-		if (ret == -1 && errno == EAGAIN) {
-			g_io_wait(gvnc->channel, G_IO_IN);
-			continue;
-		}
-		if (ret <= 0) {
-			gvnc->has_error = TRUE;
-			return -1;
-		} else {
-			got += ret;
-		}
-	}
-
-	return len;
+
+ retry:
+	ret = read(fd, data, len);
+	if (ret < 0) {
+		if (errno == EINTR)
+			goto retry;
+		return -1;
+	}
+	return ret;
 }
 
 
@@ -519,7 +506,7 @@ static gboolean gvnc_tls_initialize(void
 	if (gnutls_dh_params_generate2 (dh_params, DH_BITS) < 0)
 		return FALSE;
 
-#if DEBUG
+#if DEBUG == 2
 	gnutls_global_set_log_level(10);
 	gnutls_global_set_log_function(debug_log);
 #endif
@@ -651,11 +638,14 @@ static int gvnc_validate_certificate(str
 		}
 
 		if (i == 0) {
-			/* XXX Fixme */
-			const char *hostname = "foo";
-			if (!gnutls_x509_crt_check_hostname (cert, hostname)) {
+			if (!vnc->host) {
+				GVNC_DEBUG ("No hostname provided for certificate verification\n");
+				gnutls_x509_crt_deinit (cert);
+				return FALSE;
+			}
+			if (!gnutls_x509_crt_check_hostname (cert, vnc->host)) {
 				GVNC_DEBUG ("The certificate's owner does not match hostname '%s'\n",
-					    hostname);
+					    vnc->host);
 				gnutls_x509_crt_deinit (cert);
 				return FALSE;
 			}
@@ -1304,7 +1294,16 @@ static gboolean gvnc_start_tls(struct gv
 		gnutls_transport_set_pull_function(gvnc->tls_session, gvnc_tls_pull);
 	}
 
+ retry:
 	if ((ret = gnutls_handshake(gvnc->tls_session)) < 0) {
+		if (!gnutls_error_is_fatal(ret)) {
+			GVNC_DEBUG("Handshake was blocking\n");
+			if (!gnutls_record_get_direction(gvnc->tls_session))
+				g_io_wait(gvnc->channel, G_IO_IN);
+			else
+				g_io_wait(gvnc->channel, G_IO_OUT);
+			goto retry;
+		}
 		GVNC_DEBUG("Handshake failed %s\n", gnutls_strerror(ret));
 		gnutls_deinit(gvnc->tls_session);
 		gvnc->tls_session = NULL;