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

diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index b4528f121cd1ee49bf4ac21e8fdf1b8a9de83d9a..c52ce2c1cf7a286698e9d983facb862f2d7abf58 100644 (file)
--- 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);