CVE-2020-14398: Loop with Unreachable Exit Condition ('Infinite Loop') (CWE-835)
value: An issue was discovered in LibVNCServer before 0.9.13. An improperly closed TCP connection causes an infinite loop in libvncclient/sockets.c.
CWE description: The product contains an iteration or loop with an exit condition that cannot be reached, i.e., an infinite loop.
CVSS v3 base score: 7.5
Published: 2020-06-17T16:15Z
Repository: https://github.com/LibVNC/libvncserver
Language: C
Files changed: libvncclient/sockets.c, libvncclient/vncviewer.c, rfb/rfbclient.h
Commit date: 2020-04-24 15:52:05 +0200
Commit message: libvncclient: handle half-open TCP connections

When a connection is not reset properly at the TCP level (e.g. sudden
power loss or process crash) the TCP connection becomes half-open and
read() always returns -1 with errno = EAGAIN while select() always
returns 0. This leads to an infinite loop and can be fixed by closing
the connection after a certain number of retries (based on a timeout)
has been exceeded.

Diff: diff --git a/libvncclient/sockets.c b/libvncclient/sockets.c
index 40e6e8a0c..7ce6c990d 100644
--- a/libvncclient/sockets.c
+++ b/libvncclient/sockets.c
@@ -60,10 +60,12 @@ rfbBool errorMessageOnReadFailure = TRUE;
  */
 
 rfbBool
 ReadFromRFBServer(rfbClient* client, char *out, unsigned int n)
 {
+  const int USECS_WAIT_PER_RETRY = 100000;
+  int retries = 0;
 #undef DEBUG_READ_EXACT
 #ifdef DEBUG_READ_EXACT
 	char* oout=out;
 	unsigned int nn=n;
 	rfbClientLog("ReadFromRFBServer %d bytes\n",n);
@@ -149,14 +151,20 @@ ReadFromRFBServer(rfbClient* client, char *out, unsigned int n)
 #endif
   
       if (i <= 0) {
 	if (i < 0) {
 	  if (errno == EWOULDBLOCK || errno == EAGAIN) {
+	    if (client->readTimeout > 0 &&
+		++retries > (client->readTimeout * 1000 * 1000 / USECS_WAIT_PER_RETRY))
+	    {
+	      rfbClientLog("Connection timed out\n");
+	      return FALSE;
+	    }
 	    /* TODO:
 	       ProcessXtEvents();
 	    */
-	    WaitForMessage(client, 100000);
+	    WaitForMessage(client, USECS_WAIT_PER_RETRY);
 	    i = 0;
 	  } else {
 	    rfbClientErr("read (%d: %s)\n",errno,strerror(errno));
 	    return FALSE;
 	  }
@@ -192,14 +200,20 @@ ReadFromRFBServer(rfbClient* client, char *out, unsigned int n)
 	if (i < 0) {
 #ifdef WIN32
 	  errno=WSAGetLastError();
 #endif
 	  if (errno == EWOULDBLOCK || errno == EAGAIN) {
+	    if (client->readTimeout > 0 &&
+		++retries > (client->readTimeout * 1000 * 1000 / USECS_WAIT_PER_RETRY))
+	    {
+		rfbClientLog("Connection timed out\n");
+		return FALSE;
+	    }
 	    /* TODO:
 	       ProcessXtEvents();
 	    */
-	    WaitForMessage(client, 100000);
+	    WaitForMessage(client, USECS_WAIT_PER_RETRY);
 	    i = 0;
 	  } else {
 	    rfbClientErr("read (%s)\n",strerror(errno));
 	    return FALSE;
 	  }
diff --git a/libvncclient/vncviewer.c b/libvncclient/vncviewer.c
index a41464b30..7d08e1233 100644
--- a/libvncclient/vncviewer.c
+++ b/libvncclient/vncviewer.c
@@ -270,10 +270,11 @@ rfbClient* rfbGetClient(int bitsPerSample,int samplesPerPixel,
 
   client->destHost = NULL;
   client->destPort = 5900;
   
   client->connectTimeout = DEFAULT_CONNECT_TIMEOUT;
+  client->readTimeout = DEFAULT_READ_TIMEOUT;
 
   client->CurrentKeyboardLedState = 0;
   client->HandleKeyboardLedState = (HandleKeyboardLedStateProc)DummyPoint;
 
   /* default: use complete frame buffer */ 
diff --git a/rfb/rfbclient.h b/rfb/rfbclient.h
index 9fd48aca0..4042d76f2 100644
--- a/rfb/rfbclient.h
+++ b/rfb/rfbclient.h
@@ -83,10 +83,11 @@
 #define LISTEN_PORT_OFFSET 5500
 #define TUNNEL_PORT_OFFSET 5500
 #define SERVER_PORT_OFFSET 5900
 
 #define DEFAULT_CONNECT_TIMEOUT 60
+#define DEFAULT_READ_TIMEOUT 0
 
 #define DEFAULT_SSH_CMD "/usr/bin/ssh"
 #define DEFAULT_TUNNEL_CMD  \
   (DEFAULT_SSH_CMD " -f -L %L:localhost:%R %H sleep 20")
 #define DEFAULT_VIA_CMD     \
@@ -452,10 +453,13 @@ typedef struct _rfbClient {
 
 #endif
 #endif
 	/* timeout in seconds for select() after connect() */
 	unsigned int connectTimeout;
+	/* timeout in seconds when reading from half-open connections in
+	 * ReadFromRFBServer() - keep at 0 to disable timeout detection and handling */
+	unsigned int readTimeout;
 } rfbClient;
 
 /* cursor.c */
 /**
  * Handles XCursor and RichCursor shape updates from the server.

Vulnerable code: 	    WaitForMessage(client, 100000);
	    WaitForMessage(client, 100000);
Fixed code:   const int USECS_WAIT_PER_RETRY = 100000;
  int retries = 0;
	    if (client->readTimeout > 0 &&
		++retries > (client->readTimeout * 1000 * 1000 / USECS_WAIT_PER_RETRY))
	    {
	      rfbClientLog("Connection timed out\n");
	      return FALSE;
	    }
	    WaitForMessage(client, USECS_WAIT_PER_RETRY);
	    if (client->readTimeout > 0 &&
		++retries > (client->readTimeout * 1000 * 1000 / USECS_WAIT_PER_RETRY))
	    {
		rfbClientLog("Connection timed out\n");
		return FALSE;
	    }
	    WaitForMessage(client, USECS_WAIT_PER_RETRY);
  client->readTimeout = DEFAULT_READ_TIMEOUT;
#define DEFAULT_READ_TIMEOUT 0
	/* timeout in seconds when reading from half-open connections in
	 * ReadFromRFBServer() - keep at 0 to disable timeout detection and handling */
	unsigned int readTimeout;