4
\$\begingroup\$

I wrote this little program to... I don't know. But it connects to IRC servers using OpenSSL and can login via SASL. The code needs refactoring, of that I'm certain, but I need some advice in that regard, so please roast my code.

tallis.h:

#ifndef TALLIS_H
#define TALLIS_H
#include <openssl/ssl.h>
#include <openssl/bio.h>
#include <libconfig.h>
typedef struct tallis_config_settings_struct
{
 config_t config;
 int has_config, has_nick, has_sasl, has_sasl_password;
} settings_t;
typedef struct tallis_bot_struct
{
 int sfd;
 BIO *bio;
 SSL *ssl_connection;
 SSL_CTX *ssl_context;
 X509_VERIFY_PARAM *param;
 char *remote_host, *remote_port, *nick,
 *nethost, *domain, *sasl_challenge;
 const char *sasl_password;
 size_t sasl_challenge_len;
 settings_t settings;
} tallis_t;
void tallis_shutdown(tallis_t*);
#endif /* TALLIS_H */

tallis.c:

#include <stdio.h>
#include <stdlib.h>
#include <libconfig.h>
#include <openssl/ssl.h>
#include <openssl/bio.h>
#include <openssl/err.h>
#include "tallis.h"
#include "net.h"
#include "conf.h"
#include "error.h"
int main(int argc, char *argv[])
{
 int rv;
 tallis_t *tallis = malloc(sizeof(tallis_t));
 tallis->nick = "tallis";
 tallis->remote_host = "irc.freenode.net";
 tallis->remote_port = "6697";
 tallis->nethost = "eleison";
 tallis->domain = "vatican.va";
 tallis->sasl_password = NULL;
 tallis->sasl_challenge_len = 0;
 tallis->bio = NULL;
 tallis->ssl_connection = NULL;
 tallis_ssl_init();
 tallis->ssl_context = SSL_CTX_new(TLS_method());
 tallis->ssl_connection = SSL_new(tallis->ssl_context);
 tallis->param = SSL_get0_param(tallis->ssl_connection);
 char *tallis_conf_path = NULL;
 tallis_conf_path = tallis_set_conf_path();
 if (tallis_conf_path != NULL)
 {
 rv = tallis_parse_config(&tallis->settings.config, tallis_conf_path);
 free(tallis_conf_path);
 if (rv != CONFIG_TRUE)
 tallis_config_fail(tallis);
 else if (rv == CONFIG_TRUE)
 tallis->settings.has_config = 1;
 }
 else if (tallis_conf_path == NULL)
 tallis_config_fail(tallis);
 tallis_check_sasl(tallis);
 rv = tallis_init_ssl_verify(tallis);
 if (!rv)
 DIE("%s\n", "error initializing ssl verification data");
 rv = tallis_connect(tallis);
 if (!rv)
 DIE("%s\n", "connection failed");
 ERR_clear_error();
 X509 *cert = NULL;
 cert = SSL_get_peer_certificate(tallis->ssl_connection);
 if (!cert)
 DIE("%s\n", ERR_error_string(ERR_get_error(), NULL));
 X509_free(cert);
 rv = tallis_ssl_verify(tallis, cert);
 if (!rv)
 DIE("%s\n", "certificate verificiation failed");
 else
 printf("%s\n", "certificate verification succeeded");
 rv = tallis_loop(tallis);
 if (!rv)
 DIE("%s\n", "socket connection terminated");
 rv = ssl_shutdown(tallis);
 if (!rv)
 DIE("%s\n", "ssl shutdown failed");
 return EXIT_SUCCESS;
}
_Noreturn void tallis_shutdown(tallis_t* tallis)
{
 int code = ssl_shutdown(tallis);
 free(tallis);
 exit(code);
}

net.h:

#ifndef NET_H
#define NET_H
#include <stdio.h>
#include <openssl/ssl.h>
#include <openssl/bio.h>
#include <openssl/x509v3.h>
#include "tallis.h"
void tallis_ssl_init();
int ssl_shutdown(tallis_t*);
int tallis_connect(tallis_t*);
int tallis_init_ssl_verify(tallis_t*);
int tallis_ssl_verify(tallis_t*, X509*);
int tallis_verify_cert_chain(tallis_t*, X509*);
void tallis_base64_encode(char*, int, char**);
int tallis_send(tallis_t*, const char*, size_t);
int tallis_loop(tallis_t*);
void tallis_print(char*, ssize_t);
#endif /* NET_H */

net.c:

#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#include <stdbool.h>
#include <inttypes.h>
#include <string.h>
#include <errno.h>
#include <sys/types.h>
#include <unistd.h>
#include <sys/socket.h>
#include <netdb.h>
#include <libconfig.h>
#include <openssl/ssl.h>
#include <openssl/bio.h>
#include <openssl/x509v3.h>
#include <openssl/x509_vfy.h>
#include <openssl/err.h>
#include "tallis.h"
#include "net.h"
#include "conf.h"
#include "irc.h"
#include "lexer.h"
#include "error.h"
void tallis_ssl_init()
{
 SSL_library_init();
}
int ssl_shutdown(tallis_t *tallis)
{
 int rv, err;
 ERR_clear_error();
 rv = SSL_shutdown(tallis->ssl_connection);
 if (rv == 0)
 SSL_shutdown(tallis->ssl_connection);
 if (rv < 0)
 {
 err = SSL_get_error(tallis->ssl_connection, rv);
 if (err == SSL_ERROR_SSL)
 fprintf(stderr, "%s\n", ERR_error_string(ERR_get_error(), NULL));
 fprintf(stderr, "%s\n", SSL_state_string(tallis->ssl_connection));
 return 1;
 }
 ERR_free_strings();
 SSL_free(tallis->ssl_connection);
 SSL_CTX_free(tallis->ssl_context);
 return 0;
}
int tallis_init_ssl_verify(tallis_t *tallis)
{
 int rv;
 ERR_clear_error();
 rv = SSL_CTX_load_verify_locations(
 tallis->ssl_context,
 "/etc/ssl/certs/AddTrust_External_Root.pem",
 "/etc/ssl/certs");
 if (!rv)
 {
 fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));
 return 0;
 }
 X509_VERIFY_PARAM_set_hostflags(
 tallis->param,
 X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS);
 ERR_clear_error();
 rv = X509_VERIFY_PARAM_set_flags(
 tallis->param,
 X509_V_FLAG_CRL_CHECK || X509_V_FLAG_CRL_CHECK_ALL);
 if (!rv)
 {
 fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));
 return 0;
 }
 rv = X509_VERIFY_PARAM_set1_host(tallis->param, tallis->nethost, 0);
 if (!rv)
 {
 fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));
 return 0;
 }
 SSL_CTX_set_verify(tallis->ssl_context, SSL_VERIFY_PEER, NULL);
 SSL_set_verify(tallis->ssl_connection, SSL_VERIFY_PEER, NULL);
 return 1;
}
int tallis_ssl_verify(tallis_t *tallis, X509 *cert)
{
 int rv;
 ERR_clear_error();
 rv = SSL_get_verify_result(tallis->ssl_connection);
 if (rv != X509_V_OK)
 {
 fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));
 return 0;
 }
 return 1;
}
void tallis_base64_encode(char *src, int len, char **dest)
{
 BIO *bio = NULL, *b64 = NULL;
 BUF_MEM *bufferPtr;
 b64 = BIO_new(BIO_f_base64());
 bio = BIO_new(BIO_s_mem());
 bio = BIO_push(b64, bio);
 BIO_set_flags(bio, BIO_FLAGS_BASE64_NO_NL);
 BIO_write(bio, src, len);
 BIO_flush(bio);
 BIO_get_mem_ptr(bio, &bufferPtr);
 BIO_set_close(bio, BIO_NOCLOSE);
 BIO_free_all(bio);
 *dest=(*bufferPtr).data;
}
int tallis_connect(tallis_t *tallis)
{
 int rv;
 ERR_clear_error();
 tallis->bio = BIO_new_ssl_connect(tallis->ssl_context);
 if (!tallis->bio)
 {
 fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));
 return 0;
 }
 BIO_get_ssl(tallis->bio, &(tallis->ssl_connection));
 SSL_set_mode(tallis->ssl_connection, SSL_MODE_AUTO_RETRY);
 BIO_set_conn_hostname(tallis->bio, tallis->remote_host);
 BIO_set_conn_port(tallis->bio, tallis->remote_port);
 ERR_clear_error();
 rv = BIO_do_connect(tallis->bio);
 if (!rv)
 {
 fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));
 return 0;
 }
 ERR_clear_error();
 rv = BIO_do_handshake(tallis->bio);
 if (!rv)
 {
 fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));
 return 0;
 }
 return 1;
}
int tallis_send(tallis_t *tallis, const char *msg, size_t len)
{
 ssize_t bytes_written;
 int retry_limit = 2;
 ERR_clear_error();
 bytes_written = BIO_write(tallis->bio, msg, len);
 while (BIO_should_retry(tallis->bio) && bytes_written < len)
 bytes_written = BIO_write(tallis->bio, msg, len);
 if (!bytes_written)
 {
 fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));
 return 0;
 }
 return 1;
}
inline void tallis_print(char *buf, ssize_t len)
{
 for (int i = 0; i < len; ++i)
 printf("%c", buf[i]);
}
void tallis_parse(tallis_t *tallis, char buf[], int len)
{
 char pong[512] = "PONG :";
 size_t msg_len;
 if (strncmp(buf, "PING :", 6) == 0)
 {
 const char *pong_msg = strncat(pong, buf + 6, len - 6);
 msg_len = strlen(pong_msg);
 tallis_send(tallis, pong_msg, msg_len);
 }
 if (strstr(buf, "CAP * ACK :multi-prefix sasl"))
 {
 const char *sasl_mechanism_msg = "AUTHENTICATE PLAIN\r\n";
 msg_len = strlen(sasl_mechanism_msg);
 tallis_send(tallis, sasl_mechanism_msg, msg_len);
 }
 if (strstr(buf, "CAP * NAK :multi-prefix sasl") != NULL)
 {
 const char *cap_end_msg = "CAP END\r\n";
 msg_len = strlen(cap_end_msg);
 tallis_send(tallis, cap_end_msg, msg_len);
 }
 if (strstr(buf, "AUTHENTICATE +") != NULL)
 {
 const char *sasl_auth_msg = "AUTHENTICATE";
 msg_len = strlen(sasl_auth_msg);
 tallis_send(tallis, sasl_auth_msg, msg_len);
 const char *space = " ";
 msg_len = strlen(space);
 tallis_send(tallis, space, msg_len);
 if (tallis->sasl_challenge)
 {
 msg_len = tallis->sasl_challenge_len;
 tallis_send(tallis, tallis->sasl_challenge, msg_len);
 }
 const char *delimiter = "\r\n";
 msg_len = strlen(delimiter);
 tallis_send(tallis, delimiter, msg_len);
 }
 if (strstr(buf, "SASL authentication successful") != NULL)
 {
 const char *cap_end_msg = "CAP END\r\n";
 msg_len = strlen(cap_end_msg);
 tallis_send(tallis, cap_end_msg, msg_len);
 }
 if (strstr(buf, "MODE tallis") != NULL)
 {
 const char *join_msg = "JOIN #tallistestchannel\r\n";
 msg_len = strlen(join_msg);
 tallis_send(tallis, join_msg, msg_len);
 }
 if (strstr(buf, "~tallis help") != NULL)
 {
 const char *kok_msg = "PRIVMSG #tallistestchannel :KOK\r\n";
 msg_len = strlen(kok_msg);
 tallis_send(tallis, kok_msg, msg_len);
 }
 if (strstr(buf, "~tallis quit") != NULL)
 tallis_shutdown(tallis);
 /* lexer_analyze(buf); */
}
int tallis_loop(tallis_t *tallis)
{
 ssize_t bytes_read;
 size_t msg_len;
 char buf[512] = {0}, initial_message[512];
 snprintf(initial_message, 512, "%s %s\r\n%s %s %s %s :%s\r\n", "NICK",
 tallis->nick, "USER", tallis->nick, tallis->nethost,
 tallis->domain, tallis->nick);
 if (!tallis_get_sasl_password(tallis))
 tallis->settings.has_sasl = 0;
 if (!tallis->settings.has_config || !tallis->settings.has_sasl ||
 !tallis->settings.has_sasl_password)
 puts("running with SASL disabled");
 else if (tallis->settings.has_config && tallis->settings.has_sasl &&
 tallis->settings.has_sasl_password)
 {
 size_t nicklen = strlen(tallis->nick),
 passlen = strlen(tallis->sasl_password);
 int len = nicklen + 1 + nicklen + 1 + passlen;
 char *temp = malloc(len);
 memcpy(temp, tallis->nick, nicklen);
 memcpy(temp + nicklen + 1, tallis->nick, nicklen + 1);
 memcpy(temp + nicklen * 2 + 2, tallis->sasl_password, passlen);
 tallis_base64_encode(temp, len, &tallis->sasl_challenge);
 tallis->sasl_challenge_len = strlen(tallis->sasl_challenge);
 const char *cap_req_msg = "CAP REQ :multi-prefix sasl\r\n";
 msg_len = strlen(cap_req_msg);
 tallis_send(tallis, cap_req_msg, msg_len);
 }
 msg_len = strlen(initial_message);
 if (!tallis_send(tallis, initial_message, msg_len))
 return 0;
 while (1)
 {
 memset(buf, '0円', 512);
 bytes_read = BIO_read(tallis->bio, buf, 512);
 if (!bytes_read)
 {
 if (!BIO_should_retry(tallis->bio))
 {
 fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));
 return 1;
 }
 puts("retry read?");
 }
 buf[bytes_read] = '0円';
 tallis_print(buf, bytes_read);
 tallis_parse(tallis, buf, bytes_read);
 }
 return 1;
}

conf.h:

#ifndef CONF_H
#define CONF_H
#include <libconfig.h>
#include "tallis.h"
int tallis_check_sasl(tallis_t*);
int tallis_get_sasl_password(tallis_t*);
void tallis_config_fail(tallis_t*);
char *tallis_set_conf_path(void);
int tallis_parse_config(config_t*, const char*);
#endif /* CONF_H */

conf.c:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <pwd.h>
#include <errno.h>
#include <libconfig.h>
#include "conf.h"
#include "tallis.h"
#include "error.h"
int tallis_check_sasl(tallis_t *tallis)
{
 int rv = config_lookup_bool(&tallis->settings.config, "sasl",
 &tallis->settings.has_sasl);
 if (!rv)
 return 0;
 return 1;
}
int tallis_get_sasl_password(tallis_t* tallis)
{
 int rv = config_lookup_string(&tallis->settings.config, "sasl_password",
 &tallis->sasl_password);
 if (rv && tallis->sasl_password != NULL)
 tallis->settings.has_sasl_password = 1;
 else
 return 0;
 return 1;
}
void tallis_config_fail(tallis_t *tallis)
{
 tallis->settings.has_config = 0;
 puts("running with default settings, create ~/.tallis/tallis.conf" \
 " or fix your syntax");
}
char *tallis_set_conf_path()
{
 errno = 0;
 const char *home = getenv("HOME");
 if (!home)
 home = getpwuid(getuid())->pw_dir;
 if (!home)
 {
 perror(NULL);
 fprintf(stderr, "%s\n", "could not determine home directory");
 return NULL;
 }
 const char *conf_dir = "/.tallis/", *conf_file = "tallis.conf";
 int len = strlen(home) + strlen(conf_dir) + strlen(conf_file) + 1;
 char *path = malloc(len);
 snprintf(path, len, "%s%s%s", home, conf_dir, conf_file);
 return path;
}
int tallis_parse_config(config_t *config, const char *path)
{
 errno = 0;
 FILE *file;
 file = fopen(path, "a+");
 if (!file)
 {
 perror(NULL);
 fprintf(stderr, "%s\n", "couldn't open config");
 return 0;
 }
 fclose(file);
 config_init(config);
 int rv = config_read_file(config, path);
 if (rv == CONFIG_FALSE)
 {
 int err = config_error_type(config);
 switch (err)
 {
 case CONFIG_ERR_PARSE:
 fprintf(stderr, "%s\n%s:line %d\n%s\n", "error parsing file:",
 path,
 config_error_line(config),
 config_error_text(config));
 config_destroy(config);
 return 0;
 break;
 case CONFIG_ERR_FILE_IO:
 fprintf(stderr, "%s\n%s\n", "error opening file:",
 path);
 config_destroy(config);
 return 0;
 break;
 case CONFIG_ERR_NONE:
 fprintf(stderr, "%s\n", "unknown error");
 config_destroy(config);
 return 0;
 break;
 }
 }
 return 1;
}

irc.h:

#ifndef IRC_H
#define IRC_H
#include <stdbool.h>
#include "tallis.h"
typedef struct tallis_server_capabilities_struct
{
 bool cap_sasl, cap_multi_prefix;
} tallis_server_capabilities_t;
void tallis_check_capability(tallis_t*,
 tallis_server_capabilities_t*, char*);
#endif /* IRC_H */

irc.c:

#include <stdio.h>
#include <string.h>
#include <stdbool.h>
#include "irc.h"
#include "tallis.h"
#include "net.h"
/* Use CAP to determine capabilities of IRC server (SASL support etc) */
void tallis_check_capability(tallis_t *tallis,
 tallis_server_capabilities_t* capabilities, char *buf)
{
 char *command = "CAP REQ :multi-prefix sasl\r\n";
 int msg_len = strlen(command);
 tallis_send(tallis, command, msg_len);
 if (strstr(buf, "CAP * ACK :multi-prefix sasl"))
 capabilities->cap_multi_prefix = capabilities->cap_sasl = true;
}

error.h:

#ifndef ERROR_H
#define ERROR_H
#define DIE(format, ...) die("\n%s:%d in %s\n" format, __FILE__, __LINE__, __func__, __VA_ARGS__)
void die(const char*, ...);
#endif /* ERROR_H */

error.c:

#include <stdio.h>
#include <stdlib.h>
#include <stdarg.h>
#include "error.h"
#ifdef __GNUC__
__attribute__ ((format(__printf__, 1, 2)))
#endif
_Noreturn void die(const char *format, ...)
{
 va_list vargs;
 va_start(vargs, format);
 vfprintf(stderr, format, vargs);
 va_end(vargs);
 exit(1);
}
asked Mar 3, 2017 at 22:42
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Parsing Incoming Messages

After calling BIO_read on a 512 byte buffer, you simply set buf[bytes_read] = '0円' (which overflows if you just happen to have read exactly 512 bytes) and start parsing the buffer.

However, you have no guarantee that your buffer contains one and only one IRC message. It may contain only the start of a message, multiple full messages or even a few full messages and a partial one at the end. You need to buffer the input and then search for newlines which IRC uses to separate messages.

Compiler Warnings

The compiler shows various warnings when compiling the code. These should be fixed

net.c:69:9: warning: format not a string literal and no format arguments [-Wformat-security]
 fprintf(stderr, ERR_error_string(ERR_get_error(), NULL));
net.c:176:9: warning: unused variable ‘retry_limit’ [-Wunused-variable]
 int retry_limit = 2;
tallis.c:27:39: warning: implicit declaration of function ‘TLS_method’ [-Wimplicit-function-declaration]
 tallis->ssl_context = SSL_CTX_new(TLS_method());

Memory Management

Using pointers doesn't force you to dynamically allocate memory. In you main function, you declare tallis_t *tallis = malloc(sizeof(tallis_t)); only to pass the pointer around and free it later. As it doesn't consume much memory, you can simply allocate it on the stack.

Code Duplication

Every time you call tallis_send, you invoke strlen on the line just before. If you know you'll pass null-terminated strings, make tallis_send or a helper function call strlen every time (perhaps make it even add the newline delimiter).

Code Division

Most of your connection logic is in net.c, but one function seems to have been misplaced inside irc.c. I would except net.c to deal with the communication infrastructure and let irc.c handle all the parsing/generation of IRC specific messages.

The main function also contains a lot of code. It's also confusing to find a lot of hard-coded values, when there is also code that deals with loading a configuration file.

tallis_set_conf_path contains the verb set but it actually returns a value. The memory allocated for the return value is never freed.

Running the Application

Starting the application, it writes an ambiguous No such file or directory and then crashes in tallis_check_sasl as no config file is loaded, yet the application fails to check for this on all branches.

Memory is also leaked:

==5027== HEAP SUMMARY:
==5027== in use at exit: 98,388 bytes in 376 blocks
==5027== total heap usage: 739 allocs, 363 frees, 174,824 bytes allocated
==5027==
==5027== 3,744 (824 direct, 2,920 indirect) bytes in 1 blocks are definitely lost in loss record 297 of 303
==5027== at 0x4C2CB3F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5027== by 0x5104E87: CRYPTO_malloc (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027== by 0x4E790AF: SSL_new (in /lib/x86_64-linux-gnu/libssl.so.1.0.0)
==5027== by 0x10ACD5: main (tallis.c:28)
==5027==
==5027== 4,096 bytes in 1 blocks are definitely lost in loss record 300 of 303
==5027== at 0x4C2CB3F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5027== by 0x4041AB1: ???
==5027== by 0x4054D1B: ???
==5027== by 0x4063A30: ???
==5027== by 0x403ED12: ???
==5027== by 0x40648DD: ???
==5027== by 0x580DA47: gethostbyname_r@@GLIBC_2.2.5 (getXXbyYY_r.c:315)
==5027== by 0x580D01E: gethostbyname (getXXbyYY.c:116)
==5027== by 0x51B8701: BIO_get_host_ip (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027== by 0x51B4B33: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027== by 0x51B508F: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027== by 0x51B294A: BIO_write (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==5027==
==5027== LEAK SUMMARY:
==5027== definitely lost: 4,920 bytes in 2 blocks
==5027== indirectly lost: 2,920 bytes in 8 blocks
==5027== possibly lost: 0 bytes in 0 blocks
==5027== still reachable: 90,548 bytes in 366 blocks
==5027== suppressed: 0 bytes in 0 blocks
answered Mar 4, 2017 at 12:44
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.