Skip to content

Commit d5f95aa

Browse files
authored
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.
1 parent c80f59e commit d5f95aa

File tree

3 files changed

+95
-20
lines changed

3 files changed

+95
-20
lines changed

Diff for: pjlib/src/pj/ssl_sock_imp_common.c

+35-12
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,8 @@ static pj_bool_t on_handshake_complete(pj_ssl_sock_t *ssock,
255255

256256
/* Accepting */
257257
if (ssock->is_server) {
258+
pj_bool_t ret = PJ_TRUE;
259+
258260
if (status != PJ_SUCCESS) {
259261
/* Handshake failed in accepting, destroy our self silently. */
260262

@@ -272,6 +274,12 @@ static pj_bool_t on_handshake_complete(pj_ssl_sock_t *ssock,
272274
status);
273275
}
274276

277+
/* Decrement ref count of parent */
278+
if (ssock->parent->param.grp_lock) {
279+
pj_grp_lock_dec_ref(ssock->parent->param.grp_lock);
280+
ssock->parent = NULL;
281+
}
282+
275283
/* Originally, this is a workaround for ticket #985. However,
276284
* a race condition may occur in multiple worker threads
277285
* 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,
315323

316324
return PJ_FALSE;
317325
}
326+
318327
/* Notify application the newly accepted SSL socket */
319328
if (ssock->param.cb.on_accept_complete2) {
320-
pj_bool_t ret;
321329
ret = (*ssock->param.cb.on_accept_complete2)
322330
(ssock->parent, ssock, (pj_sockaddr_t*)&ssock->rem_addr,
323331
pj_sockaddr_get_len((pj_sockaddr_t*)&ssock->rem_addr),
324332
status);
325-
if (ret == PJ_FALSE)
326-
return PJ_FALSE;
327333
} else if (ssock->param.cb.on_accept_complete) {
328-
pj_bool_t ret;
329334
ret = (*ssock->param.cb.on_accept_complete)
330335
(ssock->parent, ssock, (pj_sockaddr_t*)&ssock->rem_addr,
331336
pj_sockaddr_get_len((pj_sockaddr_t*)&ssock->rem_addr));
332-
if (ret == PJ_FALSE)
333-
return PJ_FALSE;
334337
}
338+
339+
/* Decrement ref count of parent and reset parent (we don't need it
340+
* anymore, right?).
341+
*/
342+
if (ssock->parent->param.grp_lock) {
343+
pj_grp_lock_dec_ref(ssock->parent->param.grp_lock);
344+
ssock->parent = NULL;
345+
}
346+
347+
if (ret == PJ_FALSE)
348+
return PJ_FALSE;
335349
}
336350

337351
/* Connecting */
@@ -930,9 +944,13 @@ static pj_bool_t ssock_on_accept_complete (pj_ssl_sock_t *ssock_parent,
930944
if (status != PJ_SUCCESS)
931945
goto on_return;
932946

947+
/* Set parent and add ref count (avoid parent destroy during handshake) */
948+
ssock->parent = ssock_parent;
949+
if (ssock->parent->param.grp_lock)
950+
pj_grp_lock_add_ref(ssock->parent->param.grp_lock);
951+
933952
/* Update new SSL socket attributes */
934953
ssock->sock = newsock;
935-
ssock->parent = ssock_parent;
936954
ssock->is_server = PJ_TRUE;
937955
if (ssock_parent->cert) {
938956
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,
957975
ssock->asock_rbuf = (void**)pj_pool_calloc(ssock->pool,
958976
ssock->param.async_cnt,
959977
sizeof(void*));
960-
if (!ssock->asock_rbuf)
961-
return PJ_ENOMEM;
978+
if (!ssock->asock_rbuf) {
979+
status = PJ_ENOMEM;
980+
goto on_return;
981+
}
962982

963983
for (i = 0; i<ssock->param.async_cnt; ++i) {
964984
ssock->asock_rbuf[i] = (void*) pj_pool_alloc(
965985
ssock->pool,
966986
ssock->param.read_buffer_size +
967987
sizeof(read_data_t*));
968-
if (!ssock->asock_rbuf[i])
969-
return PJ_ENOMEM;
988+
if (!ssock->asock_rbuf[i]) {
989+
status = PJ_ENOMEM;
990+
goto on_return;
991+
}
970992
}
971993

972994
/* 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,
9801002
goto on_return;
9811003

9821004
pj_grp_lock_add_ref(glock);
983-
asock_cfg.grp_lock = ssock->param.grp_lock = glock;
1005+
ssock->param.grp_lock = glock;
9841006
pj_grp_lock_add_handler(ssock->param.grp_lock, ssock->pool, ssock,
9851007
ssl_on_destroy);
9861008
}
@@ -1008,6 +1030,7 @@ static pj_bool_t ssock_on_accept_complete (pj_ssl_sock_t *ssock_parent,
10081030

10091031
/* Create active socket */
10101032
pj_activesock_cfg_default(&asock_cfg);
1033+
asock_cfg.grp_lock = ssock->param.grp_lock;
10111034
asock_cfg.async_cnt = ssock->param.async_cnt;
10121035
asock_cfg.concurrency = ssock->param.concurrency;
10131036
asock_cfg.whole_data = PJ_TRUE;

Diff for: pjlib/src/pj/ssl_sock_ossl.c

+38-7
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,8 @@ static pj_status_t STATUS_FROM_SSL_ERR(char *action, pj_ssl_sock_t *ssock,
327327
ERROR_LOG("STATUS_FROM_SSL_ERR", err, ssock);
328328
}
329329

330-
ssock->last_err = err;
330+
if (ssock)
331+
ssock->last_err = err;
331332
return GET_STATUS_FROM_SSL_ERR(err);
332333
}
333334

@@ -344,7 +345,8 @@ static pj_status_t STATUS_FROM_SSL_ERR2(char *action, pj_ssl_sock_t *ssock,
344345
/* Dig for more from OpenSSL error queue */
345346
SSLLogErrors(action, ret, err, len, ssock);
346347

347-
ssock->last_err = ssl_err;
348+
if (ssock)
349+
ssock->last_err = ssl_err;
348350
return GET_STATUS_FROM_SSL_ERR(ssl_err);
349351
}
350352

@@ -786,6 +788,13 @@ static pj_status_t init_openssl(void)
786788

787789
/* Create OpenSSL application data index for SSL socket */
788790
sslsock_idx = SSL_get_ex_new_index(0, "SSL socket", NULL, NULL, NULL);
791+
if (sslsock_idx == -1) {
792+
status = STATUS_FROM_SSL_ERR2("Init", NULL, -1, ERR_get_error(), 0);
793+
PJ_LOG(1,(THIS_FILE,
794+
"Fatal error: failed to get application data index for "
795+
"SSL socket"));
796+
return status;
797+
}
789798

790799
#if defined(PJ_SSL_SOCK_OSSL_USE_THREAD_CB) && \
791800
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)
819828
}
820829

821830

822-
/* SSL password callback. */
831+
/* SSL certificate verification result callback.
832+
* Note that this callback seems to be always called from library worker
833+
* thread, e.g: active socket on_read_complete callback, which should have
834+
* already been equipped with race condition avoidance mechanism (should not
835+
* be destroyed while callback is being invoked).
836+
*/
823837
static int verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx)
824838
{
825-
pj_ssl_sock_t *ssock;
826-
SSL *ossl_ssl;
839+
pj_ssl_sock_t *ssock = NULL;
840+
SSL *ossl_ssl = NULL;
827841
int err;
828842

829843
/* Get SSL instance */
830844
ossl_ssl = X509_STORE_CTX_get_ex_data(x509_ctx,
831845
SSL_get_ex_data_X509_STORE_CTX_idx());
832-
pj_assert(ossl_ssl);
846+
if (!ossl_ssl) {
847+
PJ_LOG(1,(THIS_FILE,
848+
"SSL verification callback failed to get SSL instance"));
849+
goto on_return;
850+
}
833851

834852
/* Get SSL socket instance */
835853
ssock = SSL_get_ex_data(ossl_ssl, sslsock_idx);
836-
pj_assert(ssock);
854+
if (!ssock) {
855+
/* SSL socket may have been destroyed */
856+
PJ_LOG(1,(THIS_FILE,
857+
"SSL verification callback failed to get SSL socket "
858+
"instance (sslsock_idx=%d).", sslsock_idx));
859+
goto on_return;
860+
}
837861

838862
/* Store verification status */
839863
err = X509_STORE_CTX_get_error(x509_ctx);
@@ -911,6 +935,7 @@ static int verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx)
911935
if (PJ_FALSE == ssock->param.verify_peer)
912936
preverify_ok = 1;
913937

938+
on_return:
914939
return preverify_ok;
915940
}
916941

@@ -1474,6 +1499,12 @@ static void ssl_destroy(pj_ssl_sock_t *ssock)
14741499
static void ssl_reset_sock_state(pj_ssl_sock_t *ssock)
14751500
{
14761501
ossl_sock_t *ossock = (ossl_sock_t *)ssock;
1502+
1503+
/* Detach from SSL instance */
1504+
if (ossock->ossl_ssl) {
1505+
SSL_set_ex_data(ossock->ossl_ssl, sslsock_idx, NULL);
1506+
}
1507+
14771508
/**
14781509
* Avoid calling SSL_shutdown() if handshake wasn't completed.
14791510
* OpenSSL 1.0.2f complains if SSL_shutdown() is called during an

Diff for: pjsip/src/pjsip/sip_transport_tls.c

+22-1
Original file line numberDiff line numberDiff line change
@@ -1333,9 +1333,26 @@ static pj_bool_t on_accept_complete2(pj_ssl_sock_t *ssock,
13331333
PJ_UNUSED_ARG(src_addr_len);
13341334

13351335
listener = (struct tls_listener*) pj_ssl_sock_get_user_data(ssock);
1336+
if (!listener) {
1337+
/* Listener already destroyed, e.g: after TCP accept but before SSL
1338+
* handshake is completed.
1339+
*/
1340+
if (new_ssock && accept_status == PJ_SUCCESS) {
1341+
/* Close the SSL socket if the accept op is successful */
1342+
PJ_LOG(4,(THIS_FILE,
1343+
"Incoming TLS connection from %s (sock=%d) is discarded "
1344+
"because listener is already destroyed",
1345+
pj_sockaddr_print(src_addr, addr, sizeof(addr), 3),
1346+
new_ssock));
1347+
1348+
pj_ssl_sock_close(new_ssock);
1349+
}
1350+
1351+
return PJ_FALSE;
1352+
}
13361353

13371354
if (accept_status != PJ_SUCCESS) {
1338-
if (listener && listener->tls_setting.on_accept_fail_cb) {
1355+
if (listener->tls_setting.on_accept_fail_cb) {
13391356
pjsip_tls_on_accept_fail_param param;
13401357
pj_ssl_sock_info ssi;
13411358

@@ -1358,6 +1375,8 @@ static pj_bool_t on_accept_complete2(pj_ssl_sock_t *ssock,
13581375
PJ_ASSERT_RETURN(new_ssock, PJ_TRUE);
13591376

13601377
if (!listener->is_registered) {
1378+
pj_ssl_sock_close(new_ssock);
1379+
13611380
if (listener->tls_setting.on_accept_fail_cb) {
13621381
pjsip_tls_on_accept_fail_param param;
13631382
pj_bzero(&param, sizeof(param));
@@ -1409,6 +1428,8 @@ static pj_bool_t on_accept_complete2(pj_ssl_sock_t *ssock,
14091428
ssl_info.grp_lock, &tls);
14101429

14111430
if (status != PJ_SUCCESS) {
1431+
pj_ssl_sock_close(new_ssock);
1432+
14121433
if (listener->tls_setting.on_accept_fail_cb) {
14131434
pjsip_tls_on_accept_fail_param param;
14141435
pj_bzero(&param, sizeof(param));

0 commit comments

Comments
 (0)