[Asterisk-code-review] res pjsip transport websocket: Fix use-after-free bugs. (asterisk[master])

[Ivan Poddubny]

Ivan Poddubny has uploaded a new change for review.

  https://gerrit.asterisk.org/598

Change subject: res_pjsip_transport_websocket: Fix use-after-free bugs.
......................................................................

res_pjsip_transport_websocket: Fix use-after-free bugs.

This patch fixes use-after-free bugs caught by AddressSanitizer.

1. PJSIP transport manager may decide to destroy transport on its own.
For example, when the contact registered via websocket has not renewed
its registration in time. The transport was destoyed, but the websocket
listener thread was still active until the socket closes, and then tried
to call transport_shutdown on transport that has been freed.

This is fixed by adding a group lock that ensures that transport is
destroyed only when both websocket listener and pjsip transport manager
are finished with it.

2. The websocket listener deletes the last reference on websocket session
when the tcp connection is closed, and it gets destroyed, but
the transport manager may still use it, for example when disconnect
happens in the middle of a SIP transaction.

A new reference to websocket session has been added that is released
with the transport to prevent this.

3. The transport destructor accessed wstransport->rdata.tp_info.pool
right after freeing memory that contained wstransport itself.

ASTERISK-25096 #close
Reported by: Josh Kitchens

ASTERISK-24963 #close
Reported by: Badalian Vyacheslav

Change-Id: Idc0b63eb6e459c1ddfb2430127d34b3c4d8d373b
---
M res/res_pjsip_transport_websocket.c
1 file changed, 61 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/98/598/1

diff --git a/res/res_pjsip_transport_websocket.c b/res/res_pjsip_transport_websocket.c
index 914c8b8..be81e94 100644
--- a/res/res_pjsip_transport_websocket.c
+++ b/res/res_pjsip_transport_websocket.c
@@ -48,6 +48,7 @@
 	pjsip_transport transport;
 	pjsip_rx_data rdata;
 	struct ast_websocket *ws_session;
+	pj_grp_lock_t *grp_lock;
 };
 
 /*!
@@ -80,28 +81,56 @@
 {
 	struct ws_transport *wstransport = (struct ws_transport *)transport;
 
-	if (wstransport->transport.ref_cnt) {
-		pj_atomic_destroy(wstransport->transport.ref_cnt);
-	}
-
-	if (wstransport->transport.lock) {
-		pj_lock_destroy(wstransport->transport.lock);
-	}
-
-	pjsip_endpt_release_pool(wstransport->transport.endpt, wstransport->transport.pool);
-
-	if (wstransport->rdata.tp_info.pool) {
-		pjsip_endpt_release_pool(wstransport->transport.endpt, wstransport->rdata.tp_info.pool);
-	}
+	pj_grp_lock_dec_ref(wstransport->grp_lock);
 
 	return PJ_SUCCESS;
 }
 
+static void ws_on_destroy(void *arg)
+{
+	struct ws_transport *wstransport = (struct ws_transport*)arg;
+
+	if (wstransport->ws_session) {
+		if (ast_websocket_fd(wstransport->ws_session) > 0) {
+			ast_websocket_close(wstransport->ws_session, 1000);
+		}
+		ast_websocket_unref(wstransport->ws_session);
+		wstransport->ws_session = NULL;
+	}
+
+	if (wstransport->transport.ref_cnt) {
+		pj_atomic_destroy(wstransport->transport.ref_cnt);
+		wstransport->transport.ref_cnt = NULL;
+	}
+
+	if (wstransport->transport.lock) {
+		pj_lock_destroy(wstransport->transport.lock);
+		wstransport->transport.lock = NULL;
+	}
+
+	if (wstransport->rdata.tp_info.pool) {
+		pjsip_endpt_release_pool(wstransport->transport.endpt, wstransport->rdata.tp_info.pool);
+		wstransport->rdata.tp_info.pool = NULL;
+	}
+
+	if (wstransport->transport.pool) {
+		pj_pool_t *pool;
+
+		pool = wstransport->transport.pool;
+		wstransport->transport.pool = NULL;
+		pjsip_endpt_release_pool(wstransport->transport.endpt, pool);
+	}
+}
+
 static int transport_shutdown(void *data)
 {
-	pjsip_transport *transport = data;
+	struct ws_transport *wstransport = data;
 
-	pjsip_transport_shutdown(transport);
+	if (!wstransport->transport.is_shutdown && !wstransport->transport.is_destroying) {
+		pjsip_transport_shutdown(&wstransport->transport);
+	}
+
+	pj_grp_lock_dec_ref(wstransport->grp_lock);
 	return 0;
 }
 
@@ -164,6 +193,20 @@
 	newtransport->transport.send_msg = &ws_send_msg;
 	newtransport->transport.destroy = &ws_destroy;
 
+	if (PJ_SUCCESS != pj_grp_lock_create(pool, NULL, &newtransport->grp_lock)) {
+		ast_log(LOG_ERROR, "Failed to get pj_grp_lock.\n");
+		pjsip_endpt_release_pool(endpt, pool);
+		return -1;
+	}
+
+	/*
+	 * Group lock ensures the transport is not destroyed until both
+	 * pjsip transport manager and websocket listener are finished
+	 */
+	pj_grp_lock_add_ref(newtransport->grp_lock);
+	pj_grp_lock_add_ref(newtransport->grp_lock);
+	pj_grp_lock_add_handler(newtransport->grp_lock, pool, newtransport, &ws_on_destroy);
+
 	pjsip_transport_register(newtransport->transport.tpmgr, (pjsip_transport *)newtransport);
 
 	newtransport->rdata.tp_info.transport = &newtransport->transport;
@@ -171,10 +214,12 @@
 		PJSIP_POOL_RDATA_LEN, PJSIP_POOL_RDATA_INC);
 	if (!newtransport->rdata.tp_info.pool) {
 		ast_log(LOG_ERROR, "Failed to allocate WebSocket rdata.\n");
-		pjsip_endpt_release_pool(endpt, pool);
+		pj_grp_lock_destroy(newtransport->grp_lock);
 		return -1;
 	}
 
+	/* Add a reference for transport manager */
+	ast_websocket_ref(newtransport->ws_session);
 	create_data->transport = newtransport;
 	return 0;
 }

-- 
To view, visit https://gerrit.asterisk.org/598
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idc0b63eb6e459c1ddfb2430127d34b3c4d8d373b
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Ivan Poddubny <ivan.poddubny at gmail.com>