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

3 files changed, 50 insertions, 11 deletions

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()
 				<para>Will be executed when the recording is over.</para>
 				<para>Any strings matching <literal>^{X}</literal> will be unescaped to <variable>X</variable>.</para>
 				<para>All variables will be evaluated at the time MixMonitor is called.</para>
+				<warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable>
+				or <variable>CALLERID(name)</variable> 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
+				<variable>FILTER()</variable>.</para></warning>
 			</parameter>
 		</syntax>
 		<description>
@@ -150,6 +155,11 @@ ASTERISK_REGISTER_FILE()
 					<para>Will contain the filename used to record.</para>
 				</variable>
 			</variablelist>
+			<warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable>
+			or <variable>CALLERID(name)</variable> 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 <variable>FILTER()</variable>.</para></warning>
 		</description>
 		<see-also>
 			<ref type="application">Monitor</ref>
@@ -224,6 +234,11 @@ ASTERISK_REGISTER_FILE()
 				<para>Will be executed when the recording is over.
 				Any strings matching <literal>^{X}</literal> will be unescaped to <variable>X</variable>.
 				All variables will be evaluated at the time MixMonitor is called.</para>
+				<warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable>
+				or <variable>CALLERID(name)</variable> 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
+				<variable>FILTER()</variable>.</para></warning>
 			</parameter>
 		</syntax>
 		<description>
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()
 		<syntax>
 			<parameter name="command" required="true">
 				<para>Command to execute</para>
+				<warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable>
+				or <variable>CALLERID(name)</variable> 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
+				<variable>FILTER()</variable>.</para></warning>
 			</parameter>
 		</syntax>
 		<description>
@@ -73,6 +78,11 @@ ASTERISK_REGISTER_FILE()
 		<syntax>
 			<parameter name="command" required="true">
 				<para>Command to execute</para>
+				<warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable>
+				or <variable>CALLERID(name)</variable> 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
+				<variable>FILTER()</variable>.</para></warning>
 			</parameter>
 		</syntax>
 		<description>