From 05d3d94613168187cbf7d54ac6de345bb75910dd Mon Sep 17 00:00:00 2001 From: Luke Bratch Date: Tue, 9 Jul 2019 22:36:35 +0100 Subject: Avoid SSL_accept() blocking if the client fails to do TLS negotiation. --- TODO | 7 +++++++ blabouncer.c | 51 +++++++++++++++++++++++++++-------------------- functions.c | 1 + sockets.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ sockets.h | 12 +++++++++++ structures.h | 1 + 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 #include #include +#include #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 -- cgit v1.2.3