[Asterisk-code-review] res_rtp_asterisk: bad audio (static) due to incomplete dtls/srtp setup (asterisk[certified/16.8])
George Joseph
asteriskteam at digium.com
Mon Feb 17 11:27:39 CST 2020
George Joseph has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/13795 )
Change subject: res_rtp_asterisk: bad audio (static) due to incomplete dtls/srtp setup
......................................................................
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
---
M res/res_rtp_asterisk.c
1 file changed, 28 insertions(+), 2 deletions(-)
Approvals:
Joshua Colp: Looks good to me, but someone else must approve
Benjamin Keith Ford: Looks good to me, but someone else must approve
George Joseph: Looks good to me, approved; Approved for Submit
diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index b4528f1..c52ce2c 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -2480,6 +2480,9 @@
{
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 @@
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 @@
#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 @@
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 @@
}
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 @@
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 @@
}
/* 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 @@
}
#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);
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13795
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: certified/16.8
Gerrit-Change-Id: I34c9e6bad5a39b087164646e2836e3e48fe6892f
Gerrit-Change-Number: 13795
Gerrit-PatchSet: 2
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200217/217536cb/attachment.html>
More information about the asterisk-code-review
mailing list