<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6347">View Change</a></p><div style="white-space:pre-wrap">Approvals:
Joshua Colp: Looks good to me, approved; Approved for Submit
</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">AST-2017-006: Fix app_minivm application MinivmNotify command injection<br><br>An admin can configure app_minivm with an externnotify program to be run<br>when a voicemail is received. The app_minivm application MinivmNotify<br>uses ast_safe_system() for this purpose which is vulnerable to command<br>injection since the Caller-ID name and number values given to externnotify<br>can come from an external untrusted source.<br><br>* Add ast_safe_execvp() function. This gives modules the ability to run<br>external commands with greater safety compared to ast_safe_system().<br>Specifically when some parameters are filled by untrusted sources the new<br>function does not allow malicious input to break argument encoding. This<br>may be of particular concern where CALLERID(name) or CALLERID(num) may be<br>used as a parameter to a script run by ast_safe_system() which could<br>potentially allow arbitrary command execution.<br><br>* Changed app_minivm.c:run_externnotify() to use the new ast_safe_execvp()<br>instead of ast_safe_system() to avoid command injection.<br><br>* Document code injection potential from untrusted data sources for other<br>shell commands that are under user control.<br><br>ASTERISK-27103<br><br>Change-Id: I7552472247a84cde24e1358aaf64af160107aef1<br>---<br>M README-SERIOUSLY.bestpractices.txt<br>M apps/app_minivm.c<br>M apps/app_mixmonitor.c<br>M apps/app_system.c<br>M configs/samples/minivm.conf.sample<br>M funcs/func_shell.c<br>M include/asterisk/app.h<br>M main/asterisk.c<br>M res/res_monitor.c<br>9 files changed, 179 insertions(+), 31 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/README-SERIOUSLY.bestpractices.txt b/README-SERIOUSLY.bestpractices.txt<br>index b6b418d..0d3e670 100644<br>--- a/README-SERIOUSLY.bestpractices.txt<br>+++ b/README-SERIOUSLY.bestpractices.txt<br>@@ -94,6 +94,13 @@<br> ways in which you can mitigate this impact: stricter pattern matching, or using<br> the FILTER() dialplan function.<br> <br>+The CALLERID(num) and CALLERID(name) values are other commonly used values that<br>+are sources of data potentially supplied by outside sources. If you use these<br>+values as parameters to the System(), MixMonitor(), or Monitor() applications<br>+or the SHELL() dialplan function, you can allow injection of arbitrary operating<br>+system command execution. The FILTER() dialplan function is available to remove<br>+dangerous characters from untrusted strings to block the command injection.<br>+<br> Strict Pattern Matching<br> -----------------------<br> <br>diff --git a/apps/app_minivm.c b/apps/app_minivm.c<br>index 15449ad..0d7a5f4 100644<br>--- a/apps/app_minivm.c<br>+++ b/apps/app_minivm.c<br>@@ -1774,21 +1774,35 @@<br> /*! \brief Run external notification for voicemail message */<br> static void run_externnotify(struct ast_channel *chan, struct minivm_account *vmu)<br> {<br>- char arguments[BUFSIZ];<br>+ char fquser[AST_MAX_CONTEXT * 2];<br>+ char *argv[5] = { NULL };<br>+ struct ast_party_caller *caller;<br>+ char *cid;<br>+ int idx;<br> <br>- if (ast_strlen_zero(vmu->externnotify) && ast_strlen_zero(global_externnotify))<br>+ if (ast_strlen_zero(vmu->externnotify) && ast_strlen_zero(global_externnotify)) {<br> return;<br>+ }<br> <br>- snprintf(arguments, sizeof(arguments), "%s %s@%s %s %s&", <br>- ast_strlen_zero(vmu->externnotify) ? global_externnotify : vmu->externnotify, <br>- vmu->username, vmu->domain,<br>- (ast_channel_caller(chan)->id.name.valid && ast_channel_caller(chan)->id.name.str)<br>- ? ast_channel_caller(chan)->id.name.str : "",<br>- (ast_channel_caller(chan)->id.number.valid && ast_channel_caller(chan)->id.number.str)<br>- ? ast_channel_caller(chan)->id.number.str : "");<br>+ snprintf(fquser, sizeof(fquser), "%s@%s", vmu->username, vmu->domain);<br> <br>- ast_debug(1, "Executing: %s\n", arguments);<br>- ast_safe_system(arguments);<br>+ caller = ast_channel_caller(chan);<br>+ idx = 0;<br>+ argv[idx++] = ast_strlen_zero(vmu->externnotify) ? global_externnotify : vmu->externnotify;<br>+ argv[idx++] = fquser;<br>+ cid = S_COR(caller->id.name.valid, caller->id.name.str, NULL);<br>+ if (cid) {<br>+ argv[idx++] = cid;<br>+ }<br>+ cid = S_COR(caller->id.number.valid, caller->id.number.str, NULL);<br>+ if (cid) {<br>+ argv[idx++] = cid;<br>+ }<br>+ argv[idx] = NULL;<br>+<br>+ ast_debug(1, "Executing: %s %s %s %s\n",<br>+ argv[0], argv[1], argv[2] ?: "", argv[3] ?: "");<br>+ ast_safe_execvp(1, argv[0], argv);<br> }<br> <br> /*!\internal<br>diff --git a/apps/app_mixmonitor.c b/apps/app_mixmonitor.c<br>index 3258b30..ac45642 100644<br>--- a/apps/app_mixmonitor.c<br>+++ b/apps/app_mixmonitor.c<br>@@ -136,6 +136,11 @@<br> <para>Will be executed when the recording is over.</para><br> <para>Any strings matching <literal>^{X}</literal> will be unescaped to <variable>X</variable>.</para><br> <para>All variables will be evaluated at the time MixMonitor is called.</para><br>+ <warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable><br>+ or <variable>CALLERID(name)</variable> as part of the command parameters. You<br>+ risk a command injection attack executing arbitrary commands if the untrusted<br>+ strings aren't filtered to remove dangerous characters. See function<br>+ <variable>FILTER()</variable>.</para></warning><br> </parameter><br> </syntax><br> <description><br>@@ -148,6 +153,11 @@<br> <para>Will contain the filename used to record.</para><br> </variable><br> </variablelist><br>+ <warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable><br>+ or <variable>CALLERID(name)</variable> as part of ANY of the application's<br>+ parameters. You risk a command injection attack executing arbitrary commands<br>+ if the untrusted strings aren't filtered to remove dangerous characters. See<br>+ function <variable>FILTER()</variable>.</para></warning><br> </description><br> <see-also><br> <ref type="application">Monitor</ref><br>@@ -222,6 +232,11 @@<br> <para>Will be executed when the recording is over.<br> Any strings matching <literal>^{X}</literal> will be unescaped to <variable>X</variable>.<br> All variables will be evaluated at the time MixMonitor is called.</para><br>+ <warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable><br>+ or <variable>CALLERID(name)</variable> as part of the command parameters. You<br>+ risk a command injection attack executing arbitrary commands if the untrusted<br>+ strings aren't filtered to remove dangerous characters. See function<br>+ <variable>FILTER()</variable>.</para></warning><br> </parameter><br> </syntax><br> <description><br>diff --git a/apps/app_system.c b/apps/app_system.c<br>index 09179f7..64d5297 100644<br>--- a/apps/app_system.c<br>+++ b/apps/app_system.c<br>@@ -46,6 +46,11 @@<br> <syntax><br> <parameter name="command" required="true"><br> <para>Command to execute</para><br>+ <warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable><br>+ or <variable>CALLERID(name)</variable> as part of the command parameters. You<br>+ risk a command injection attack executing arbitrary commands if the untrusted<br>+ strings aren't filtered to remove dangerous characters. See function<br>+ <variable>FILTER()</variable>.</para></warning><br> </parameter><br> </syntax><br> <description><br>@@ -71,6 +76,11 @@<br> <syntax><br> <parameter name="command" required="true"><br> <para>Command to execute</para><br>+ <warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable><br>+ or <variable>CALLERID(name)</variable> as part of the command parameters. You<br>+ risk a command injection attack executing arbitrary commands if the untrusted<br>+ strings aren't filtered to remove dangerous characters. See function<br>+ <variable>FILTER()</variable>.</para></warning><br> </parameter><br> </syntax><br> <description><br>diff --git a/configs/samples/minivm.conf.sample b/configs/samples/minivm.conf.sample<br>index 2df3449..79fdbb0 100644<br>--- a/configs/samples/minivm.conf.sample<br>+++ b/configs/samples/minivm.conf.sample<br>@@ -51,7 +51,7 @@<br> ; If you need to have an external program, i.e. /usr/bin/myapp called when a<br> ; voicemail is received by the server. The arguments are<br> ;<br>-; <app> <username@domain> <callerid-number> <callerid-name><br>+; <app> <username@domain> <callerid-name> <callerid-number><br> ;<br> ;externnotify=/usr/bin/myapp<br> ; The character set for voicemail messages can be specified here<br>diff --git a/funcs/func_shell.c b/funcs/func_shell.c<br>index 0398cd8..fe1debe 100644<br>--- a/funcs/func_shell.c<br>+++ b/funcs/func_shell.c<br>@@ -82,6 +82,11 @@<br> <syntax><br> <parameter name="command" required="true"><br> <para>The command that the shell should execute.</para><br>+ <warning><para>Do not use untrusted strings such as <variable>CALLERID(num)</variable><br>+ or <variable>CALLERID(name)</variable> as part of the command parameters. You<br>+ risk a command injection attack executing arbitrary commands if the untrusted<br>+ strings aren't filtered to remove dangerous characters. See function<br>+ <variable>FILTER()</variable>.</para></warning><br> </parameter><br> </syntax><br> <description><br>diff --git a/include/asterisk/app.h b/include/asterisk/app.h<br>index d86b633..0505a6b 100644<br>--- a/include/asterisk/app.h<br>+++ b/include/asterisk/app.h<br>@@ -871,9 +871,34 @@<br> int ast_vm_test_create_user(const char *context, const char *mailbox);<br> #endif<br> <br>-/*! \brief Safely spawn an external program while closing file descriptors<br>- \note This replaces the \b system call in all Asterisk modules<br>-*/<br>+/*!<br>+ * \brief Safely spawn an external program while closing file descriptors<br>+ *<br>+ * \note This replaces the \b execvp call in all Asterisk modules<br>+ *<br>+ * \param dualfork Non-zero to simulate running the program in the<br>+ * background by forking twice. The option provides similar<br>+ * functionality to the '&' in the OS shell command "cmd &". The<br>+ * option allows Asterisk to run a reaper loop to watch the first fork<br>+ * which immediately exits after spaning the second fork. The actual<br>+ * program is run in the second fork.<br>+ * \param file execvp(file, argv) file parameter<br>+ * \param argv execvp(file, argv) argv parameter<br>+ */<br>+int ast_safe_execvp(int dualfork, const char *file, char *const argv[]);<br>+<br>+/*!<br>+ * \brief Safely spawn an OS shell command while closing file descriptors<br>+ *<br>+ * \note This replaces the \b system call in all Asterisk modules<br>+ *<br>+ * \param s - OS shell command string to execute.<br>+ *<br>+ * \warning Command injection can happen using this call if the passed<br>+ * in string is created using untrusted data from an external source.<br>+ * It is best not to use untrusted data. However, the caller could<br>+ * filter out dangerous characters to avoid command injection.<br>+ */<br> int ast_safe_system(const char *s);<br> <br> /*!<br>diff --git a/main/asterisk.c b/main/asterisk.c<br>index 0a131fd..d555949 100644<br>--- a/main/asterisk.c<br>+++ b/main/asterisk.c<br>@@ -1170,11 +1170,10 @@<br> ast_mutex_unlock(&safe_system_lock);<br> }<br> <br>-int ast_safe_system(const char *s)<br>+/*! \brief fork and perform other preparations for spawning applications */<br>+static pid_t safe_exec_prep(int dualfork)<br> {<br> pid_t pid;<br>- int res;<br>- int status;<br> <br> #if defined(HAVE_WORKING_FORK) || defined(HAVE_WORKING_VFORK)<br> ast_replace_sigchld();<br>@@ -1196,35 +1195,101 @@<br> cap_free(cap);<br> #endif<br> #ifdef HAVE_WORKING_FORK<br>- if (ast_opt_high_priority)<br>+ if (ast_opt_high_priority) {<br> ast_set_priority(0);<br>+ }<br> /* Close file descriptors and launch system command */<br> ast_close_fds_above_n(STDERR_FILENO);<br> #endif<br>- execl("/bin/sh", "/bin/sh", "-c", s, (char *) NULL);<br>- _exit(1);<br>- } else if (pid > 0) {<br>+ if (dualfork) {<br>+#ifdef HAVE_WORKING_FORK<br>+ pid = fork();<br>+#else<br>+ pid = vfork();<br>+#endif<br>+ if (pid < 0) {<br>+ /* Second fork failed. */<br>+ /* No logger available. */<br>+ _exit(1);<br>+ }<br>+<br>+ if (pid > 0) {<br>+ /* This is the first fork, exit so the reaper finishes right away. */<br>+ _exit(0);<br>+ }<br>+<br>+ /* This is the second fork. The first fork will exit immediately so<br>+ * Asterisk doesn't have to wait for completion.<br>+ * ast_safe_system("cmd &") would run in the background, but the '&'<br>+ * cannot be added with ast_safe_execvp, so we have to double fork.<br>+ */<br>+ }<br>+ }<br>+<br>+ if (pid < 0) {<br>+ ast_log(LOG_WARNING, "Fork failed: %s\n", strerror(errno));<br>+ }<br>+#else<br>+ ast_log(LOG_WARNING, "Fork failed: %s\n", strerror(ENOTSUP));<br>+ pid = -1;<br>+#endif<br>+<br>+ return pid;<br>+}<br>+<br>+/*! \brief wait for spawned application to complete and unreplace sigchld */<br>+static int safe_exec_wait(pid_t pid)<br>+{<br>+ int res = -1;<br>+<br>+#if defined(HAVE_WORKING_FORK) || defined(HAVE_WORKING_VFORK)<br>+ if (pid > 0) {<br> for (;;) {<br>+ int status;<br>+<br> res = waitpid(pid, &status, 0);<br> if (res > -1) {<br> res = WIFEXITED(status) ? WEXITSTATUS(status) : -1;<br> break;<br>- } else if (errno != EINTR)<br>+ }<br>+ if (errno != EINTR) {<br> break;<br>+ }<br> }<br>- } else {<br>- ast_log(LOG_WARNING, "Fork failed: %s\n", strerror(errno));<br>- res = -1;<br> }<br> <br> ast_unreplace_sigchld();<br>-#else /* !defined(HAVE_WORKING_FORK) && !defined(HAVE_WORKING_VFORK) */<br>- res = -1;<br> #endif<br> <br> return res;<br> }<br> <br>+int ast_safe_execvp(int dualfork, const char *file, char *const argv[])<br>+{<br>+ pid_t pid = safe_exec_prep(dualfork);<br>+<br>+ if (pid == 0) {<br>+ execvp(file, argv);<br>+ _exit(1);<br>+ /* noreturn from _exit */<br>+ }<br>+<br>+ return safe_exec_wait(pid);<br>+}<br>+<br>+int ast_safe_system(const char *s)<br>+{<br>+ pid_t pid = safe_exec_prep(0);<br>+<br>+ if (pid == 0) {<br>+ execl("/bin/sh", "/bin/sh", "-c", s, (char *) NULL);<br>+ _exit(1);<br>+ /* noreturn from _exit */<br>+ }<br>+<br>+ return safe_exec_wait(pid);<br>+}<br>+<br> /*!<br> * \brief enable or disable a logging level to a specified console<br> */<br>diff --git a/res/res_monitor.c b/res/res_monitor.c<br>index fd3ff7a..3e3611b 100644<br>--- a/res/res_monitor.c<br>+++ b/res/res_monitor.c<br>@@ -59,17 +59,17 @@<br> <syntax><br> <parameter name="file_format" argsep=":"><br> <argument name="file_format" required="true"><br>- <para>optional, if not set, defaults to <literal>wav</literal></para><br>+ <para>Optional. If not set, defaults to <literal>wav</literal></para><br> </argument><br> <argument name="urlbase" /><br> </parameter><br> <parameter name="fname_base"><br>- <para>if set, changes the filename used to the one specified.</para><br>+ <para>If set, changes the filename used to the one specified.</para><br> </parameter><br> <parameter name="options"><br> <optionlist><br> <option name="m"><br>- <para>when the recording ends mix the two leg files into one and<br>+ <para>When the recording ends mix the two leg files into one and<br> delete the two leg files. If the variable <variable>MONITOR_EXEC</variable><br> is set, the application referenced in it will be executed instead of<br> soxmix/sox and the raw leg files will NOT be deleted automatically.<br>@@ -80,6 +80,13 @@<br> will be passed on as additional arguments to <variable>MONITOR_EXEC</variable>.<br> Both <variable>MONITOR_EXEC</variable> and the Mix flag can be set from the<br> administrator interface.</para><br>+ <warning><para>Do not use untrusted strings such as<br>+ <variable>CALLERID(num)</variable> or <variable>CALLERID(name)</variable><br>+ as part of <variable>MONITOR_EXEC</variable> or<br>+ <variable>MONITOR_EXEC_ARGS</variable>. You risk a command injection<br>+ attack executing arbitrary commands if the untrusted strings aren't<br>+ filtered to remove dangerous characters. See function<br>+ <variable>FILTER()</variable>.</para></warning><br> </option><br> <option name="b"><br> <para>Don't begin recording unless a call is bridged to another channel.</para><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6347">change 6347</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/6347"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15 </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: I7552472247a84cde24e1358aaf64af160107aef1 </div>
<div style="display:none"> Gerrit-Change-Number: 6347 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>