srtp: Fix possible race condition, and add NULL checks

Somehow it's possible for the srtp session object to be NULL even though the
Asterisk srtp object itself is valid. When this happened it would cause a
crash down in the srtp code when attempting to protect or unprotect data.

After looking at the code there is at least one spot that makes this situation
possible. If Asterisk fails to unprotect the data, and after several retries
it still can't then the srtp->session gets freed, and set to NULL while still
leaving the Asterisk srtp object around. However, according to the original
issue reporter this does not appear to be their situation since they found
no errors logged stating the above happened (which Asterisk does for that
situation).

An issue was found however, where a possible race condition could occur between
the pjsip incoming negotiation, and the receiving of RTP packets. Both places
could attempt to create/setup srtp for the same rtp instance at the same time.
This potentially could be the cause of the problem as well.

Given the above this patch adds locking around srtp setup for a given rtp, or
rtcp instance. NULL checks for the session have also been added within the
protect and unprotect functions as a precaution. These checks should at least
stop Asterisk from crashing if it gets in this situation again.

This patch also fixes one other issue noticed during investigation. When doing
a replace the old object was freed before creating the replacement. If the new
replacement object failed to create then the rtp/rtcp instance would now point
to freed srtp data which could potentially cause a crash as well when the next
attempt to reference it was made. This is now fixed so the old srtp object is
kept upon replacement failure.

Lastly, more logging has been added to help diagnose future issues.

ASTERISK-28472

Change-Id: I240e11cbb1e9ea8083d59d50db069891228fe5cc

diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index c56ec5fa315b48f3c9eeaf3c445cf646e5283b19..d952352e6e6eb4d2922a0501219a5e3409bec002 100644 (file)
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -1859,6 +1859,7 @@ int ast_rtp_instance_add_srtp_policy(struct ast_rtp_instance *instance, struct a
                return -1;
        }
 
+       ao2_lock(instance);
 
        srtp = rtcp ? &instance->rtcp_srtp : &instance->srtp;
 
@@ -1871,6 +1872,8 @@ int ast_rtp_instance_add_srtp_policy(struct ast_rtp_instance *instance, struct a
                res = res_srtp->add_stream(*srtp, local_policy);
        }
 
+       ao2_unlock(instance);
+
        return res;
 }
 

diff --git a/res/res_pjsip_sdp_rtp.c b/res/res_pjsip_sdp_rtp.c
index cc53913786770a6b2029b68eacd97378f5d8511d..76d47a3f6d7c42b526c5d0705ae80d9cd99d5f87 100644 (file)
--- a/res/res_pjsip_sdp_rtp.c
+++ b/res/res_pjsip_sdp_rtp.c
@@ -933,7 +933,7 @@ static int setup_sdes_srtp(struct ast_sip_session_media *session_media,
                        return 0;
                }
 
-               ast_debug(1, "Ignoring crypto offer with unsupported parameters: %s\n", crypto_str);
+               ast_log(LOG_WARNING, "Ignoring crypto offer with unsupported parameters: %s\n", crypto_str);
        }
 
        /* no usable crypto attributes found */

diff --git a/res/res_srtp.c b/res/res_srtp.c
index 8e8e1850c9756c41a14ae1acd334e02449a6e67c..a2a845200ba17bbf910df4b142133e09d10fb00b 100644 (file)
--- a/res/res_srtp.c
+++ b/res/res_srtp.c
@@ -345,6 +345,12 @@ static int ast_srtp_unprotect(struct ast_srtp *srtp, void *buf, int *len, int rt
 
 tryagain:
 
+       if (!srtp->session) {
+               ast_log(LOG_ERROR, "SRTP unprotect %s - missing session\n", rtcp ? "rtcp" : "rtp");
+               errno = EINVAL;
+               return -1;
+       }
+
        for (i = 0; i < 2; i++) {
                res = rtcp ? srtp_unprotect_rtcp(srtp->session, buf, len) : srtp_unprotect(srtp->session, buf, len);
                if (res != err_status_no_ctx) {
@@ -452,6 +458,12 @@ static int ast_srtp_protect(struct ast_srtp *srtp, void **buf, int *len, int rtc
        int res;
        unsigned char *localbuf;
 
+       if (!srtp->session) {
+               ast_log(LOG_ERROR, "SRTP protect %s - missing session\n", rtcp ? "rtcp" : "rtp");
+               errno = EINVAL;
+               return -1;
+       }
+
        if ((*len + SRTP_MAX_TRAILER_LEN) > sizeof(srtp->buf)) {
                return -1;
        }
@@ -472,6 +484,7 @@ static int ast_srtp_protect(struct ast_srtp *srtp, void **buf, int *len, int rtc
 static int ast_srtp_create(struct ast_srtp **srtp, struct ast_rtp_instance *rtp, struct ast_srtp_policy *policy)
 {
        struct ast_srtp *temp;
+       int status;
 
        if (!(temp = res_srtp_new())) {
                return -1;
@@ -479,10 +492,13 @@ static int ast_srtp_create(struct ast_srtp **srtp, struct ast_rtp_instance *rtp,
        ast_module_ref(ast_module_info->self);
 
        /* Any failures after this point can use ast_srtp_destroy to destroy the instance */
-       if (srtp_create(&temp->session, &policy->sp) != err_status_ok) {
+       status = srtp_create(&temp->session, &policy->sp);
+       if (status != err_status_ok) {
                /* Session either wasn't created or was created and dealloced. */
                temp->session = NULL;
                ast_srtp_destroy(temp);
+               ast_log(LOG_ERROR, "Failed to create srtp session on rtp instance (%p) - %s\n",
+                               rtp, srtp_errstr(status));
                return -1;
        }
 
@@ -496,10 +512,19 @@ static int ast_srtp_create(struct ast_srtp **srtp, struct ast_rtp_instance *rtp,
 
 static int ast_srtp_replace(struct ast_srtp **srtp, struct ast_rtp_instance *rtp, struct ast_srtp_policy *policy)
 {
-       if ((*srtp) != NULL) {
-               ast_srtp_destroy(*srtp);
+       struct ast_srtp *old = *srtp;
+       int res = ast_srtp_create(srtp, rtp, policy);
+
+       if (!res && old) {
+               ast_srtp_destroy(old);
        }
-       return ast_srtp_create(srtp, rtp, policy);
+
+       if (res) {
+               ast_log(LOG_ERROR, "Failed to replace srtp (%p) on rtp instance (%p) "
+                               "- keeping old\n", *srtp, rtp);
+       }
+
+       return res;
 }
 
 static void ast_srtp_destroy(struct ast_srtp *srtp)