Avoid SSL_accept() blocking if the client fails to do TLS negotiation.

6 files changed, 116 insertions, 21 deletions

diff --git a/TODO b/TODO
index e69de29..f0b21b6 100644
--- a/TODO
+++ b/TODO
@@ -0,0 +1,7 @@
+Make some unnecessary configuration options optional (e.g. replaytime if replaymode != "time")
+
+Fuzzing.
+
+If openssl_error_string() doesn't get a string just after SSL_accept() then the debug log entry will fail to contain a trailing new line - maybe make log function ensure last character is new line?
+
+Rename selret to pselret.
diff --git a/blabouncer.c b/blabouncer.c
index 73bef8c..3a5924f 100644
--- a/blabouncer.c
+++ b/blabouncer.c
@@ -387,6 +387,7 @@ void dochat(int *serversockfd, int *clientsockfd, struct settings *settings) {
     clients[i].fd = 0;
     clients[i].authed = 0;
     clients[i].registered = 0;
+    clients[i].pendingsslaccept = 0;
     clients[i].pendingchannelmode = 0;
     clients[i].pendingban = 0;
     clients[i].pendingwho = 0;
@@ -739,30 +740,32 @@ void dochat(int *serversockfd, int *clientsockfd, struct settings *settings) {
             if (newfd > fdmax) {    // keep track of the max
               fdmax = newfd;
             }
-            // Find a free element in the clients array and set to new fd value
+            // Find a free element in the clients array and set to new fd value (plus start SSL_accept() if using client TLS)
             for (int j = 0; j < MAXCLIENTS; j++) {
-                if (clients[j].fd == 0) {
-                  clients[j].fd = newfd;
-                  // Ensure its authentication status is set to 0
-                  clients[j].authed = 0;
-                  // If using TLS then...
-                  if (settings->clienttls) {
-                    // ...set as OpenSSL FD and SSL_accept it
-                    clients[j].ssl = SSL_new(ctx);
-                    SSL_set_fd(clients[j].ssl, newfd);
-                    if (SSL_accept(clients[j].ssl) <= 0) {
-                      char* errstr = openssl_error_string();
-                      debugprint(DEBUG_CRIT, "SSL_accept failed for fd %d - %s", clients[j].fd, errstr);
-                      if (errstr != NULL) free(errstr);
-                    } else {
-                      debugprint(DEBUG_FULL, "SSL_accept succeeded for fd %d.\n", clients[j].fd);
-                    }
-                  } else {
-                    // If not using TLS then cast newfd to SSL* even though it will just be the original newfd int really
-                    clients[j].ssl = (SSL*)(long int)newfd;
+              if (clients[j].fd == 0) {
+                clients[j].fd = newfd;
+                // Ensure its authentication status is set to 0
+                clients[j].authed = 0;
+                // If using TLS then...
+                if (settings->clienttls) {
+                  // ...set as OpenSSL FD and SSL_accept it
+                  clients[j].ssl = SSL_new(ctx);
+                  SSL_set_fd(clients[j].ssl, newfd);
+                  // Set that we are pending SSL_accept()
+                  clients[j].pendingsslaccept = 1;
+                  // Change the socket to non-blocking so SSL_accept() can't hang (e.g. if it disconnects mid-conversation or if it's some bad client)
+                  if (!fd_toggle_blocking(clients[j].fd, 0)) {
+                    debugprint(DEBUG_CRIT, "fd_toggle_blocking on failed for fd %d: %s.\n", clients[j].fd, strerror(errno));
+                    disconnectclient(clients[j].fd, clients, &ircdstate, settings, clientcodes);
                   }
-                  break;
+                  // Try to SSL_accept(), not interested in return code here since openssl_accept() does the right thing.
+                  openssl_accept(clients[j].fd, clients, &ircdstate, settings, clientcodes);
+                } else {
+                  // If not using TLS then cast newfd to SSL* even though it will just be the original newfd int really
+                  clients[j].ssl = (SSL*)(long int)newfd;
                 }
+                break;
+              }
             }
             // TODO - Handle the "find a free element" loop not finding a free element
             debugprint(DEBUG_FULL, "bouncer-client: new connection from %s on socket %d\n",
@@ -780,8 +783,14 @@ void dochat(int *serversockfd, int *clientsockfd, struct settings *settings) {
             sendtoallclients(clients, alertmsg, 0, settings);
             debugprint(DEBUG_FULL, "bouncer-client: total client connections: %d\n", numclients(clients));
           }
+        // If using client TLS and still pending SSL_accept() then re-try SSL_accept() (it can't be real client data yet)
+        } else if (clients[arrindex(clients, i)].pendingsslaccept) {
+          debugprint(DEBUG_FULL, "...previous connection pending SSL_accept!\n");
+          // Try to SSL_accept(), not interested in return code here since openssl_accept() does the right thing.
+          openssl_accept(clients[arrindex(clients, i)].fd, clients, &ircdstate, settings, clientcodes);
         } else {
           debugprint(DEBUG_FULL, "...previous connection!\n");
+
           // handle data from a client
           if ((clientnumbytes = sockread(clients[arrindex(clients, i)].ssl, clientbuf, MAXRCVSIZE - 1, settings->clienttls)) <= 0) {
             // got error or connection closed by client
diff --git a/functions.c b/functions.c
index f8609b1..ad92dd3 100644
--- a/functions.c
+++ b/functions.c
@@ -561,6 +561,7 @@ int disconnectclient(int fd, struct client *clients, struct ircdstate *ircdstate
       clients[i].fd = 0;
       clients[i].authed = 0;
       clients[i].registered = 0;
+      clients[i].pendingsslaccept = 0;
       clients[i].pendingchannelmode = 0;
       clients[i].pendingban = 0;
       clients[i].pendingwho = 0;
diff --git a/sockets.c b/sockets.c
index bf83176..d10811e 100644
--- a/sockets.c
+++ b/sockets.c
@@ -228,6 +228,7 @@ int socksend(SSL *fd, char *buf, int bufsize, int tls) {
   }
 }
 
+// Return character array of latest OpenSSL error
 char *openssl_error_string() {
   BIO *bio = BIO_new (BIO_s_mem ());
   ERR_print_errors (bio);
@@ -240,3 +241,67 @@ char *openssl_error_string() {
   BIO_free (bio);
   return ret;
 }
+
+// Set a socket "fd" to be blocking ("blocking" = 1) or non-blocking ("blocking" = 0).
+// Returns 1 on success or 0 on failure.
+int fd_toggle_blocking(int fd, int blocking) {
+  debugprint(DEBUG_FULL, "fd_toggle_blocking(): setting blocking to %d for fd %d.\n", blocking, fd);
+
+  // Save the current flags
+  int flags = fcntl(fd, F_GETFL, 0);
+  if (flags == -1) {
+    // Error getting current flags
+    return 0;
+  }
+
+  // Add or remove O_NONBLOCK as appropriate
+  if (blocking) {
+    flags &= ~O_NONBLOCK;
+  } else {
+    flags |= O_NONBLOCK;
+  }
+
+  if (fcntl(fd, F_SETFL, flags) == -1) {
+    return 0;
+  } else {
+    return 1;
+  }
+}
+
+// Attempt to do SSL_accept() on a client with fd "fd".  Expects the socket fd to have just been set
+// to non-blocking.  Will make the socket blocking again and set the client's pendingsslaccept status
+// to 0 if SSL_accept() succeeds.  Calls disconnectclient() on hard failure.
+// Returns 1 on success, 0 on hard failure, or -1 on SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE.
+int openssl_accept(int fd, struct client *clients, struct ircdstate *ircdstate, struct settings *settings, struct clientcodes *clientcodes) {
+  // Get the index of the this client fd
+  int clientindex = arrindex(clients, fd);
+
+  // Clear OpenSSL errors before proceeding so we can reliably get errors from SSL_accept
+  ERR_clear_error();
+  // Try to SSL_accept();
+  int ret;
+  if ((ret = SSL_accept(clients[clientindex].ssl)) <= 0) {
+    // SSL_accept() either failed (bad) or is just pending read or write (which is OK)
+    int sslerr = SSL_get_error(clients[clientindex].ssl, ret);
+    if (sslerr == SSL_ERROR_WANT_READ || sslerr == SSL_ERROR_WANT_WRITE) {
+      debugprint(DEBUG_CRIT, "SSL_accept() pending for new connection fd %d (ret = %d, sslerr = %d), looping...\n", clients[clientindex].fd, ret, sslerr);
+      return -1;
+    } else {
+      char* errstr = openssl_error_string();
+      debugprint(DEBUG_CRIT, "SSL_accept failed for new connection fd %d (ret = %d) - %s", clients[clientindex].fd, ret, errstr);
+      if (errstr != NULL) free(errstr);
+      disconnectclient(clients[clientindex].fd, clients, ircdstate, settings, clientcodes);
+      return 0;
+    }
+  } else {
+    debugprint(DEBUG_FULL, "SSL_accept succeeded for new connection fd %d.\n", clients[clientindex].fd);
+    // Change the socket back to blocking
+    if (!fd_toggle_blocking(clients[clientindex].fd, 1)) {
+      debugprint(DEBUG_CRIT, "fd_toggle_blocking off failed for fd %d: %s.\n", clients[clientindex].fd, strerror(errno));
+      disconnectclient(clients[clientindex].fd, clients, ircdstate, settings, clientcodes);
+    }
+    // And mark as no longer pending SSL_accept()
+    clients[clientindex].pendingsslaccept = 0;
+    return 1;
+  }
+}
diff --git a/sockets.h b/sockets.h
index 4fb6c20..2a2ef1b 100644
--- a/sockets.h
+++ b/sockets.h
@@ -31,8 +31,10 @@
 #include <sys/select.h>
 #include <openssl/ssl.h>
 #include <openssl/err.h>
+#include <fcntl.h>
 
 #include "functions.h"
+#include "structures.h"
 
 #define DEBUG_CRIT 0
 #define DEBUG_SOME 1
@@ -71,4 +73,14 @@ int socksend(SSL *fd, char *buf, int bufsize, int tls);
 // Return character array of latest OpenSSL error
 char *openssl_error_string();
 
+// Set a socket "fd" to be blocking ("blocking" = 1) or non-blocking ("blocking" = 0).
+// Returns 1 on success or 0 on failure.
+int fd_toggle_blocking(int fd, int blocking);
+
+// Attempt to do SSL_accept() on a client with fd "fd".  Expects the socket fd to have just been set
+// to non-blocking.  Will make the socket blocking again and set the client's pendingsslaccept status
+// to 0 if SSL_accept() succeeds.
+// Returns 1 on success, 0 on hard failure, or -1 on SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE.
+int openssl_accept(int fd, struct client *clients, struct ircdstate *ircdstate, struct settings *settings, struct clientcodes *clientcodes);
+
 #endif
diff --git a/structures.h b/structures.h
index b96b781..d9b1f7f 100644
--- a/structures.h
+++ b/structures.h
@@ -88,6 +88,7 @@ struct client {
   int fd; // Client socket fd - 0 means not connected, greater than 0 means connected and the value is the fd number (so we know which ones to try to read and send to)
   int authed; // Client authentication status - 0 means not authenticated, 1 means authenticated.
   SSL *ssl; // OpenSSL structures when using TLS, or faked by casting fd ints to SSL* if not. - TODO - Can we drop one of either "int fd" or "SSL *ssl" now?
+  int pendingsslaccept; // Whether the client is still pending SSL_accept() completing (when in client TLS mode only)
   int registered; // Whether the client has finished registering with the bouncer
   int pendingchannelmode; // Whether the client is waiting to hear back from a "MODE #channel" command
   int pendingban; // Whether the client is waiting to hear back from a "MODE #channel b" command