[Asterisk-code-review] srtp: Fix possible race condition, and add NULL checks (...asterisk[16])
Kevin Harwell
asteriskteam at digium.com
Wed Aug 7 18:10:56 CDT 2019
Kevin Harwell has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/11697
Change subject: srtp: Fix possible race condition, and add NULL checks
......................................................................
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
---
M main/rtp_engine.c
M res/res_pjsip_sdp_rtp.c
M res/res_srtp.c
3 files changed, 34 insertions(+), 5 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/97/11697/1
diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index 2153a8f..39ad1b3 100644
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -2742,6 +2742,7 @@
return -1;
}
+ ao2_lock(instance);
srtp = rtcp ? &instance->rtcp_srtp : &instance->srtp;
@@ -2754,6 +2755,8 @@
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 dfbaad6..8d9cece 100644
--- a/res/res_pjsip_sdp_rtp.c
+++ b/res/res_pjsip_sdp_rtp.c
@@ -1006,7 +1006,7 @@
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 06d5188..315a7ad 100644
--- a/res/res_srtp.c
+++ b/res/res_srtp.c
@@ -197,6 +197,7 @@
srtp->policies = ao2_t_container_alloc_hash(AO2_ALLOC_OPT_LOCK_MUTEX, 0, 5,
policy_hash_fn, NULL, policy_cmp_fn, "SRTP policy container");
if (!srtp->policies) {
+ ast_log(LOG_ERROR, "Unable to allocate memory for srtp policies\n");
ast_free(srtp);
return NULL;
}
@@ -373,6 +374,12 @@
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 +488,12 @@
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 +514,7 @@
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 +522,13 @@
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 +542,19 @@
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)
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11697
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I240e11cbb1e9ea8083d59d50db069891228fe5cc
Gerrit-Change-Number: 11697
Gerrit-PatchSet: 1
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190807/63cf7317/attachment.html>
More information about the asterisk-code-review
mailing list