You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
290 lines
10 KiB
290 lines
10 KiB
From d5f95aa066f878b0aef6a64e60b61e8626e664cd Mon Sep 17 00:00:00 2001
|
|
From: Nanang Izzuddin <nanang@teluu.com>
|
|
Date: Fri, 23 Jul 2021 10:49:21 +0700
|
|
Subject: [PATCH] Merge pull request from GHSA-cv8x-p47p-99wr
|
|
|
|
* - Avoid SSL socket parent/listener getting destroyed during handshake by increasing parent's reference count.
|
|
- Add missing SSL socket close when the newly accepted SSL socket is discarded in SIP TLS transport.
|
|
|
|
* - Fix silly mistake: accepted active socket created without group lock in SSL socket.
|
|
- Replace assertion with normal validation check of SSL socket instance in OpenSSL verification callback (verify_cb()) to avoid crash, e.g: if somehow race condition with SSL socket destroy happens or OpenSSL application data index somehow gets corrupted.
|
|
---
|
|
pjlib/src/pj/ssl_sock_imp_common.c | 47 +++++++++++++++++++++--------
|
|
pjlib/src/pj/ssl_sock_ossl.c | 45 ++++++++++++++++++++++-----
|
|
pjsip/src/pjsip/sip_transport_tls.c | 23 +++++++++++++-
|
|
3 files changed, 95 insertions(+), 20 deletions(-)
|
|
|
|
diff --git a/pjlib/src/pj/ssl_sock_imp_common.c b/pjlib/src/pj/ssl_sock_imp_common.c
|
|
index 025832da4..24533b397 100644
|
|
--- a/pjlib/src/pj/ssl_sock_imp_common.c
|
|
+++ b/pjlib/src/pj/ssl_sock_imp_common.c
|
|
@@ -255,6 +255,8 @@ static pj_bool_t on_handshake_complete(pj_ssl_sock_t *ssock,
|
|
|
|
/* Accepting */
|
|
if (ssock->is_server) {
|
|
+ pj_bool_t ret = PJ_TRUE;
|
|
+
|
|
if (status != PJ_SUCCESS) {
|
|
/* Handshake failed in accepting, destroy our self silently. */
|
|
|
|
@@ -272,6 +274,12 @@ static pj_bool_t on_handshake_complete(pj_ssl_sock_t *ssock,
|
|
status);
|
|
}
|
|
|
|
+ /* Decrement ref count of parent */
|
|
+ if (ssock->parent->param.grp_lock) {
|
|
+ pj_grp_lock_dec_ref(ssock->parent->param.grp_lock);
|
|
+ ssock->parent = NULL;
|
|
+ }
|
|
+
|
|
/* Originally, this is a workaround for ticket #985. However,
|
|
* a race condition may occur in multiple worker threads
|
|
* environment when we are destroying SSL objects while other
|
|
@@ -315,23 +323,29 @@ static pj_bool_t on_handshake_complete(pj_ssl_sock_t *ssock,
|
|
|
|
return PJ_FALSE;
|
|
}
|
|
+
|
|
/* Notify application the newly accepted SSL socket */
|
|
if (ssock->param.cb.on_accept_complete2) {
|
|
- pj_bool_t ret;
|
|
ret = (*ssock->param.cb.on_accept_complete2)
|
|
(ssock->parent, ssock, (pj_sockaddr_t*)&ssock->rem_addr,
|
|
pj_sockaddr_get_len((pj_sockaddr_t*)&ssock->rem_addr),
|
|
status);
|
|
- if (ret == PJ_FALSE)
|
|
- return PJ_FALSE;
|
|
} else if (ssock->param.cb.on_accept_complete) {
|
|
- pj_bool_t ret;
|
|
ret = (*ssock->param.cb.on_accept_complete)
|
|
(ssock->parent, ssock, (pj_sockaddr_t*)&ssock->rem_addr,
|
|
pj_sockaddr_get_len((pj_sockaddr_t*)&ssock->rem_addr));
|
|
- if (ret == PJ_FALSE)
|
|
- return PJ_FALSE;
|
|
}
|
|
+
|
|
+ /* Decrement ref count of parent and reset parent (we don't need it
|
|
+ * anymore, right?).
|
|
+ */
|
|
+ if (ssock->parent->param.grp_lock) {
|
|
+ pj_grp_lock_dec_ref(ssock->parent->param.grp_lock);
|
|
+ ssock->parent = NULL;
|
|
+ }
|
|
+
|
|
+ if (ret == PJ_FALSE)
|
|
+ return PJ_FALSE;
|
|
}
|
|
|
|
/* Connecting */
|
|
@@ -930,9 +944,13 @@ static pj_bool_t ssock_on_accept_complete (pj_ssl_sock_t *ssock_parent,
|
|
if (status != PJ_SUCCESS)
|
|
goto on_return;
|
|
|
|
+ /* Set parent and add ref count (avoid parent destroy during handshake) */
|
|
+ ssock->parent = ssock_parent;
|
|
+ if (ssock->parent->param.grp_lock)
|
|
+ pj_grp_lock_add_ref(ssock->parent->param.grp_lock);
|
|
+
|
|
/* Update new SSL socket attributes */
|
|
ssock->sock = newsock;
|
|
- ssock->parent = ssock_parent;
|
|
ssock->is_server = PJ_TRUE;
|
|
if (ssock_parent->cert) {
|
|
status = pj_ssl_sock_set_certificate(ssock, ssock->pool,
|
|
@@ -957,16 +975,20 @@ static pj_bool_t ssock_on_accept_complete (pj_ssl_sock_t *ssock_parent,
|
|
ssock->asock_rbuf = (void**)pj_pool_calloc(ssock->pool,
|
|
ssock->param.async_cnt,
|
|
sizeof(void*));
|
|
- if (!ssock->asock_rbuf)
|
|
- return PJ_ENOMEM;
|
|
+ if (!ssock->asock_rbuf) {
|
|
+ status = PJ_ENOMEM;
|
|
+ goto on_return;
|
|
+ }
|
|
|
|
for (i = 0; i<ssock->param.async_cnt; ++i) {
|
|
ssock->asock_rbuf[i] = (void*) pj_pool_alloc(
|
|
ssock->pool,
|
|
ssock->param.read_buffer_size +
|
|
sizeof(read_data_t*));
|
|
- if (!ssock->asock_rbuf[i])
|
|
- return PJ_ENOMEM;
|
|
+ if (!ssock->asock_rbuf[i]) {
|
|
+ status = PJ_ENOMEM;
|
|
+ goto on_return;
|
|
+ }
|
|
}
|
|
|
|
/* If listener socket has group lock, automatically create group lock
|
|
@@ -980,7 +1002,7 @@ static pj_bool_t ssock_on_accept_complete (pj_ssl_sock_t *ssock_parent,
|
|
goto on_return;
|
|
|
|
pj_grp_lock_add_ref(glock);
|
|
- asock_cfg.grp_lock = ssock->param.grp_lock = glock;
|
|
+ ssock->param.grp_lock = glock;
|
|
pj_grp_lock_add_handler(ssock->param.grp_lock, ssock->pool, ssock,
|
|
ssl_on_destroy);
|
|
}
|
|
@@ -1008,6 +1030,7 @@ static pj_bool_t ssock_on_accept_complete (pj_ssl_sock_t *ssock_parent,
|
|
|
|
/* Create active socket */
|
|
pj_activesock_cfg_default(&asock_cfg);
|
|
+ asock_cfg.grp_lock = ssock->param.grp_lock;
|
|
asock_cfg.async_cnt = ssock->param.async_cnt;
|
|
asock_cfg.concurrency = ssock->param.concurrency;
|
|
asock_cfg.whole_data = PJ_TRUE;
|
|
diff --git a/pjlib/src/pj/ssl_sock_ossl.c b/pjlib/src/pj/ssl_sock_ossl.c
|
|
index 88a2a6b94..df4f4f96a 100644
|
|
--- a/pjlib/src/pj/ssl_sock_ossl.c
|
|
+++ b/pjlib/src/pj/ssl_sock_ossl.c
|
|
@@ -327,7 +327,8 @@ static pj_status_t STATUS_FROM_SSL_ERR(char *action, pj_ssl_sock_t *ssock,
|
|
ERROR_LOG("STATUS_FROM_SSL_ERR", err, ssock);
|
|
}
|
|
|
|
- ssock->last_err = err;
|
|
+ if (ssock)
|
|
+ ssock->last_err = err;
|
|
return GET_STATUS_FROM_SSL_ERR(err);
|
|
}
|
|
|
|
@@ -344,7 +345,8 @@ static pj_status_t STATUS_FROM_SSL_ERR2(char *action, pj_ssl_sock_t *ssock,
|
|
/* Dig for more from OpenSSL error queue */
|
|
SSLLogErrors(action, ret, err, len, ssock);
|
|
|
|
- ssock->last_err = ssl_err;
|
|
+ if (ssock)
|
|
+ ssock->last_err = ssl_err;
|
|
return GET_STATUS_FROM_SSL_ERR(ssl_err);
|
|
}
|
|
|
|
@@ -786,6 +788,13 @@ static pj_status_t init_openssl(void)
|
|
|
|
/* Create OpenSSL application data index for SSL socket */
|
|
sslsock_idx = SSL_get_ex_new_index(0, "SSL socket", NULL, NULL, NULL);
|
|
+ if (sslsock_idx == -1) {
|
|
+ status = STATUS_FROM_SSL_ERR2("Init", NULL, -1, ERR_get_error(), 0);
|
|
+ PJ_LOG(1,(THIS_FILE,
|
|
+ "Fatal error: failed to get application data index for "
|
|
+ "SSL socket"));
|
|
+ return status;
|
|
+ }
|
|
|
|
#if defined(PJ_SSL_SOCK_OSSL_USE_THREAD_CB) && \
|
|
PJ_SSL_SOCK_OSSL_USE_THREAD_CB != 0 && OPENSSL_VERSION_NUMBER < 0x10100000L
|
|
@@ -819,21 +828,36 @@ static int password_cb(char *buf, int num, int rwflag, void *user_data)
|
|
}
|
|
|
|
|
|
-/* SSL password callback. */
|
|
+/* SSL certificate verification result callback.
|
|
+ * Note that this callback seems to be always called from library worker
|
|
+ * thread, e.g: active socket on_read_complete callback, which should have
|
|
+ * already been equipped with race condition avoidance mechanism (should not
|
|
+ * be destroyed while callback is being invoked).
|
|
+ */
|
|
static int verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx)
|
|
{
|
|
- pj_ssl_sock_t *ssock;
|
|
- SSL *ossl_ssl;
|
|
+ pj_ssl_sock_t *ssock = NULL;
|
|
+ SSL *ossl_ssl = NULL;
|
|
int err;
|
|
|
|
/* Get SSL instance */
|
|
ossl_ssl = X509_STORE_CTX_get_ex_data(x509_ctx,
|
|
SSL_get_ex_data_X509_STORE_CTX_idx());
|
|
- pj_assert(ossl_ssl);
|
|
+ if (!ossl_ssl) {
|
|
+ PJ_LOG(1,(THIS_FILE,
|
|
+ "SSL verification callback failed to get SSL instance"));
|
|
+ goto on_return;
|
|
+ }
|
|
|
|
/* Get SSL socket instance */
|
|
ssock = SSL_get_ex_data(ossl_ssl, sslsock_idx);
|
|
- pj_assert(ssock);
|
|
+ if (!ssock) {
|
|
+ /* SSL socket may have been destroyed */
|
|
+ PJ_LOG(1,(THIS_FILE,
|
|
+ "SSL verification callback failed to get SSL socket "
|
|
+ "instance (sslsock_idx=%d).", sslsock_idx));
|
|
+ goto on_return;
|
|
+ }
|
|
|
|
/* Store verification status */
|
|
err = X509_STORE_CTX_get_error(x509_ctx);
|
|
@@ -911,6 +935,7 @@ static int verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx)
|
|
if (PJ_FALSE == ssock->param.verify_peer)
|
|
preverify_ok = 1;
|
|
|
|
+on_return:
|
|
return preverify_ok;
|
|
}
|
|
|
|
@@ -1474,6 +1499,12 @@ static void ssl_destroy(pj_ssl_sock_t *ssock)
|
|
static void ssl_reset_sock_state(pj_ssl_sock_t *ssock)
|
|
{
|
|
ossl_sock_t *ossock = (ossl_sock_t *)ssock;
|
|
+
|
|
+ /* Detach from SSL instance */
|
|
+ if (ossock->ossl_ssl) {
|
|
+ SSL_set_ex_data(ossock->ossl_ssl, sslsock_idx, NULL);
|
|
+ }
|
|
+
|
|
/**
|
|
* Avoid calling SSL_shutdown() if handshake wasn't completed.
|
|
* OpenSSL 1.0.2f complains if SSL_shutdown() is called during an
|
|
diff --git a/pjsip/src/pjsip/sip_transport_tls.c b/pjsip/src/pjsip/sip_transport_tls.c
|
|
index 56a06cf99..24e43ef60 100644
|
|
--- a/pjsip/src/pjsip/sip_transport_tls.c
|
|
+++ b/pjsip/src/pjsip/sip_transport_tls.c
|
|
@@ -1333,9 +1333,26 @@ static pj_bool_t on_accept_complete2(pj_ssl_sock_t *ssock,
|
|
PJ_UNUSED_ARG(src_addr_len);
|
|
|
|
listener = (struct tls_listener*) pj_ssl_sock_get_user_data(ssock);
|
|
+ if (!listener) {
|
|
+ /* Listener already destroyed, e.g: after TCP accept but before SSL
|
|
+ * handshake is completed.
|
|
+ */
|
|
+ if (new_ssock && accept_status == PJ_SUCCESS) {
|
|
+ /* Close the SSL socket if the accept op is successful */
|
|
+ PJ_LOG(4,(THIS_FILE,
|
|
+ "Incoming TLS connection from %s (sock=%d) is discarded "
|
|
+ "because listener is already destroyed",
|
|
+ pj_sockaddr_print(src_addr, addr, sizeof(addr), 3),
|
|
+ new_ssock));
|
|
+
|
|
+ pj_ssl_sock_close(new_ssock);
|
|
+ }
|
|
+
|
|
+ return PJ_FALSE;
|
|
+ }
|
|
|
|
if (accept_status != PJ_SUCCESS) {
|
|
- if (listener && listener->tls_setting.on_accept_fail_cb) {
|
|
+ if (listener->tls_setting.on_accept_fail_cb) {
|
|
pjsip_tls_on_accept_fail_param param;
|
|
pj_ssl_sock_info ssi;
|
|
|
|
@@ -1358,6 +1375,8 @@ static pj_bool_t on_accept_complete2(pj_ssl_sock_t *ssock,
|
|
PJ_ASSERT_RETURN(new_ssock, PJ_TRUE);
|
|
|
|
if (!listener->is_registered) {
|
|
+ pj_ssl_sock_close(new_ssock);
|
|
+
|
|
if (listener->tls_setting.on_accept_fail_cb) {
|
|
pjsip_tls_on_accept_fail_param param;
|
|
pj_bzero(¶m, sizeof(param));
|
|
@@ -1409,6 +1428,8 @@ static pj_bool_t on_accept_complete2(pj_ssl_sock_t *ssock,
|
|
ssl_info.grp_lock, &tls);
|
|
|
|
if (status != PJ_SUCCESS) {
|
|
+ pj_ssl_sock_close(new_ssock);
|
|
+
|
|
if (listener->tls_setting.on_accept_fail_cb) {
|
|
pjsip_tls_on_accept_fail_param param;
|
|
pj_bzero(¶m, sizeof(param));
|
|
--
|
|
2.31.1
|
|
|