From d5f95aa066f878b0aef6a64e60b61e8626e664cd Mon Sep 17 00:00:00 2001 From: Nanang Izzuddin 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; iparam.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