From 9c20596400cef759ecc760e1b028fd1748e9be5d Mon Sep 17 00:00:00 2001 From: Corey Farrell Date: Sat, 1 Jul 2017 20:24:27 -0400 Subject: AST-2017-006: Fix app_minivm application MinivmNotify command injection An admin can configure app_minivm with an externnotify program to be run when a voicemail is received. The app_minivm application MinivmNotify uses ast_safe_system() for this purpose which is vulnerable to command injection since the Caller-ID name and number values given to externnotify can come from an external untrusted source. * Add ast_safe_execvp() function. This gives modules the ability to run external commands with greater safety compared to ast_safe_system(). Specifically when some parameters are filled by untrusted sources the new function does not allow malicious input to break argument encoding. This may be of particular concern where CALLERID(name) or CALLERID(num) may be used as a parameter to a script run by ast_safe_system() which could potentially allow arbitrary command execution. * Changed app_minivm.c:run_externnotify() to use the new ast_safe_execvp() instead of ast_safe_system() to avoid command injection. * Document code injection potential from untrusted data sources for other shell commands that are under user control. ASTERISK-27103 Change-Id: I7552472247a84cde24e1358aaf64af160107aef1 --- apps/app_minivm.c | 36 +++++++++++++++++++++++++----------- apps/app_mixmonitor.c | 15 +++++++++++++++ apps/app_system.c | 10 ++++++++++ 3 files changed, 50 insertions(+), 11 deletions(-) (limited to 'apps') diff --git a/apps/app_minivm.c b/apps/app_minivm.c index 41fd5e77d..647746048 100644 --- a/apps/app_minivm.c +++ b/apps/app_minivm.c @@ -1776,21 +1776,35 @@ static int play_record_review(struct ast_channel *chan, char *playfile, char *re /*! \brief Run external notification for voicemail message */ static void run_externnotify(struct ast_channel *chan, struct minivm_account *vmu) { - char arguments[BUFSIZ]; + char fquser[AST_MAX_CONTEXT * 2]; + char *argv[5] = { NULL }; + struct ast_party_caller *caller; + char *cid; + int idx; - if (ast_strlen_zero(vmu->externnotify) && ast_strlen_zero(global_externnotify)) + if (ast_strlen_zero(vmu->externnotify) && ast_strlen_zero(global_externnotify)) { return; + } + + snprintf(fquser, sizeof(fquser), "%s@%s", vmu->username, vmu->domain); - snprintf(arguments, sizeof(arguments), "%s %s@%s %s %s&", - ast_strlen_zero(vmu->externnotify) ? global_externnotify : vmu->externnotify, - vmu->username, vmu->domain, - (ast_channel_caller(chan)->id.name.valid && ast_channel_caller(chan)->id.name.str) - ? ast_channel_caller(chan)->id.name.str : "", - (ast_channel_caller(chan)->id.number.valid && ast_channel_caller(chan)->id.number.str) - ? ast_channel_caller(chan)->id.number.str : ""); + caller = ast_channel_caller(chan); + idx = 0; + argv[idx++] = ast_strlen_zero(vmu->externnotify) ? global_externnotify : vmu->externnotify; + argv[idx++] = fquser; + cid = S_COR(caller->id.name.valid, caller->id.name.str, NULL); + if (cid) { + argv[idx++] = cid; + } + cid = S_COR(caller->id.number.valid, caller->id.number.str, NULL); + if (cid) { + argv[idx++] = cid; + } + argv[idx] = NULL; - ast_debug(1, "Executing: %s\n", arguments); - ast_safe_system(arguments); + ast_debug(1, "Executing: %s %s %s %s\n", + argv[0], argv[1], argv[2] ?: "", argv[3] ?: ""); + ast_safe_execvp(1, argv[0], argv); } /*!\internal diff --git a/apps/app_mixmonitor.c b/apps/app_mixmonitor.c index 8de4995af..7fef15ebf 100644 --- a/apps/app_mixmonitor.c +++ b/apps/app_mixmonitor.c @@ -138,6 +138,11 @@ ASTERISK_REGISTER_FILE() Will be executed when the recording is over. Any strings matching ^{X} will be unescaped to X. All variables will be evaluated at the time MixMonitor is called. + Do not use untrusted strings such as CALLERID(num) + or CALLERID(name) as part of the command parameters. You + risk a command injection attack executing arbitrary commands if the untrusted + strings aren't filtered to remove dangerous characters. See function + FILTER(). @@ -150,6 +155,11 @@ ASTERISK_REGISTER_FILE() Will contain the filename used to record. + Do not use untrusted strings such as CALLERID(num) + or CALLERID(name) as part of ANY of the application's + parameters. You risk a command injection attack executing arbitrary commands + if the untrusted strings aren't filtered to remove dangerous characters. See + function FILTER(). Monitor @@ -224,6 +234,11 @@ ASTERISK_REGISTER_FILE() Will be executed when the recording is over. Any strings matching ^{X} will be unescaped to X. All variables will be evaluated at the time MixMonitor is called. + Do not use untrusted strings such as CALLERID(num) + or CALLERID(name) as part of the command parameters. You + risk a command injection attack executing arbitrary commands if the untrusted + strings aren't filtered to remove dangerous characters. See function + FILTER(). diff --git a/apps/app_system.c b/apps/app_system.c index 7fcffb196..74d33119f 100644 --- a/apps/app_system.c +++ b/apps/app_system.c @@ -48,6 +48,11 @@ ASTERISK_REGISTER_FILE() Command to execute + Do not use untrusted strings such as CALLERID(num) + or CALLERID(name) as part of the command parameters. You + risk a command injection attack executing arbitrary commands if the untrusted + strings aren't filtered to remove dangerous characters. See function + FILTER(). @@ -73,6 +78,11 @@ ASTERISK_REGISTER_FILE() Command to execute + Do not use untrusted strings such as CALLERID(num) + or CALLERID(name) as part of the command parameters. You + risk a command injection attack executing arbitrary commands if the untrusted + strings aren't filtered to remove dangerous characters. See function + FILTER(). -- cgit v1.2.3