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 2153a8f048d5a5c3a49810685f5467b904f6a7aa..39ad1b3bb3d1626d0eed8ac18252a757c377cd9c 100644 (file)
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -2742,6 +2742,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;
 
@@ -2754,6 +2755,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 dfbaad6450354c70cc5a7febb4676bc9728726dc..8d9ceceeb5b2d35e54712ae81815c2bfa8d331f1 100644 (file)
--- a/res/res_pjsip_sdp_rtp.c
+++ b/res/res_pjsip_sdp_rtp.c
@@ -1006,7 +1006,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 06d51887ad64ea6cb612d8df00cb1a05f3998fa3..3e4b51fafb42b396b7cb216652edb832446e9d05 100644 (file)
--- a/res/res_srtp.c
+++ b/res/res_srtp.c
@@ -373,6 +373,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) {
@@ -481,6 +487,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;
        }
@@ -501,6 +513,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;
@@ -508,10 +521,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;
        }
 
@@ -525,10 +541,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)