]> git.ipfire.org Git - thirdparty/asterisk.git/commit
srtp: Fix possible race condition, and add NULL checks
authorKevin Harwell <kharwell@digium.com>
Wed, 7 Aug 2019 22:54:34 +0000 (17:54 -0500)
committerKevin Harwell <kharwell@digium.com>
Thu, 8 Aug 2019 16:30:47 +0000 (11:30 -0500)
commitfeaadbd25055a2085b3a8f4be6cd54adfb7e3b5d
treec491887f5f3e2bbd6b83420d443d77db1faaeb72
parent6d174e4ef254fc6d7b347a30b11b1be0afbe8f8a
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
res/res_pjsip_sdp_rtp.c
res/res_srtp.c