<p>Joshua Colp <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6362">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 9359f82..b34239a 100644<br>--- a/apps/app_minivm.c<br>+++ b/apps/app_minivm.c<br>@@ -1755,21 +1755,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/6362">change 6362</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/6362"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 15.0 </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: 6362 </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: Joshua Colp <jcolp@digium.com> </div>