Skip to content

tcp: fix SSL_CONF_cmd/SSL_new ordering in OpenSSL client connect path #291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Cropi wants to merge 1 commit into
base: master
Choose a base branch
from
Open

tcp: fix SSL_CONF_cmd/SSL_new ordering in OpenSSL client connect path #291

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions src/tcp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1891,6 +1891,15 @@ relpTcpConnectTLSInit_ossl(relpTcp_t *const pThis)
/*if we reach this point we are in tls mode */
pThis->pEngine->dbgprint((char*)"relpTcpConnectTLSInit: TLS Mode\n");

/* set before relpTcpSetSslConfCmd_ossl: tlsConfigCmd may contain
* flag-restricted commands that require SSL_CONF_FLAG_CLIENT */
pThis->sslState = osslClient;

/* SSL_CONF_cmd targets the SSL_CTX; SSL_new() snapshots the SSL_CTX's
* group list at construction time, so this must run before SSL_new()
* or the SSL object keeps the default groups set by SSL_CTX_new() */
CHKRet(relpTcpSetSslConfCmd_ossl(pThis, pThis->tlsConfigCmd));
Comment on lines +1894 to +1901

@gemini-code-assist gemini-code-assist Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Applying configuration commands to the global SSL_CTX (ctx) via SSL_CONF_cmd is problematic in a multi-threaded environment like librelp/rsyslog. Since ctx is a single static global variable shared across all client and server connections:

  1. Data Races: Concurrent connection initializations on different threads will call SSL_CONF_cmd on the same SSL_CTX simultaneously without locking, leading to undefined behavior or crashes.
  2. Configuration Pollution/Leakage: If different connections (or clients and servers) have different tlsConfigCmd settings, they will overwrite each other's configurations on the global ctx. Whichever connection is created next via SSL_new(ctx) will inherit the configuration of the last connection that called SSL_CONF_cmd.

Recommended Solution

Instead of targeting the global SSL_CTX, the configuration should be applied directly to the per-connection SSL object (pThis->ssl) using SSL_CONF_CTX_set_ssl instead of SSL_CONF_CTX_set_ssl_ctx.

To implement this:

  1. Keep relpTcpSetSslConfCmd_ossl after SSL_new (as it was originally) so that pThis->ssl is already allocated.
  2. Modify relpTcpSetSslConfCmd_ossl (around line 1696) to target the SSL object if available:
if (pThis->ssl != NULL) {
    SSL_CONF_CTX_set_ssl(cctx, pThis->ssl);
} else {
    SSL_CONF_CTX_set_ssl_ctx(cctx, ctx);
}

This ensures thread safety, prevents configuration leakage between connections, and correctly applies the configuration to the active connection.

@Cropi Cropi Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The race condition is real but pre-dates this patch. The original code called relpTcpSetSslConfCmd_ossl on the same shared global SSL_CTX too just after SSL_new instead of before. This PR didn't introduce it.

@cubic-dev-ai cubic-dev-ai Bot Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Race condition: per-connection TLS config modifies shared global SSL_CTX without synchronization before SSL_new

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/tcp.c, line 1901:

<comment>Race condition: per-connection TLS config modifies shared global SSL_CTX without synchronization before SSL_new</comment>

<file context>
@@ -1891,6 +1891,15 @@ relpTcpConnectTLSInit_ossl(relpTcp_t *const pThis)
+	/* SSL_CONF_cmd targets the SSL_CTX; SSL_new() snapshots the SSL_CTX's
+	 * group list at construction time, so this must run before SSL_new()
+	 * or the SSL object keeps the default groups set by SSL_CTX_new() */
+	CHKRet(relpTcpSetSslConfCmd_ossl(pThis, pThis->tlsConfigCmd));
+
 	if(!(pThis->ssl = SSL_new(ctx))) {
</file context>
Fix with cubic


if(!(pThis->ssl = SSL_new(ctx))) {
relpTcpLastSSLErrorMsg(0, pThis, "relpTcpConnectTLSInit");
ABORT_FINALIZE(RELP_RET_IO_ERR);
Expand All @@ -1906,17 +1915,11 @@ relpTcpConnectTLSInit_ossl(relpTcp_t *const pThis)
} else
pThis->authmode = eRelpAuthMode_None;

/* Set TLS Options if configured */
CHKRet(relpTcpSetSslConfCmd_ossl(pThis, pThis->tlsConfigCmd));

/* Set TLS Priority Options */
CHKRet(relpTcpTLSSetPrio(pThis));

SSL_set_ex_data(pThis->ssl, 0, (void*)pThis);

/*set client state */
pThis->sslState = osslClient;

/* Create BIO from ptcp socket! */
conn = BIO_new_socket(pThis->sock, BIO_CLOSE /*BIO_NOCLOSE*/);
pThis->pEngine->dbgprint((char*)"relpTcpConnectTLSInit: Init conn BIO[%p] done\n", (void *)conn);
Expand Down
Loading