From b805e1237d7de0ee5ad165510e274288f6a8dadc Mon Sep 17 00:00:00 2001
From: Kevin Harwell <kharwell@digium.com>
Date: Wed, 7 Aug 2019 17:54:34 -0500
Subject: [PATCH] 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
---
main/rtp_engine.c | 3 +++
res/res_pjsip_sdp_rtp.c | 2 +-
res/res_srtp.c | 33 +++++++++++++++++++++++++++++----
3 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index 2153a8f048..39ad1b3bb3 100644
--- 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 dfbaad6450..8d9ceceeb5 100644
--- 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 06d51887ad..3e4b51fafb 100644
--- 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)
--
GitLab