From 5ce75dbf5812fd584c1ce3a03f1a3aa38caeefcc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Deuc=D0=B5?= <shurd@sasktel.net>
Date: Mon, 18 Dec 2023 23:45:31 -0500
Subject: [PATCH] Only mutex proect tls_certificate usage.
Holding the lock around session establishment should not be needed,
but we need to protect tls_certificate read and usage. Since we
don't have rwlocks in xpdev (yet?), hack together a crappy rwlock
that does what we need.
---
src/sbbs3/ftpsrvr.c | 4 +--
src/sbbs3/js_socket.c | 6 ++--
src/sbbs3/load_cfg.c | 6 +++-
src/sbbs3/mailsrvr.c | 20 ++++----------
src/sbbs3/services.c | 5 ++--
src/sbbs3/ssl.c | 64 ++++++++++++++++++++++++++++++++++++++++---
src/sbbs3/websrvr.c | 5 ++--
7 files changed, 79 insertions(+), 31 deletions(-)
diff --git a/src/sbbs3/ftpsrvr.c b/src/sbbs3/ftpsrvr.c
index 6ed419b16d..d1717a6017 100644
--- a/src/sbbs3/ftpsrvr.c
+++ b/src/sbbs3/ftpsrvr.c
@@ -1184,12 +1184,12 @@ static BOOL start_tls(SOCKET *sock, CRYPT_SESSION *sess, BOOL resp)
sockprintf(*sock, *sess, "431 TLS not available");
return FALSE;
}
+ unlock_ssl_cert();
nodelay = TRUE;
(void)setsockopt(*sock,IPPROTO_TCP,TCP_NODELAY,(char*)&nodelay,sizeof(nodelay));
nb=0;
ioctlsocket(*sock,FIONBIO,&nb);
if ((status = cryptSetAttribute(*sess, CRYPT_SESSINFO_NETWORKSOCKET, *sock)) != CRYPT_OK) {
- unlock_ssl_cert();
GCES(status, *sock, *sess, estr, "setting network socket");
cryptDestroySession(*sess);
*sess = -1;
@@ -1200,11 +1200,9 @@ static BOOL start_tls(SOCKET *sock, CRYPT_SESSION *sess, BOOL resp)
if (resp)
sockprintf(*sock, -1, "234 Ready to start TLS");
if ((status = cryptSetAttribute(*sess, CRYPT_SESSINFO_ACTIVE, 1)) != CRYPT_OK) {
- unlock_ssl_cert();
GCES(status, *sock, *sess, estr, "setting session active");
return TRUE;
}
- unlock_ssl_cert();
if (startup->max_inactivity) {
if ((status = cryptSetAttribute(*sess, CRYPT_OPTION_NET_READTIMEOUT, startup->max_inactivity)) != CRYPT_OK) {
GCES(status, *sock, *sess, estr, "setting read timeout");
diff --git a/src/sbbs3/js_socket.c b/src/sbbs3/js_socket.c
index 96da84eaef..ae2a66dbdf 100644
--- a/src/sbbs3/js_socket.c
+++ b/src/sbbs3/js_socket.c
@@ -2368,24 +2368,22 @@ static JSBool js_socket_set(JSContext *cx, JSObject *obj, jsid id, JSBool strict
free_crypt_attrstr(estr);
}
}
+ lock_ssl_cert();
if (scfg->tls_certificate == -1)
ret = CRYPT_ERROR_NOTAVAIL;
else {
- lock_ssl_cert();
ret = cryptSetAttribute(p->session, CRYPT_SESSINFO_PRIVATEKEY, scfg->tls_certificate);
if (ret != CRYPT_OK) {
- unlock_ssl_cert();
GCES(ret, p, estr, "setting private key");
}
}
+ unlock_ssl_cert();
}
}
if(ret==CRYPT_OK) {
if((ret=do_cryptAttribute(p->session, CRYPT_SESSINFO_ACTIVE, 1))!=CRYPT_OK) {
GCES(ret, p, estr, "setting session active");
}
- if (tiny == SOCK_PROP_SSL_SERVER)
- unlock_ssl_cert();
}
}
}
diff --git a/src/sbbs3/load_cfg.c b/src/sbbs3/load_cfg.c
index 4789346f37..f2274fade3 100644
--- a/src/sbbs3/load_cfg.c
+++ b/src/sbbs3/load_cfg.c
@@ -349,8 +349,12 @@ void prep_cfg(scfg_t* cfg)
void free_cfg(scfg_t* cfg)
{
#ifdef USE_CRYPTLIB
- if (cfg->tls_certificate != -1 && cfg->prepped)
+ lock_ssl_cert();
+ if (cfg->tls_certificate != -1 && cfg->prepped) {
cryptDestroyContext(cfg->tls_certificate);
+ cfg->tls_certificate == -1;
+ }
+ unlock_ssl_cert();
#endif
free_node_cfg(cfg);
free_main_cfg(cfg);
diff --git a/src/sbbs3/mailsrvr.c b/src/sbbs3/mailsrvr.c
index 9916a7bce3..337783650b 100644
--- a/src/sbbs3/mailsrvr.c
+++ b/src/sbbs3/mailsrvr.c
@@ -1092,21 +1092,19 @@ static bool pop3_client_thread(pop3_t* pop3)
GCESH(stat, client.protocol, socket, host_ip, session, "setting private key");
return false;
}
+ unlock_ssl_cert();
nodelay = TRUE;
setsockopt(socket,IPPROTO_TCP,TCP_NODELAY,(char*)&nodelay,sizeof(nodelay));
nb=0;
ioctlsocket(socket,FIONBIO,&nb);
if ((stat = cryptSetAttribute(session, CRYPT_SESSINFO_NETWORKSOCKET, socket)) != CRYPT_OK) {
- unlock_ssl_cert();
GCESH(stat, client.protocol, socket, host_ip, session, "setting session socket");
return false;
}
if ((stat = cryptSetAttribute(session, CRYPT_SESSINFO_ACTIVE, 1)) != CRYPT_OK) {
- unlock_ssl_cert();
GCESH(stat, client.protocol, socket, host_ip, session, "setting session active");
return false;
}
- unlock_ssl_cert();
if (startup->max_inactivity) {
if (cryptSetAttribute(session, CRYPT_OPTION_NET_READTIMEOUT, startup->max_inactivity) != CRYPT_OK) {
GCESH(stat, client.protocol, socket, host_ip, session, "setting read timeout");
@@ -1216,23 +1214,21 @@ static bool pop3_client_thread(pop3_t* pop3)
buf[0] = 0;
break;
}
+ unlock_ssl_cert();
nodelay = TRUE;
setsockopt(socket,IPPROTO_TCP,TCP_NODELAY,(char*)&nodelay,sizeof(nodelay));
nb=0;
ioctlsocket(socket,FIONBIO,&nb);
if ((stat = cryptSetAttribute(session, CRYPT_SESSINFO_NETWORKSOCKET, socket)) != CRYPT_OK) {
- unlock_ssl_cert();
GCESH(stat, client.protocol, socket, host_ip, session, "setting network socket");
buf[0] = 0;
break;
}
if ((stat=cryptSetAttribute(session, CRYPT_SESSINFO_ACTIVE, 1)) != CRYPT_OK) {
- unlock_ssl_cert();
GCESH(stat, client.protocol, socket, host_ip, session, "setting session active");
buf[0] = 0;
break;
}
- unlock_ssl_cert();
if (startup->max_inactivity) {
if ((stat=cryptSetAttribute(session, CRYPT_OPTION_NET_READTIMEOUT, startup->max_inactivity)) != CRYPT_OK) {
GCESH(stat, client.protocol, socket, host_ip, session, "setting read timeout");
@@ -2993,21 +2989,19 @@ static bool smtp_client_thread(smtp_t* smtp)
GCESH(cstat, client.protocol, socket, host_ip, session, "setting private key");
return false;
}
+ unlock_ssl_cert();
nodelay = TRUE;
setsockopt(socket,IPPROTO_TCP,TCP_NODELAY,(char*)&nodelay,sizeof(nodelay));
nb=0;
ioctlsocket(socket,FIONBIO,&nb);
if ((cstat = cryptSetAttribute(session, CRYPT_SESSINFO_NETWORKSOCKET, socket)) != CRYPT_OK) {
- unlock_ssl_cert();
GCESH(cstat, client.protocol, socket, host_ip, session, "setting network socket");
return false;
}
if ((cstat = cryptSetAttribute(session, CRYPT_SESSINFO_ACTIVE, 1)) != CRYPT_OK) {
- unlock_ssl_cert();
GCESH(cstat, client.protocol, socket, host_ip, session, "setting session active");
return false;
}
- unlock_ssl_cert();
if (startup->max_inactivity) {
if ((cstat = cryptSetAttribute(session, CRYPT_OPTION_NET_READTIMEOUT, startup->max_inactivity)) != CRYPT_OK) {
GCESH(cstat, client.protocol, socket, host_ip, session, "setting read timeout");
@@ -4975,12 +4969,12 @@ static bool smtp_client_thread(smtp_t* smtp)
sockprintf(socket, client.protocol, session, "454 TLS not available");
continue;
}
+ unlock_ssl_cert();
nodelay = TRUE;
setsockopt(socket,IPPROTO_TCP,TCP_NODELAY,(char*)&nodelay,sizeof(nodelay));
nb=0;
ioctlsocket(socket,FIONBIO,&nb);
if ((cstat = cryptSetAttribute(session, CRYPT_SESSINFO_NETWORKSOCKET, socket)) != CRYPT_OK) {
- unlock_ssl_cert();
GCESH(cstat, "SMTPS", socket, host_ip, session, "setting network socket");
cryptDestroySession(session);
session = -1;
@@ -4989,11 +4983,9 @@ static bool smtp_client_thread(smtp_t* smtp)
}
sockprintf(socket, client.protocol, -1, "220 Ready to start TLS");
if ((cstat=cryptSetAttribute(session, CRYPT_SESSINFO_ACTIVE, 1)) != CRYPT_OK) {
- unlock_ssl_cert();
GCESH(cstat, "SMTPS", socket, host_ip, session, "setting session active");
break;
}
- unlock_ssl_cert();
if (startup->max_inactivity) {
if ((cstat=cryptSetAttribute(session, CRYPT_OPTION_NET_READTIMEOUT, startup->max_inactivity)) != CRYPT_OK) {
GCESH(cstat, "SMTPS", socket, host_ip, session, "setting read timeout");
@@ -5405,21 +5397,19 @@ static SOCKET sendmail_negotiate(CRYPT_SESSION *session, smb_t *smb, smbmsg_t *m
GCESH(status, prot, sock, server, *session, "setting private key");
continue;
}
+ unlock_ssl_cert();
nodelay = TRUE;
setsockopt(sock,IPPROTO_TCP,TCP_NODELAY,(char*)&nodelay,sizeof(nodelay));
nb=0;
ioctlsocket(sock,FIONBIO,&nb);
if ((status=cryptSetAttribute(*session, CRYPT_SESSINFO_NETWORKSOCKET, sock)) != CRYPT_OK) {
- unlock_ssl_cert();
GCESH(status, prot, sock, server, *session, "setting network socket");
continue;
}
if ((status=cryptSetAttribute(*session, CRYPT_SESSINFO_ACTIVE, 1)) != CRYPT_OK) {
- unlock_ssl_cert();
GCESHL(status, prot, sock, server, LOG_WARNING, *session, "setting session active");
continue;
}
- unlock_ssl_cert();
if (startup->max_inactivity) {
if ((status=cryptSetAttribute(*session, CRYPT_OPTION_NET_READTIMEOUT, startup->max_inactivity)) != CRYPT_OK) {
GCESH(status, prot, sock, server, *session, "setting read timeout");
diff --git a/src/sbbs3/services.c b/src/sbbs3/services.c
index ca70d13500..69efbb9e1c 100644
--- a/src/sbbs3/services.c
+++ b/src/sbbs3/services.c
@@ -1100,18 +1100,17 @@ static void js_service_thread(void* arg)
if (scfg.tls_certificate != -1) {
HANDLE_CRYPT_CALL(cryptSetAttribute(service_client.tls_sess, CRYPT_SESSINFO_PRIVATEKEY, scfg.tls_certificate), &service_client, "setting private key");
}
+ unlock_ssl_cert();
BOOL nodelay=TRUE;
setsockopt(socket,IPPROTO_TCP,TCP_NODELAY,(char*)&nodelay,sizeof(nodelay));
HANDLE_CRYPT_CALL(cryptSetAttribute(service_client.tls_sess, CRYPT_SESSINFO_NETWORKSOCKET, socket), &service_client, "setting network socket");
if (!HANDLE_CRYPT_CALL(cryptSetAttribute(service_client.tls_sess, CRYPT_SESSINFO_ACTIVE, 1), &service_client, "setting session active")) {
- unlock_ssl_cert();
if (service_client.tls_sess != -1)
cryptDestroySession(service_client.tls_sess);
js_service_failure_cleanup(service, socket);
return;
}
- unlock_ssl_cert();
}
#if 0 /* Need to export from SBBS.DLL */
@@ -1999,9 +1998,11 @@ void services_thread(void* arg)
lprintf(LOG_ERR, "Option error, TLS not yet supported for static services (%s)", service[i].protocol);
continue;
}
+ lock_ssl_cert();
if(scfg.tls_certificate == -1) {
need_cert = TRUE;
}
+ unlock_ssl_cert();
}
service[i].set=xpms_create(startup->bind_retry_count, startup->bind_retry_delay, lprintf);
if(service[i].set == NULL) {
diff --git a/src/sbbs3/ssl.c b/src/sbbs3/ssl.c
index 0419ce0abd..66d9095118 100644
--- a/src/sbbs3/ssl.c
+++ b/src/sbbs3/ssl.c
@@ -233,6 +233,7 @@ bool get_crypt_error_string(int status, CRYPT_HANDLE sess, char **estr, const ch
static pthread_once_t crypt_init_once = PTHREAD_ONCE_INIT;
static pthread_mutex_t ssl_cert_mutex;
+static pthread_mutex_t ssl_rwlock;
static bool cryptlib_initialized;
static void do_cryptEnd(void)
@@ -253,6 +254,7 @@ static void internal_do_cryptInit(void)
lprintf(LOG_ERR,"cryptInit() returned %d", ret);
}
pthread_mutex_init(&ssl_cert_mutex, NULL);
+ pthread_mutex_init(&ssl_rwlock, NULL);
return;
}
@@ -268,14 +270,34 @@ bool is_crypt_initialized(void)
return cryptlib_initialized;
}
+static uint32_t readers;
+static uint32_t writers;
+
void lock_ssl_cert(void)
{
- pthread_mutex_lock(&ssl_cert_mutex);
+ pthread_mutex_lock(&ssl_rwlock);
+ if (writers) {
+ pthread_mutex_unlock(&ssl_rwlock);
+ pthread_mutex_lock(&ssl_cert_mutex);
+ readers++;
+ pthread_mutex_unlock(&ssl_cert_mutex);
+ }
+ else {
+ readers++;
+ pthread_mutex_unlock(&ssl_rwlock);
+ }
}
void unlock_ssl_cert(void)
{
- pthread_mutex_unlock(&ssl_cert_mutex);
+ pthread_mutex_lock(&ssl_rwlock);
+ if (readers == 0) {
+ lprintf(LOG_ERR, "Unlocking ssl cert when it's not locked.");
+ }
+ else {
+ readers--;
+ }
+ pthread_mutex_unlock(&ssl_rwlock);
}
#define DO(action, handle, x) get_crypt_error_string(x, handle, estr, action, level)
@@ -287,26 +309,48 @@ CRYPT_CONTEXT get_ssl_cert(scfg_t *cfg, char **estr, int *level)
CRYPT_CERTIFICATE ssl_cert;
char sysop_email[sizeof(cfg->sys_inetaddr)+6];
char str[MAX_PATH+1];
+ int done = 0;
if (estr)
*estr = NULL;
if(!do_cryptInit())
return -1;
+ pthread_mutex_lock(&ssl_rwlock);
+ writers++;
+ // Lock cert mutex while we hold rwlock to ensure we have it whenever writers is non-zero.
pthread_mutex_lock(&ssl_cert_mutex);
+ pthread_mutex_unlock(&ssl_rwlock);
+ // Drain all the readers
+ while (!done) {
+ pthread_mutex_lock(&ssl_rwlock);
+ done = (readers == 0);
+ pthread_mutex_unlock(&ssl_rwlock);
+ if (!done) {
+ YIELD();
+ }
+ }
SAFEPRINTF2(str,"%s%s",cfg->ctrl_dir,"ssl.cert");
time32_t fd = (time32_t)fdate(str);
if (cfg->tls_certificate != -1 || !cfg->prepped) {
if (fd == cfg->tls_cert_file_date) {
+ ssl_context = cfg->tls_certificate;
+ pthread_mutex_lock(&ssl_rwlock);
+ writers--;
+ pthread_mutex_unlock(&ssl_rwlock);
pthread_mutex_unlock(&ssl_cert_mutex);
- return cfg->tls_certificate;
+ return ssl_context;
}
cfg->tls_cert_file_date = fd;
cryptDestroyContext(cfg->tls_certificate);
+ cfg->tls_certificate = -1;
}
cfg->tls_cert_file_date = fd;
/* Get the certificate... first try loading it from a file... */
if(cryptStatusOK(cryptKeysetOpen(&ssl_keyset, CRYPT_UNUSED, CRYPT_KEYSET_FILE, str, CRYPT_KEYOPT_READONLY))) {
if(!DO("getting private key", ssl_keyset, cryptGetPrivateKey(ssl_keyset, &ssl_context, CRYPT_KEYID_NAME, "ssl_cert", cfg->sys_pass))) {
+ pthread_mutex_lock(&ssl_rwlock);
+ writers--;
+ pthread_mutex_unlock(&ssl_rwlock);
pthread_mutex_unlock(&ssl_cert_mutex);
return -1;
}
@@ -314,6 +358,9 @@ CRYPT_CONTEXT get_ssl_cert(scfg_t *cfg, char **estr, int *level)
else {
/* Couldn't do that... create a new context and use the cert from there... */
if(!DO("creating SSL context", CRYPT_UNUSED,cryptCreateContext(&ssl_context, CRYPT_UNUSED, CRYPT_ALGO_RSA))) {
+ pthread_mutex_lock(&ssl_rwlock);
+ writers--;
+ pthread_mutex_unlock(&ssl_rwlock);
pthread_mutex_unlock(&ssl_cert_mutex);
return -1;
}
@@ -351,6 +398,8 @@ CRYPT_CONTEXT get_ssl_cert(scfg_t *cfg, char **estr, int *level)
cryptDestroyCert(ssl_cert);
cryptKeysetClose(ssl_keyset);
cryptDestroyContext(ssl_context);
+ cfg->tls_certificate = -1;
+ ssl_context = -1;
// Finally, load it from the file.
if(cryptStatusOK(cryptKeysetOpen(&ssl_keyset, CRYPT_UNUSED, CRYPT_KEYSET_FILE, str, CRYPT_KEYOPT_READONLY))) {
if(!DO("getting private key", ssl_keyset, cryptGetPrivateKey(ssl_keyset, &ssl_context, CRYPT_KEYID_NAME, "ssl_cert", cfg->sys_pass))) {
@@ -360,8 +409,11 @@ CRYPT_CONTEXT get_ssl_cert(scfg_t *cfg, char **estr, int *level)
}
cryptKeysetClose(ssl_keyset);
- pthread_mutex_unlock(&ssl_cert_mutex);
cfg->tls_certificate = ssl_context;
+ pthread_mutex_lock(&ssl_rwlock);
+ writers--;
+ pthread_mutex_unlock(&ssl_rwlock);
+ pthread_mutex_unlock(&ssl_cert_mutex);
return ssl_context;
failure_return_3:
@@ -370,6 +422,10 @@ failure_return_2:
cryptKeysetClose(ssl_keyset);
failure_return_1:
cryptDestroyContext(ssl_context);
+ cfg->tls_certificate = -1;
+ pthread_mutex_lock(&ssl_rwlock);
+ writers--;
+ pthread_mutex_unlock(&ssl_rwlock);
pthread_mutex_unlock(&ssl_cert_mutex);
return -1;
}
diff --git a/src/sbbs3/websrvr.c b/src/sbbs3/websrvr.c
index 2ef210b496..84751316c1 100644
--- a/src/sbbs3/websrvr.c
+++ b/src/sbbs3/websrvr.c
@@ -6613,18 +6613,17 @@ void http_session_thread(void* arg)
HANDLE_CRYPT_CALL(cryptSetAttribute(session.tls_sess, CRYPT_SESSINFO_SSL_OPTIONS, CRYPT_SSLOPTION_DISABLE_CERTVERIFY), &session, "disabling certificate verification");
HANDLE_CRYPT_CALL(cryptSetAttribute(session.tls_sess, CRYPT_SESSINFO_PRIVATEKEY, scfg.tls_certificate), &session, "setting private key");
}
+ unlock_ssl_cert();
BOOL nodelay=TRUE;
setsockopt(session.socket,IPPROTO_TCP,TCP_NODELAY,(char*)&nodelay,sizeof(nodelay));
//HANDLE_CRYPT_CALL(cryptSetAttribute(session.tls_sess, CRYPT_SESSINFO_SSL_OPTIONS, CRYPT_SSLOPTION_MINVER_TLS12), &session, "setting TLS minver to 1.2");
HANDLE_CRYPT_CALL(cryptSetAttribute(session.tls_sess, CRYPT_SESSINFO_NETWORKSOCKET, session.socket), &session, "setting network socket");
if (!HANDLE_CRYPT_CALL(cryptSetAttribute(session.tls_sess, CRYPT_SESSINFO_ACTIVE, 1), &session, "setting session active")) {
- unlock_ssl_cert();
close_session_no_rb(&session);
thread_down();
return;
}
- unlock_ssl_cert();
HANDLE_CRYPT_CALL(cryptSetAttribute(session.tls_sess, CRYPT_OPTION_NET_READTIMEOUT, 0), &session, "setting read timeout");
}
@@ -7239,10 +7238,12 @@ void web_server(void* arg)
* Add interfaces
*/
xpms_add_list(ws_set, PF_UNSPEC, SOCK_STREAM, 0, startup->interfaces, startup->port, "Web Server", open_socket, startup->seteuid, NULL);
+ lock_ssl_cert();
if(scfg.tls_certificate != -1 && startup->options&WEB_OPT_ALLOW_TLS) {
if(do_cryptInit())
xpms_add_list(ws_set, PF_UNSPEC, SOCK_STREAM, 0, startup->tls_interfaces, startup->tls_port, "Secure Web Server", open_socket, startup->seteuid, "TLS");
}
+ unlock_ssl_cert();
listInit(&log_list,/* flags */ LINK_LIST_MUTEX|LINK_LIST_SEMAPHORE);
if(startup->options&WEB_OPT_HTTP_LOGGING) {
--
GitLab