[Asterisk-code-review] AST-2021-009 - pjproject-bundled: Avoid crash during handshake for TLS (asterisk[17])
Friendly Automation
asteriskteam at digium.com
Thu Jul 22 16:19:52 CDT 2021
Friendly Automation has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/16188 )
Change subject: AST-2021-009 - pjproject-bundled: Avoid crash during handshake for TLS
......................................................................
AST-2021-009 - pjproject-bundled: Avoid crash during handshake for TLS
If an SSL socket parent/listener was destroyed during the handshake,
depending on timing, it was possible for the handling callback to
attempt access of it after the fact thus causing a crash.
ASTERISK-29415 #close
Change-Id: I105dacdcd130ea7fdd4cf2010ccf35b5eaf1432d
---
A third-party/pjproject/patches/0110-tls-parent-listener-destroyed.patch
A third-party/pjproject/patches/0111-ssl-premature-destroy.patch
2 files changed, 302 insertions(+), 0 deletions(-)
Approvals:
Friendly Automation: Looks good to me, approved; Approved for Submit
diff --git a/third-party/pjproject/patches/0110-tls-parent-listener-destroyed.patch b/third-party/pjproject/patches/0110-tls-parent-listener-destroyed.patch
new file mode 100644
index 0000000..81781f2
--- /dev/null
+++ b/third-party/pjproject/patches/0110-tls-parent-listener-destroyed.patch
@@ -0,0 +1,166 @@
+From bb92c97ea512aa0ef316c9b2335c7d57b84dfc9a Mon Sep 17 00:00:00 2001
+From: Nanang Izzuddin <nanang at teluu.com>
+Date: Wed, 16 Jun 2021 12:12:35 +0700
+Subject: [PATCH 1/2] - 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.
+
+---
+ pjlib/src/pj/ssl_sock_imp_common.c | 44 +++++++++++++++++++++--------
+ pjsip/src/pjsip/sip_transport_tls.c | 23 ++++++++++++++-
+ 2 files changed, 55 insertions(+), 12 deletions(-)
+
+diff --git a/pjlib/src/pj/ssl_sock_imp_common.c b/pjlib/src/pj/ssl_sock_imp_common.c
+index bc468bcb3..abec31805 100644
+--- a/pjlib/src/pj/ssl_sock_imp_common.c
++++ b/pjlib/src/pj/ssl_sock_imp_common.c
+@@ -224,6 +224,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. */
+
+@@ -241,6 +243,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
+@@ -284,23 +292,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 */
+@@ -864,9 +878,13 @@ static pj_bool_t asock_on_accept_complete (pj_activesock_t *asock,
+ 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,
+@@ -913,16 +931,20 @@ static pj_bool_t asock_on_accept_complete (pj_activesock_t *asock,
+ 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->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;
++ }
+ }
+
+ /* Create active socket */
+diff --git a/pjsip/src/pjsip/sip_transport_tls.c b/pjsip/src/pjsip/sip_transport_tls.c
+index 17b7ae3de..ce524d53f 100644
+--- a/pjsip/src/pjsip/sip_transport_tls.c
++++ b/pjsip/src/pjsip/sip_transport_tls.c
+@@ -1325,9 +1325,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;
+
+@@ -1350,6 +1367,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));
+@@ -1401,6 +1420,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));
diff --git a/third-party/pjproject/patches/0111-ssl-premature-destroy.patch b/third-party/pjproject/patches/0111-ssl-premature-destroy.patch
new file mode 100644
index 0000000..9de2915
--- /dev/null
+++ b/third-party/pjproject/patches/0111-ssl-premature-destroy.patch
@@ -0,0 +1,136 @@
+From 68c69f516f95df1faa42e5647e9ce7cfdc41ac38 Mon Sep 17 00:00:00 2001
+From: Nanang Izzuddin <nanang at teluu.com>
+Date: Wed, 16 Jun 2021 12:15:29 +0700
+Subject: [PATCH 2/2] - 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 | 3 +-
+ pjlib/src/pj/ssl_sock_ossl.c | 45 +++++++++++++++++++++++++-----
+ 2 files changed, 40 insertions(+), 8 deletions(-)
+
+diff --git a/pjlib/src/pj/ssl_sock_imp_common.c b/pjlib/src/pj/ssl_sock_imp_common.c
+index bc468bcb3..c2b8a846b 100644
+--- a/pjlib/src/pj/ssl_sock_imp_common.c
++++ b/pjlib/src/pj/ssl_sock_imp_common.c
+@@ -927,6 +927,7 @@ static pj_bool_t asock_on_accept_complete (pj_activesock_t *asock,
+
+ /* 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;
+@@ -942,7 +943,7 @@ static pj_bool_t asock_on_accept_complete (pj_activesock_t *asock,
+ 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);
+ }
+diff --git a/pjlib/src/pj/ssl_sock_ossl.c b/pjlib/src/pj/ssl_sock_ossl.c
+index a95b339a5..56841f80a 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);
+ }
+
+@@ -587,6 +589,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;
++ }
+
+ return status;
+ }
+@@ -614,21 +623,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);
+@@ -706,6 +730,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;
+ }
+
+@@ -1213,6 +1238,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
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16188
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 17
Gerrit-Change-Id: I105dacdcd130ea7fdd4cf2010ccf35b5eaf1432d
Gerrit-Change-Number: 16188
Gerrit-PatchSet: 2
Gerrit-Owner: Friendly Automation
Gerrit-Reviewer: Friendly Automation
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210722/4b4d6739/attachment-0001.html>
More information about the asterisk-code-review
mailing list