<p>Kevin Harwell has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13805">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">res_rtp_asterisk: bad audio (static) due to incomplete dtls/srtp setup<br><br>There was a race condition between client initiated DTLS setup, and handling<br>of server side ice completion that caused the underlying SSL object to get<br>cleared during DTLS initialization. If this happened Asterisk would be left<br>in a partial DTLS setup state. RTP packets were sent and received, but were<br>not being encrypted and decrypted. This resulted in no audio, or static.<br><br>Specifically, this occurred when '__rtp_recvfrom' was processing the handshake<br>sequence from the client to the server, and then 'ast_rtp_on_ice_complete'<br>gets called from another thread and clears the SSL object when calling the<br>'dtls_perform_setup' function. The timing had to be just right in the sense<br>that from the external SSL library perspective SSL initialization completed<br>(rtp recv), Asterisk clears/resets the SSL object (ice done), and then checks<br>to see if SSL is intialized (rtp recv). Since it was cleared, Asterisk thinks<br>it is not finished, thus not completing 'dtls_srtp_setup'.<br><br>This patch removes calls to 'dtls_perform_setup', which clears the SSL object,<br>in 'ast_rtp_on_ice_complete'. When ice completes, there is no reason to clear<br>the underlying SSL object. If an ice candidate changes a full protocol level<br>renegotiation occurs. Also, in the case of bundled ICE candidates are reused<br>when a stream is added. So no real reason to have to clear, and reset in this<br>instance.<br><br>Also, this patch adds a bit of extra logging to aid in diagnosis of any future<br>problems.<br><br>ASTERISK-28742 #close<br><br>Change-Id: I34c9e6bad5a39b087164646e2836e3e48fe6892f<br>---<br>M res/res_rtp_asterisk.c<br>1 file changed, 29 insertions(+), 2 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/05/13805/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c</span><br><span>index b4528f1..49d80dd 100644</span><br><span>--- a/res/res_rtp_asterisk.c</span><br><span>+++ b/res/res_rtp_asterisk.c</span><br><span>@@ -2480,6 +2480,9 @@</span><br><span> {</span><br><span> struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ ast_debug(3, "dtls_perform_handshake (%p) - ssl = %p, setup = %d\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ rtp, dtls->ssl, dtls->dtls_setup);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /* If we are not acting as a client connecting to the remote side then</span><br><span> * don't start the handshake as it will accomplish nothing and would conflict</span><br><span> * with the handshake we receive from the remote side.</span><br><span>@@ -2516,6 +2519,8 @@</span><br><span> SSL_set_connect_state(dtls->ssl);</span><br><span> }</span><br><span> dtls->connection = AST_RTP_DTLS_CONNECTION_NEW;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_debug(3, "dtls_perform_setup - connection reset'\n");</span><br><span> }</span><br><span> #endif</span><br><span> </span><br><span>@@ -2548,11 +2553,24 @@</span><br><span> </span><br><span> #if defined(HAVE_OPENSSL) && (OPENSSL_VERSION_NUMBER >= 0x10001000L) && !defined(OPENSSL_NO_SRTP)</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- dtls_perform_setup(&rtp->dtls);</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_debug(3, "ast_rtp_on_ice_complete (%p) - perform DTLS\n", 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%);">+ * Seemingly no reason to call dtls_perform_setup here. Currently we'll do a full</span><br><span style="color: hsl(120, 100%, 40%);">+ * protocol level renegotiation if things do change. And if bundled is being used</span><br><span style="color: hsl(120, 100%, 40%);">+ * then ICE is reused when a stream is added.</span><br><span style="color: hsl(120, 100%, 40%);">+ *</span><br><span style="color: hsl(120, 100%, 40%);">+ * Note, if for some reason in the future dtls_perform_setup does need to done here</span><br><span style="color: hsl(120, 100%, 40%);">+ * be aware that creates a race condition between the call here (on ice completion)</span><br><span style="color: hsl(120, 100%, 40%);">+ * and potential DTLS handshaking when receiving RTP. What happens is the ssl object</span><br><span style="color: hsl(120, 100%, 40%);">+ * can get cleared (SSL_clear) during that handshaking process (DTLS init). If that</span><br><span style="color: hsl(120, 100%, 40%);">+ * happens then Asterisk won't complete DTLS initialization. RTP packets are still</span><br><span style="color: hsl(120, 100%, 40%);">+ * sent/received but won't be encrypted/decrypted.</span><br><span style="color: hsl(120, 100%, 40%);">+ * condition between DTLS handshaking</span><br><span style="color: hsl(120, 100%, 40%);">+ */</span><br><span> dtls_perform_handshake(instance, &rtp->dtls, 0);</span><br><span> </span><br><span> if (rtp->rtcp && rtp->rtcp->type == AST_RTP_INSTANCE_RTCP_STANDARD) {</span><br><span style="color: hsl(0, 100%, 40%);">- dtls_perform_setup(&rtp->rtcp->dtls);</span><br><span> dtls_perform_handshake(instance, &rtp->rtcp->dtls, 1);</span><br><span> }</span><br><span> #endif</span><br><span>@@ -2868,6 +2886,8 @@</span><br><span> struct dtls_details *dtls = !rtcp ? &rtp->dtls : &rtp->rtcp->dtls;</span><br><span> int index;</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ ast_debug(3, "Setup DTLS SRTP (%p)'\n", rtp);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /* If a fingerprint is present in the SDP make sure that the peer certificate matches it */</span><br><span> if (rtp->dtls_verify & AST_RTP_DTLS_VERIFY_FINGERPRINT) {</span><br><span> X509 *certificate;</span><br><span>@@ -2906,6 +2926,7 @@</span><br><span> }</span><br><span> </span><br><span> if (dtls_srtp_add_local_ssrc(rtp, instance, rtcp, ast_rtp_instance_get_ssrc(instance), 1)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_log(LOG_ERROR, "Failed to add local source '%p'\n", rtp);</span><br><span> return -1;</span><br><span> }</span><br><span> </span><br><span>@@ -3014,6 +3035,8 @@</span><br><span> return -1;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ ast_debug(3, "__rtp_recvfrom (%p) - Got SSL packet '%d'\n", rtp, *in);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /*</span><br><span> * A race condition is prevented between dtls_perform_handshake()</span><br><span> * and this function because both functions have to get the</span><br><span>@@ -3053,6 +3076,8 @@</span><br><span> }</span><br><span> /* Notify that dtls has been established */</span><br><span> res = RTP_DTLS_ESTABLISHED;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ ast_debug(3, "__rtp_recvfrom (%p) - DTLS established'\n", rtp);</span><br><span> } else {</span><br><span> /* Since we've sent additional traffic start the timeout timer for retransmission */</span><br><span> dtls_srtp_start_timeout_timer(instance, rtp, rtcp);</span><br><span>@@ -8519,6 +8544,8 @@</span><br><span> }</span><br><span> #endif</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ ast_debug(3, "ast_rtp_activate (%p) - setup and perform DTLS'\n", rtp);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> dtls_perform_setup(&rtp->dtls);</span><br><span> dtls_perform_handshake(instance, &rtp->dtls, 0);</span><br><span> </span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/13805">change 13805</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/+/13805"/><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: I34c9e6bad5a39b087164646e2836e3e48fe6892f </div>
<div style="display:none"> Gerrit-Change-Number: 13805 </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>