From 82ab48d41bfef30ab51b407b48dfcda9ebc5f7e7 Mon Sep 17 00:00:00 2001 From: Luke Bratch Date: Sun, 5 Jan 2020 22:25:22 +0000 Subject: Fix some situations where the remote IP of a connecting/disconnecting client is wrong in the debug log and NOTICEs. --- TODO | 2 +- blabouncer.c | 31 ++++++++++++++++++++++--------- functions.c | 1 + message.c | 9 ++++++--- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/TODO b/TODO index 2f7e060..8d8d80c 100644 --- a/TODO +++ b/TODO @@ -25,4 +25,4 @@ Can memory usage be reduced further? (e.g. better channel struct management) Ability to load new certificate whilst running. -Disconnecting IP address announced is not always correct. +arrindex() shouldn't return 0 on failure as 0 is a valid index. Instead return -1 and change callers to check this. diff --git a/blabouncer.c b/blabouncer.c index 5ee48b5..c4e92ed 100644 --- a/blabouncer.c +++ b/blabouncer.c @@ -758,12 +758,24 @@ void dochat(int *serversockfd, int *clientsockfd, struct settings *settings) { if (newfd > fdmax) { // keep track of the max fdmax = newfd; } + + // If openssl_accept fails later on, this is the message we'll append to the usual connection announcement + char opensslfailmsg[] = ", but was disconnected due to a failure in SSL_accept()"; + // Store the client's IP address for now, since we may need to refer to it after disconnecting + // them (thus clearing the array entry that the IP is read from) if something goes wrong + char remoteip[INET6_ADDRSTRLEN]; + strncpy(remoteip, inet_ntop(remoteaddr.ss_family, get_in_addr((struct sockaddr*)&remoteaddr), remoteIP, INET6_ADDRSTRLEN), INET6_ADDRSTRLEN); + // 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; + + // Record the remote IP address of this client in the clients array + strncpy(clients[j].remoteip, remoteip, INET6_ADDRSTRLEN); + // If using TLS then... if (settings->clienttls) { // ...set as OpenSSL FD and SSL_accept it @@ -776,27 +788,28 @@ void dochat(int *serversockfd, int *clientsockfd, struct settings *settings) { 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); } - // 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); + // Try to SSL_accept() + if (openssl_accept(clients[j].fd, clients, &ircdstate, settings, clientcodes)) { + // It succeeded, so clear the failure message + opensslfailmsg[0] = '\0'; + } } 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; + // There can't be an openssl_accept failure if we're not using TLS + opensslfailmsg[0] = '\0'; } - // Record the remote IP address of this client in the clients array - strncpy(clients[j].remoteip, inet_ntop(remoteaddr.ss_family, get_in_addr((struct sockaddr*)&remoteaddr), remoteIP, INET6_ADDRSTRLEN), INET6_ADDRSTRLEN); - 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", - clients[arrindex(clients, newfd)].remoteip, newfd); + debugprint(DEBUG_FULL, "bouncer-client: new connection from %s on socket %d%s\n", remoteip, newfd, opensslfailmsg); // Alert other clients about the new connection char alertmsg[MAXDATASIZE]; - if (!snprintf(alertmsg, MAXDATASIZE, "NOTICE %s :blabouncer: new client connected from %s.", ircdstate.ircnick, - clients[arrindex(clients, newfd)].remoteip)) { + if (!snprintf(alertmsg, MAXDATASIZE, "NOTICE %s :blabouncer: new client connected from %s%s.", ircdstate.ircnick, + remoteip, opensslfailmsg)) { fprintf(stderr, "Error while preparing new client connection NOTICE!\n"); debugprint(DEBUG_CRIT, "Error while preparing new client connection NOTICE!\n"); alertmsg[0] = '\0'; diff --git a/functions.c b/functions.c index 2b039e0..10e71a1 100644 --- a/functions.c +++ b/functions.c @@ -404,6 +404,7 @@ int arrindex(struct client *clients, int clientfd) { } // Something went wrong, we didn't find it + // TODO - Don't return 0 since 0 is a valid array index, return -1 and have callers check for that. return 0; } diff --git a/message.c b/message.c index 7d50f0a..2c6eb16 100644 --- a/message.c +++ b/message.c @@ -846,12 +846,15 @@ int processclientmessage(SSL *server_ssl, char *str, struct client *clients, int } } } else { - debugprint(DEBUG_SOME, "Password rejected, disconnecting client %s with fd %d.\n", clients[arrindex(clients, sourcefd)].remoteip, sourcefd); + // Store the client's IP address for now, since we need to refer to it after disconnecting + // them (thus clearing the array entry that the IP is read from) + char remoteip[INET6_ADDRSTRLEN]; + strncpy(remoteip, clients[arrindex(clients, sourcefd)].remoteip, INET6_ADDRSTRLEN); + debugprint(DEBUG_SOME, "Password rejected, disconnecting client %s with fd %d.\n", remoteip, sourcefd); disconnectclient(sourcefd, clients, ircdstate, settings, clientcodes); // Alert other clients about the failed authentication char alertmsg[MAXDATASIZE]; - if (!snprintf(alertmsg, MAXDATASIZE, "NOTICE %s :blabouncer: new client %s has failed to authenticate.", ircdstate->ircnick, - clients[arrindex(clients, sourcefd)].remoteip)) { + if (!snprintf(alertmsg, MAXDATASIZE, "NOTICE %s :blabouncer: new client %s failed to authenticate.", ircdstate->ircnick, remoteip)) { fprintf(stderr, "Error while preparing authentication failure NOTICE!\n"); debugprint(DEBUG_CRIT, "Error while preparing authentication failure NOTICE!\n"); alertmsg[0] = '\0'; -- cgit v1.2.3