diff options
author | Richard Mudgett <rmudgett@digium.com> | 2017-03-15 13:24:33 -0500 |
---|---|---|
committer | Richard Mudgett <rmudgett@digium.com> | 2017-03-15 17:43:54 -0500 |
commit | adad6020beb2aae1f5535bad4c0ff6ea73026848 (patch) | |
tree | 91d6e3ee6dfb15d08a47f7be07bfbc2855bdeb67 /apps | |
parent | 481ed5c8f133aa40c2456f575ad154d2ecb5bc82 (diff) |
autochan/mixmonitor/chanspy: Fix unsafe channel locking and references.
Dereferencing struct ast_autochan.chan without first calling
ast_autochan_channel_lock() is unsafe because the pointer could change at
any time due to a masquerade. Unfortunately, ast_autochan_channel_lock()
itself uses struct ast_autochan.chan unsafely and can result in a deadlock
if the original channel happens to get destroyed after a masquerade in
addition to the pointer getting changed.
The problem is more likely to happen with v11 and earlier because
masquerades are used to optimize out local channels on those versions.
However, it could still happen on newer versions if the channel is
executing a dialplan application when the channel is transferred or
redirected. In this situation a masquerade still must be used.
* Added a lock to struct ast_autochan to safely be able to use
ast_autochan.chan while trying to get the channel lock in
ast_autochan_channel_lock(). The locking order is the channel lock then
the autochan lock. Locking in the other direction requires deadlock
avoidance.
* Fix unsafe ast_autochan.chan usages in app_mixmonitor.c.
* Fix unsafe ast_autochan.chan usages in app_chanspy.c.
* app_chanspy.c: Removed unused autochan parameter from next_channel().
ASTERISK-26867
Change-Id: Id29dd22bc0f369b44e23ca423d2f3657187cc592
Diffstat (limited to 'apps')
-rw-r--r-- | apps/app_chanspy.c | 62 | ||||
-rw-r--r-- | apps/app_mixmonitor.c | 21 |
2 files changed, 60 insertions, 23 deletions
diff --git a/apps/app_chanspy.c b/apps/app_chanspy.c index 19675fb28..608eb6be1 100644 --- a/apps/app_chanspy.c +++ b/apps/app_chanspy.c @@ -498,10 +498,15 @@ static struct ast_generator spygen = { static int start_spying(struct ast_autochan *autochan, const char *spychan_name, struct ast_audiohook *audiohook) { + int res; + + ast_autochan_channel_lock(autochan); ast_log(LOG_NOTICE, "Attaching %s to %s\n", spychan_name, ast_channel_name(autochan->chan)); ast_set_flag(audiohook, AST_AUDIOHOOK_TRIGGER_SYNC | AST_AUDIOHOOK_SMALL_QUEUE); - return ast_audiohook_attach(autochan->chan, audiohook); + res = ast_audiohook_attach(autochan->chan, audiohook); + ast_autochan_channel_unlock(autochan); + return res; } static void change_spy_mode(const char digit, struct ast_flags *flags) @@ -585,8 +590,14 @@ static int attach_barge(struct ast_autochan *spyee_autochan, { int retval = 0; struct ast_autochan *internal_bridge_autochan; - RAII_VAR(struct ast_channel *, bridged, ast_channel_bridge_peer(spyee_autochan->chan), ast_channel_cleanup); + struct ast_channel *spyee_chan; + RAII_VAR(struct ast_channel *, bridged, NULL, ast_channel_cleanup); + ast_autochan_channel_lock(spyee_autochan); + spyee_chan = ast_channel_ref(spyee_autochan->chan); + ast_autochan_channel_unlock(spyee_autochan); + bridged = ast_channel_bridge_peer(spyee_chan); + ast_channel_unref(spyee_chan); if (!bridged) { return -1; } @@ -598,12 +609,10 @@ static int attach_barge(struct ast_autochan *spyee_autochan, return -1; } - ast_autochan_channel_lock(internal_bridge_autochan); if (start_spying(internal_bridge_autochan, spyer_name, bridge_whisper_audiohook)) { ast_log(LOG_WARNING, "Unable to attach barge audiohook on spyee '%s'. Barge mode disabled.\n", name); retval = -1; } - ast_autochan_channel_unlock(internal_bridge_autochan); *spyee_bridge_autochan = internal_bridge_autochan; @@ -623,21 +632,25 @@ static int channel_spy(struct ast_channel *chan, struct ast_autochan *spyee_auto struct ast_autochan *spyee_bridge_autochan = NULL; const char *spyer_name; - if (ast_check_hangup(chan) || ast_check_hangup(spyee_autochan->chan) || - ast_test_flag(ast_channel_flags(spyee_autochan->chan), AST_FLAG_ZOMBIE)) { + ast_channel_lock(chan); + if (ast_check_hangup(chan)) { + ast_channel_unlock(chan); return 0; } - - ast_channel_lock(chan); spyer_name = ast_strdupa(ast_channel_name(chan)); ast_channel_unlock(chan); ast_autochan_channel_lock(spyee_autochan); + if (ast_check_hangup(spyee_autochan->chan) + || ast_test_flag(ast_channel_flags(spyee_autochan->chan), AST_FLAG_ZOMBIE)) { + ast_autochan_channel_unlock(spyee_autochan); + return 0; + } name = ast_strdupa(ast_channel_name(spyee_autochan->chan)); - ast_autochan_channel_unlock(spyee_autochan); ast_verb(2, "Spying on channel %s\n", name); publish_chanspy_message(chan, spyee_autochan->chan, 1); + ast_autochan_channel_unlock(spyee_autochan); memset(&csth, 0, sizeof(csth)); ast_copy_flags(&csth.flags, flags, AST_FLAGS_ALL); @@ -829,7 +842,7 @@ static int channel_spy(struct ast_channel *chan, struct ast_autochan *spyee_auto } static struct ast_autochan *next_channel(struct ast_channel_iterator *iter, - struct ast_autochan *autochan, struct ast_channel *chan) + struct ast_channel *chan) { struct ast_channel *next; struct ast_autochan *autochan_store; @@ -966,11 +979,12 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, waitms = 100; num_spyed_upon = 0; - for (autochan = next_channel(iter, autochan, chan); - autochan; - prev = autochan->chan, ast_autochan_destroy(autochan), - autochan = next_autochan ? next_autochan : - next_channel(iter, autochan, chan), next_autochan = NULL) { + for (autochan = next_channel(iter, chan); + autochan; + prev = autochan->chan, + ast_autochan_destroy(autochan), + autochan = next_autochan ?: next_channel(iter, chan), + next_autochan = NULL) { int igrp = !mygroup; int ienf = !myenforced; @@ -984,13 +998,19 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, break; } - if (ast_test_flag(flags, OPTION_BRIDGED) && !ast_channel_is_bridged(autochan->chan)) { + ast_autochan_channel_lock(autochan); + if (ast_test_flag(flags, OPTION_BRIDGED) + && !ast_channel_is_bridged(autochan->chan)) { + ast_autochan_channel_unlock(autochan); continue; } - if (ast_check_hangup(autochan->chan) || ast_test_flag(ast_channel_flags(autochan->chan), AST_FLAG_SPYING)) { + if (ast_check_hangup(autochan->chan) + || ast_test_flag(ast_channel_flags(autochan->chan), AST_FLAG_SPYING)) { + ast_autochan_channel_unlock(autochan); continue; } + ast_autochan_channel_unlock(autochan); if (mygroup) { int num_groups = 0; @@ -1008,11 +1028,13 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, /* Before dahdi scan was part of chanspy, it would use the "GROUP" variable * rather than "SPYGROUP", this check is done to preserve expected behavior */ + ast_autochan_channel_lock(autochan); if (ast_test_flag(flags, OPTION_DAHDI_SCAN)) { group = pbx_builtin_getvar_helper(autochan->chan, "GROUP"); } else { group = pbx_builtin_getvar_helper(autochan->chan, "SPYGROUP"); } + ast_autochan_channel_unlock(autochan); if (!ast_strlen_zero(group)) { ast_copy_string(dup_group, group, sizeof(dup_group)); @@ -1040,7 +1062,9 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, snprintf(buffer, sizeof(buffer) - 1, ":%s:", myenforced); + ast_autochan_channel_lock(autochan); ast_copy_string(ext + 1, ast_channel_name(autochan->chan), sizeof(ext) - 1); + ast_autochan_channel_unlock(autochan); if ((end = strchr(ext, '-'))) { *end++ = ':'; *end = '\0'; @@ -1062,7 +1086,9 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, char *ptr, *s; strcpy(peer_name, "spy-"); + ast_autochan_channel_lock(autochan); strncat(peer_name, ast_channel_name(autochan->chan), AST_NAME_STRLEN - 4 - 1); + ast_autochan_channel_unlock(autochan); if ((ptr = strchr(peer_name, '/'))) { *ptr++ = '\0'; for (s = peer_name; s < ptr; s++) { @@ -1127,12 +1153,14 @@ static int common_exec(struct ast_channel *chan, struct ast_flags *flags, next = ast_channel_unref(next); } else { /* stay on this channel, if it is still valid */ + ast_autochan_channel_lock(autochan); if (!ast_check_hangup(autochan->chan)) { next_autochan = ast_autochan_setup(autochan->chan); } else { /* the channel is gone */ next_autochan = NULL; } + ast_autochan_channel_unlock(autochan); } } else if (res == 0 && ast_test_flag(flags, OPTION_EXITONHANGUP)) { ast_autochan_destroy(autochan); diff --git a/apps/app_mixmonitor.c b/apps/app_mixmonitor.c index 3515f4b8c..979bf2d70 100644 --- a/apps/app_mixmonitor.c +++ b/apps/app_mixmonitor.c @@ -622,6 +622,16 @@ static void mixmonitor_save_prep(struct mixmonitor *mixmonitor, char *filename, } } +static int mixmonitor_autochan_is_bridged(struct ast_autochan *autochan) +{ + int is_bridged; + + ast_autochan_channel_lock(autochan); + is_bridged = ast_channel_is_bridged(autochan->chan); + ast_autochan_channel_unlock(autochan); + return is_bridged; +} + static void *mixmonitor_thread(void *obj) { struct mixmonitor *mixmonitor = obj; @@ -679,8 +689,7 @@ static void *mixmonitor_thread(void *obj) ast_audiohook_unlock(&mixmonitor->audiohook); if (!ast_test_flag(mixmonitor, MUXFLAG_BRIDGED) - || (mixmonitor->autochan->chan - && ast_channel_is_bridged(mixmonitor->autochan->chan))) { + || mixmonitor_autochan_is_bridged(mixmonitor->autochan)) { ast_mutex_lock(&mixmonitor->mixmonitor_ds->lock); /* Write out the frame(s) */ @@ -729,11 +738,11 @@ static void *mixmonitor_thread(void *obj) ast_audiohook_unlock(&mixmonitor->audiohook); - ast_autochan_channel_lock(mixmonitor->autochan); if (ast_test_flag(mixmonitor, MUXFLAG_BEEP_STOP)) { + ast_autochan_channel_lock(mixmonitor->autochan); ast_stream_and_wait(mixmonitor->autochan->chan, "beep", ""); + ast_autochan_channel_unlock(mixmonitor->autochan); } - ast_autochan_channel_unlock(mixmonitor->autochan); ast_autochan_destroy(mixmonitor->autochan); @@ -805,11 +814,11 @@ static int setup_mixmonitor_ds(struct mixmonitor *mixmonitor, struct ast_channel return -1; } - ast_autochan_channel_lock(mixmonitor->autochan); if (ast_test_flag(mixmonitor, MUXFLAG_BEEP_START)) { + ast_autochan_channel_lock(mixmonitor->autochan); ast_stream_and_wait(mixmonitor->autochan->chan, "beep", ""); + ast_autochan_channel_unlock(mixmonitor->autochan); } - ast_autochan_channel_unlock(mixmonitor->autochan); mixmonitor_ds->samp_rate = 8000; mixmonitor_ds->audiohook = &mixmonitor->audiohook; |