CVE-2021-32686: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition') (CWE-362)
value: PJSIP is a free and open source multimedia communication library written in C language implementing standard based protocols such as SIP, SDP, RTP, STUN, TURN, and ICE. In PJSIP before version 2.11.1, there are a couple of issues found in the SSL socket. First, a race condition between callback and destroy, due to the accepted socket having no group lock. Second, the SSL socket parent/listener may get destroyed during handshake. Both issues were reported to happen intermittently in heavy load TLS connections. They cause a crash, resulting in a denial of service. These are fixed in version 2.11.1.
CWE description: The product contains a code sequence that can run concurrently with other code, and the code sequence requires temporary, exclusive access to a shared resource, but a timing window exists in which the shared resource can be modified by another code sequence that is operating concurrently.
CVSS v3 base score: 5.9
Published: 2021-07-23T22:15Z
Repository: https://github.com/pjsip/pjproject
Language: C
Files changed: pjlib/src/pj/ssl_sock_imp_common.c, pjlib/src/pj/ssl_sock_ossl.c, pjsip/src/pjsip/sip_transport_tls.c
Commit date: 2021-07-23 11:49:21 +0800
Commit message: 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.
Diff: 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
@@ -253,10 +253,12 @@ static pj_bool_t on_handshake_complete(pj_ssl_sock_t *ssock,
     if (status == PJ_SUCCESS)
 	ssl_update_certs_info(ssock);
 
     /* Accepting */
     if (ssock->is_server) {
+	pj_bool_t ret = PJ_TRUE;
+
 	if (status != PJ_SUCCESS) {
 	    /* Handshake failed in accepting, destroy our self silently. */
 
 	    char buf[PJ_INET6_ADDRSTRLEN+10];
 
@@ -270,10 +272,16 @@ static pj_bool_t on_handshake_complete(pj_ssl_sock_t *ssock,
 		      (ssock->parent, ssock, (pj_sockaddr_t*)&ssock->rem_addr, 
 		      pj_sockaddr_get_len((pj_sockaddr_t*)&ssock->rem_addr), 
 		      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
 	     * threads are still accessing them.
 	     * Please see ticket #1930 for more info.
@@ -313,27 +321,33 @@ static pj_bool_t on_handshake_complete(pj_ssl_sock_t *ssock,
 	    }
 #endif
 
 	    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 */
     else {
 	/* On failure, reset SSL socket state first, as app may try to 
@@ -928,13 +942,17 @@ static pj_bool_t ssock_on_accept_complete (pj_ssl_sock_t *ssock_parent,
     status = pj_ssl_sock_create(ssock_parent->pool,
 				&ssock_parent->newsock_param, &ssock);
     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, 
 					     ssock_parent->cert);
 	if (status != PJ_SUCCESS)
@@ -955,20 +973,24 @@ static pj_bool_t ssock_on_accept_complete (pj_ssl_sock_t *ssock_parent,
 
     /* Prepare read buffer */
     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
      * for the new socket.
      */
@@ -978,11 +1000,11 @@ static pj_bool_t ssock_on_accept_complete (pj_ssl_sock_t *ssock_parent,
 	status = pj_grp_lock_create(ssock->pool, NULL, &glock);
 	if (status != PJ_SUCCESS)
 	    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);
     }
 
 #ifdef SSL_SOCK_IMP_USE_OWN_NETWORK
@@ -1006,10 +1028,11 @@ static pj_bool_t ssock_on_accept_complete (pj_ssl_sock_t *ssock_parent,
 	    goto on_return;
     }
 
     /* 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;
 
     pj_bzero(&asock_cb, sizeof(asock_cb));
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
@@ -325,11 +325,12 @@ static pj_status_t STATUS_FROM_SSL_ERR(char *action, pj_ssl_sock_t *ssock,
     if (err == SSL_ERROR_SSL) {
 	err = ERR_get_error();
 	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);
 }
 
 /* err contains SSL_get_error() status */
 static pj_status_t STATUS_FROM_SSL_ERR2(char *action, pj_ssl_sock_t *ssock,
@@ -342,11 +343,12 @@ 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);
 }
 
 static pj_status_t GET_SSL_STATUS(pj_ssl_sock_t *ssock)
 {
@@ -784,10 +786,17 @@ static pj_status_t init_openssl(void)
 	SSL_CTX_free(ctx);
     }
 
     /* 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
 
     status = init_ossl_lock();
@@ -817,25 +826,40 @@ static int password_cb(char *buf, int num, int rwflag, void *user_data)
     pj_memcpy(buf, cert->privkey_pass.ptr, cert->privkey_pass.slen);
     return (int)cert->privkey_pass.slen;
 }
 
 
-/* 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);
     switch (err) {
     case X509_V_OK:
@@ -909,10 +933,11 @@ static int verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx)
      * application can still get the verification status.
      */
     if (PJ_FALSE == ssock->param.verify_peer)
 	preverify_ok = 1;
 
+on_return:
     return preverify_ok;
 }
 
 /* Setting SSL sock cipher list */
 static pj_status_t set_cipher_list(pj_ssl_sock_t *ssock);
@@ -1472,10 +1497,16 @@ static void ssl_destroy(pj_ssl_sock_t *ssock)
 
 /* Reset SSL socket state */
 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
      * SSL handshake, while previous versions always return 0.
      */
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
@@ -1331,13 +1331,30 @@ static pj_bool_t on_accept_complete2(pj_ssl_sock_t *ssock,
     char addr_buf[PJ_INET6_ADDRSTRLEN+10];        
 
     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;
 
 	    pj_bzero(&param, sizeof(param));
 	    param.status = accept_status;
@@ -1356,10 +1373,12 @@ 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(&param, sizeof(param));
 	    param.status = PJSIP_TLS_EACCEPT;
 	    param.local_addr = &listener->factory.local_addr;
@@ -1407,10 +1426,12 @@ static pj_bool_t on_accept_complete2(pj_ssl_sock_t *ssock,
     status = tls_create( listener, NULL, new_ssock, PJ_TRUE,
 			 &ssl_info.local_addr, &tmp_src_addr, NULL,
 			 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(&param, sizeof(param));
 	    param.status = status;
 	    param.local_addr = &listener->factory.local_addr;

Vulnerable code: 	    pj_bool_t ret;
	    if (ret == PJ_FALSE)
		return PJ_FALSE;	
	    pj_bool_t ret;
	    if (ret == PJ_FALSE)
		return PJ_FALSE;
    ssock->parent = ssock_parent;
    if (!ssock->asock_rbuf)
        return PJ_ENOMEM;
        if (!ssock->asock_rbuf[i])
            return PJ_ENOMEM;
	asock_cfg.grp_lock = ssock->param.grp_lock = glock;
    ssock->last_err = err;
    ssock->last_err = ssl_err;
/* SSL password callback. */
    pj_ssl_sock_t *ssock;
    SSL *ossl_ssl;
    pj_assert(ossl_ssl);
    pj_assert(ssock);
	if (listener && listener->tls_setting.on_accept_fail_cb) {
Fixed code: 	pj_bool_t ret = PJ_TRUE;

	    /* Decrement ref count of parent */
	    if (ssock->parent->param.grp_lock) {
		pj_grp_lock_dec_ref(ssock->parent->param.grp_lock);
		ssock->parent = NULL;
	    }



	/* 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;
    /* 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);

    if (!ssock->asock_rbuf) {
	status = PJ_ENOMEM;
	goto on_return;
    }
	if (!ssock->asock_rbuf[i]) {
	    status = PJ_ENOMEM;
	    goto on_return;
	}
	ssock->param.grp_lock = glock;
    asock_cfg.grp_lock = ssock->param.grp_lock;
    if (ssock)
	ssock->last_err = err;
    if (ssock)
	ssock->last_err = ssl_err;
    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;
    }
/* 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).
 */
    pj_ssl_sock_t *ssock = NULL;
    SSL *ossl_ssl = NULL;
    if (!ossl_ssl) {
	PJ_LOG(1,(THIS_FILE,
		  "SSL verification callback failed to get SSL instance"));
	goto on_return;
    }
    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;
    }
on_return:

    /* Detach from SSL instance */
    if (ossock->ossl_ssl) {
	SSL_set_ex_data(ossock->ossl_ssl, sslsock_idx, NULL);
    }

    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 (listener->tls_setting.on_accept_fail_cb) {
	pj_ssl_sock_close(new_ssock);

	pj_ssl_sock_close(new_ssock);