<p>Kevin Harwell has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/11697">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">srtp: Fix possible race condition, and add NULL checks<br><br>Somehow it's possible for the srtp session object to be NULL even though the<br>Asterisk srtp object itself is valid. When this happened it would cause a<br>crash down in the srtp code when attempting to protect or unprotect data.<br><br>After looking at the code there is at least one spot that makes this situation<br>possible. If Asterisk fails to unprotect the data, and after several retries<br>it still can't then the srtp->session gets freed, and set to NULL while still<br>leaving the Asterisk srtp object around. However, according to the original<br>issue reporter this does not appear to be their situation since they found<br>no errors logged stating the above happened (which Asterisk does for that<br>situation).<br><br>An issue was found however, where a possible race condition could occur between<br>the pjsip incoming negotiation, and the receiving of RTP packets. Both places<br>could attempt to create/setup srtp for the same rtp instance at the same time.<br>This potentially could be the cause of the problem as well.<br><br>Given the above this patch adds locking around srtp setup for a given rtp, or<br>rtcp instance. NULL checks for the session have also been added within the<br>protect and unprotect functions as a precaution. These checks should at least<br>stop Asterisk from crashing if it gets in this situation again.<br><br>This patch also fixes one other issue noticed during investigation. When doing<br>a replace the old object was freed before creating the replacement. If the new<br>replacement object failed to create then the rtp/rtcp instance would now point<br>to freed srtp data which could potentially cause a crash as well when the next<br>attempt to reference it was made. This is now fixed so the old srtp object is<br>kept upon replacement failure.<br><br>Lastly, more logging has been added to help diagnose future issues.<br><br>ASTERISK-28472<br><br>Change-Id: I240e11cbb1e9ea8083d59d50db069891228fe5cc<br>---<br>M main/rtp_engine.c<br>M res/res_pjsip_sdp_rtp.c<br>M res/res_srtp.c<br>3 files changed, 34 insertions(+), 5 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/97/11697/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/main/rtp_engine.c b/main/rtp_engine.c</span><br><span>index 2153a8f..39ad1b3 100644</span><br><span>--- a/main/rtp_engine.c</span><br><span>+++ b/main/rtp_engine.c</span><br><span>@@ -2742,6 +2742,7 @@</span><br><span> return -1;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_lock(instance);</span><br><span> </span><br><span> srtp = rtcp ? &instance->rtcp_srtp : &instance->srtp;</span><br><span> </span><br><span>@@ -2754,6 +2755,8 @@</span><br><span> res = res_srtp->add_stream(*srtp, local_policy);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ ao2_unlock(instance);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> return res;</span><br><span> }</span><br><span> </span><br><span>diff --git a/res/res_pjsip_sdp_rtp.c b/res/res_pjsip_sdp_rtp.c</span><br><span>index dfbaad6..8d9cece 100644</span><br><span>--- a/res/res_pjsip_sdp_rtp.c</span><br><span>+++ b/res/res_pjsip_sdp_rtp.c</span><br><span>@@ -1006,7 +1006,7 @@</span><br><span> return 0;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- ast_debug(1, "Ignoring crypto offer with unsupported parameters: %s\n", crypto_str);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_log(LOG_WARNING, "Ignoring crypto offer with unsupported parameters: %s\n", crypto_str);</span><br><span> }</span><br><span> </span><br><span> /* no usable crypto attributes found */</span><br><span>diff --git a/res/res_srtp.c b/res/res_srtp.c</span><br><span>index 06d5188..315a7ad 100644</span><br><span>--- a/res/res_srtp.c</span><br><span>+++ b/res/res_srtp.c</span><br><span>@@ -197,6 +197,7 @@</span><br><span> srtp->policies = ao2_t_container_alloc_hash(AO2_ALLOC_OPT_LOCK_MUTEX, 0, 5,</span><br><span> policy_hash_fn, NULL, policy_cmp_fn, "SRTP policy container");</span><br><span> if (!srtp->policies) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_log(LOG_ERROR, "Unable to allocate memory for srtp policies\n");</span><br><span> ast_free(srtp);</span><br><span> return NULL;</span><br><span> }</span><br><span>@@ -373,6 +374,12 @@</span><br><span> </span><br><span> tryagain:</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ if (!srtp->session) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_log(LOG_ERROR, "SRTP unprotect %s - missing session\n", rtcp ? "rtcp" : "rtp");</span><br><span style="color: hsl(120, 100%, 40%);">+ errno = EINVAL;</span><br><span style="color: hsl(120, 100%, 40%);">+ return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> for (i = 0; i < 2; i++) {</span><br><span> res = rtcp ? srtp_unprotect_rtcp(srtp->session, buf, len) : srtp_unprotect(srtp->session, buf, len);</span><br><span> if (res != err_status_no_ctx) {</span><br><span>@@ -481,6 +488,12 @@</span><br><span> int res;</span><br><span> unsigned char *localbuf;</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ if (!srtp->session) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_log(LOG_ERROR, "SRTP protect %s - missing session\n", rtcp ? "rtcp" : "rtp");</span><br><span style="color: hsl(120, 100%, 40%);">+ errno = EINVAL;</span><br><span style="color: hsl(120, 100%, 40%);">+ return -1;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> if ((*len + SRTP_MAX_TRAILER_LEN) > sizeof(srtp->buf)) {</span><br><span> return -1;</span><br><span> }</span><br><span>@@ -501,6 +514,7 @@</span><br><span> static int ast_srtp_create(struct ast_srtp **srtp, struct ast_rtp_instance *rtp, struct ast_srtp_policy *policy)</span><br><span> {</span><br><span> struct ast_srtp *temp;</span><br><span style="color: hsl(120, 100%, 40%);">+ int status;</span><br><span> </span><br><span> if (!(temp = res_srtp_new())) {</span><br><span> return -1;</span><br><span>@@ -508,10 +522,13 @@</span><br><span> ast_module_ref(ast_module_info->self);</span><br><span> </span><br><span> /* Any failures after this point can use ast_srtp_destroy to destroy the instance */</span><br><span style="color: hsl(0, 100%, 40%);">- if (srtp_create(&temp->session, &policy->sp) != err_status_ok) {</span><br><span style="color: hsl(120, 100%, 40%);">+ status = srtp_create(&temp->session, &policy->sp);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (status != err_status_ok) {</span><br><span> /* Session either wasn't created or was created and dealloced. */</span><br><span> temp->session = NULL;</span><br><span> ast_srtp_destroy(temp);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_log(LOG_ERROR, "Failed to create srtp session on rtp instance (%p) - %s\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ rtp, srtp_errstr(status));</span><br><span> return -1;</span><br><span> }</span><br><span> </span><br><span>@@ -525,10 +542,19 @@</span><br><span> </span><br><span> static int ast_srtp_replace(struct ast_srtp **srtp, struct ast_rtp_instance *rtp, struct ast_srtp_policy *policy)</span><br><span> {</span><br><span style="color: hsl(0, 100%, 40%);">- if ((*srtp) != NULL) {</span><br><span style="color: hsl(0, 100%, 40%);">- ast_srtp_destroy(*srtp);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct ast_srtp *old = *srtp;</span><br><span style="color: hsl(120, 100%, 40%);">+ int res = ast_srtp_create(srtp, rtp, policy);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!res && old) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_srtp_destroy(old);</span><br><span> }</span><br><span style="color: hsl(0, 100%, 40%);">- return ast_srtp_create(srtp, rtp, policy);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (res) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_log(LOG_ERROR, "Failed to replace srtp (%p) on rtp instance (%p) "</span><br><span style="color: hsl(120, 100%, 40%);">+ "- keeping old\n", *srtp, rtp);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ return res;</span><br><span> }</span><br><span> </span><br><span> static void ast_srtp_destroy(struct ast_srtp *srtp)</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/11697">change 11697</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/11697"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: I240e11cbb1e9ea8083d59d50db069891228fe5cc </div>
<div style="display:none"> Gerrit-Change-Number: 11697 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>