From 3865b3fd6a2e8c98158aee0713d81913bf84ca06 Mon Sep 17 00:00:00 2001 From: Kevin Harwell Date: Thu, 13 Feb 2020 15:08:10 -0600 Subject: [PATCH] res_rtp_asterisk: bad audio (static) due to incomplete dtls/srtp setup There was a race condition between client initiated DTLS setup, and handling of server side ice completion that caused the underlying SSL object to get cleared during DTLS initialization. If this happened Asterisk would be left in a partial DTLS setup state. RTP packets were sent and received, but were not being encrypted and decrypted. This resulted in no audio, or static. Specifically, this occurred when '__rtp_recvfrom' was processing the handshake sequence from the client to the server, and then 'ast_rtp_on_ice_complete' gets called from another thread and clears the SSL object when calling the 'dtls_perform_setup' function. The timing had to be just right in the sense that from the external SSL library perspective SSL initialization completed (rtp recv), Asterisk clears/resets the SSL object (ice done), and then checks to see if SSL is intialized (rtp recv). Since it was cleared, Asterisk thinks it is not finished, thus not completing 'dtls_srtp_setup'. This patch removes calls to 'dtls_perform_setup', which clears the SSL object, in 'ast_rtp_on_ice_complete'. When ice completes, there is no reason to clear the underlying SSL object. If an ice candidate changes a full protocol level renegotiation occurs. Also, in the case of bundled ICE candidates are reused when a stream is added. So no real reason to have to clear, and reset in this instance. Also, this patch adds a bit of extra logging to aid in diagnosis of any future problems. ASTERISK-28742 #close Change-Id: I34c9e6bad5a39b087164646e2836e3e48fe6892f --- res/res_rtp_asterisk.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c index b4528f121c..c52ce2c1cf 100644 --- a/res/res_rtp_asterisk.c +++ b/res/res_rtp_asterisk.c @@ -2480,6 +2480,9 @@ static void dtls_perform_handshake(struct ast_rtp_instance *instance, struct dtl { struct ast_rtp *rtp = ast_rtp_instance_get_data(instance); + ast_debug(3, "dtls_perform_handshake (%p) - ssl = %p, setup = %d\n", + rtp, dtls->ssl, dtls->dtls_setup); + /* If we are not acting as a client connecting to the remote side then * don't start the handshake as it will accomplish nothing and would conflict * with the handshake we receive from the remote side. @@ -2516,6 +2519,8 @@ static void dtls_perform_setup(struct dtls_details *dtls) SSL_set_connect_state(dtls->ssl); } dtls->connection = AST_RTP_DTLS_CONNECTION_NEW; + + ast_debug(3, "dtls_perform_setup - connection reset'\n"); } #endif @@ -2548,11 +2553,23 @@ static void ast_rtp_on_ice_complete(pj_ice_sess *ice, pj_status_t status) #if defined(HAVE_OPENSSL) && (OPENSSL_VERSION_NUMBER >= 0x10001000L) && !defined(OPENSSL_NO_SRTP) - dtls_perform_setup(&rtp->dtls); + ast_debug(3, "ast_rtp_on_ice_complete (%p) - perform DTLS\n", rtp); + + /* + * Seemingly no reason to call dtls_perform_setup here. Currently we'll do a full + * protocol level renegotiation if things do change. And if bundled is being used + * then ICE is reused when a stream is added. + * + * Note, if for some reason in the future dtls_perform_setup does need to done here + * be aware that creates a race condition between the call here (on ice completion) + * and potential DTLS handshaking when receiving RTP. What happens is the ssl object + * can get cleared (SSL_clear) during that handshaking process (DTLS init). If that + * happens then Asterisk won't complete DTLS initialization. RTP packets are still + * sent/received but won't be encrypted/decrypted. + */ dtls_perform_handshake(instance, &rtp->dtls, 0); if (rtp->rtcp && rtp->rtcp->type == AST_RTP_INSTANCE_RTCP_STANDARD) { - dtls_perform_setup(&rtp->rtcp->dtls); dtls_perform_handshake(instance, &rtp->rtcp->dtls, 1); } #endif @@ -2868,6 +2885,8 @@ static int dtls_srtp_setup(struct ast_rtp *rtp, struct ast_rtp_instance *instanc struct dtls_details *dtls = !rtcp ? &rtp->dtls : &rtp->rtcp->dtls; int index; + ast_debug(3, "Setup DTLS SRTP (%p)'\n", rtp); + /* If a fingerprint is present in the SDP make sure that the peer certificate matches it */ if (rtp->dtls_verify & AST_RTP_DTLS_VERIFY_FINGERPRINT) { X509 *certificate; @@ -2906,6 +2925,7 @@ static int dtls_srtp_setup(struct ast_rtp *rtp, struct ast_rtp_instance *instanc } if (dtls_srtp_add_local_ssrc(rtp, instance, rtcp, ast_rtp_instance_get_ssrc(instance), 1)) { + ast_log(LOG_ERROR, "Failed to add local source '%p'\n", rtp); return -1; } @@ -3014,6 +3034,8 @@ static int __rtp_recvfrom(struct ast_rtp_instance *instance, void *buf, size_t s return -1; } + ast_debug(3, "__rtp_recvfrom (%p) - Got SSL packet '%d'\n", rtp, *in); + /* * A race condition is prevented between dtls_perform_handshake() * and this function because both functions have to get the @@ -3053,6 +3075,8 @@ static int __rtp_recvfrom(struct ast_rtp_instance *instance, void *buf, size_t s } /* Notify that dtls has been established */ res = RTP_DTLS_ESTABLISHED; + + ast_debug(3, "__rtp_recvfrom (%p) - DTLS established'\n", rtp); } else { /* Since we've sent additional traffic start the timeout timer for retransmission */ dtls_srtp_start_timeout_timer(instance, rtp, rtcp); @@ -8519,6 +8543,8 @@ static int ast_rtp_activate(struct ast_rtp_instance *instance) } #endif + ast_debug(3, "ast_rtp_activate (%p) - setup and perform DTLS'\n", rtp); + dtls_perform_setup(&rtp->dtls); dtls_perform_handshake(instance, &rtp->dtls, 0);