diff --git a/README-SERIOUSLY.bestpractices.txt b/README-SERIOUSLY.bestpractices.txt
index 108adce8f5..b170d29693 100644
--- a/README-SERIOUSLY.bestpractices.txt
+++ b/README-SERIOUSLY.bestpractices.txt
@@ -94,6 +94,13 @@ your ITSP in a place where you didn't expect to allow it. There are a couple of
ways in which you can mitigate this impact: stricter pattern matching, or using
the FILTER() dialplan function.
+The CALLERID(num) and CALLERID(name) values are other commonly used values that
+are sources of data potentially supplied by outside sources. If you use these
+values as parameters to the System(), MixMonitor(), or Monitor() applications
+or the SHELL() dialplan function, you can allow injection of arbitrary operating
+system command execution. The FILTER() dialplan function is available to remove
+dangerous characters from untrusted strings to block the command injection.
+
Strict Pattern Matching
-----------------------
diff --git a/apps/app_minivm.c b/apps/app_minivm.c
index caf9f01073..6b1e8bb836 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(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 : "");
+ snprintf(fquser, sizeof(fquser), "%s@%s", vmu->username, vmu->domain);
- ast_debug(1, "Executing: %s\n", arguments);
- ast_safe_system(arguments);
+ 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 %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 979bf2d704..24ce3b6ef5 100644
--- a/apps/app_mixmonitor.c
+++ b/apps/app_mixmonitor.c
@@ -138,6 +138,11 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
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_FILE_VERSION(__FILE__, "$Revision$")
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_FILE_VERSION(__FILE__, "$Revision$")
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 7fe453de13..e868a07dc2 100644
--- a/apps/app_system.c
+++ b/apps/app_system.c
@@ -48,6 +48,11 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
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_FILE_VERSION(__FILE__, "$Revision$")
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().
diff --git a/configs/samples/minivm.conf.sample b/configs/samples/minivm.conf.sample
index 2df3449d13..79fdbb0e2c 100644
--- a/configs/samples/minivm.conf.sample
+++ b/configs/samples/minivm.conf.sample
@@ -51,7 +51,7 @@ silencethreshold=128
; If you need to have an external program, i.e. /usr/bin/myapp called when a
; voicemail is received by the server. The arguments are
;
-;
+;
;
;externnotify=/usr/bin/myapp
; The character set for voicemail messages can be specified here
diff --git a/funcs/func_shell.c b/funcs/func_shell.c
index e403efc2e8..79b7f99401 100644
--- a/funcs/func_shell.c
+++ b/funcs/func_shell.c
@@ -84,6 +84,11 @@ static int shell_helper(struct ast_channel *chan, const char *cmd, char *data,
The command that the shell should 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().
diff --git a/include/asterisk/app.h b/include/asterisk/app.h
index 86336e32ba..5b10b1c1c7 100644
--- a/include/asterisk/app.h
+++ b/include/asterisk/app.h
@@ -871,9 +871,34 @@ int ast_vm_test_destroy_user(const char *context, const char *mailbox);
int ast_vm_test_create_user(const char *context, const char *mailbox);
#endif
-/*! \brief Safely spawn an external program while closing file descriptors
- \note This replaces the \b system call in all Asterisk modules
-*/
+/*!
+ * \brief Safely spawn an external program while closing file descriptors
+ *
+ * \note This replaces the \b execvp call in all Asterisk modules
+ *
+ * \param dualfork Non-zero to simulate running the program in the
+ * background by forking twice. The option provides similar
+ * functionality to the '&' in the OS shell command "cmd &". The
+ * option allows Asterisk to run a reaper loop to watch the first fork
+ * which immediately exits after spaning the second fork. The actual
+ * program is run in the second fork.
+ * \param file execvp(file, argv) file parameter
+ * \param argv execvp(file, argv) argv parameter
+ */
+int ast_safe_execvp(int dualfork, const char *file, char *const argv[]);
+
+/*!
+ * \brief Safely spawn an OS shell command while closing file descriptors
+ *
+ * \note This replaces the \b system call in all Asterisk modules
+ *
+ * \param s - OS shell command string to execute.
+ *
+ * \warning Command injection can happen using this call if the passed
+ * in string is created using untrusted data from an external source.
+ * It is best not to use untrusted data. However, the caller could
+ * filter out dangerous characters to avoid command injection.
+ */
int ast_safe_system(const char *s);
/*!
diff --git a/main/asterisk.c b/main/asterisk.c
index e071f6f585..0818cfbc37 100644
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -1286,11 +1286,10 @@ void ast_unreplace_sigchld(void)
ast_mutex_unlock(&safe_system_lock);
}
-int ast_safe_system(const char *s)
+/*! \brief fork and perform other preparations for spawning applications */
+static pid_t safe_exec_prep(int dualfork)
{
pid_t pid;
- int res;
- int status;
#if defined(HAVE_WORKING_FORK) || defined(HAVE_WORKING_VFORK)
ast_replace_sigchld();
@@ -1312,35 +1311,101 @@ int ast_safe_system(const char *s)
cap_free(cap);
#endif
#ifdef HAVE_WORKING_FORK
- if (ast_opt_high_priority)
+ if (ast_opt_high_priority) {
ast_set_priority(0);
+ }
/* Close file descriptors and launch system command */
ast_close_fds_above_n(STDERR_FILENO);
#endif
- execl("/bin/sh", "/bin/sh", "-c", s, (char *) NULL);
- _exit(1);
- } else if (pid > 0) {
+ if (dualfork) {
+#ifdef HAVE_WORKING_FORK
+ pid = fork();
+#else
+ pid = vfork();
+#endif
+ if (pid < 0) {
+ /* Second fork failed. */
+ /* No logger available. */
+ _exit(1);
+ }
+
+ if (pid > 0) {
+ /* This is the first fork, exit so the reaper finishes right away. */
+ _exit(0);
+ }
+
+ /* This is the second fork. The first fork will exit immediately so
+ * Asterisk doesn't have to wait for completion.
+ * ast_safe_system("cmd &") would run in the background, but the '&'
+ * cannot be added with ast_safe_execvp, so we have to double fork.
+ */
+ }
+ }
+
+ if (pid < 0) {
+ ast_log(LOG_WARNING, "Fork failed: %s\n", strerror(errno));
+ }
+#else
+ ast_log(LOG_WARNING, "Fork failed: %s\n", strerror(ENOTSUP));
+ pid = -1;
+#endif
+
+ return pid;
+}
+
+/*! \brief wait for spawned application to complete and unreplace sigchld */
+static int safe_exec_wait(pid_t pid)
+{
+ int res = -1;
+
+#if defined(HAVE_WORKING_FORK) || defined(HAVE_WORKING_VFORK)
+ if (pid > 0) {
for (;;) {
+ int status;
+
res = waitpid(pid, &status, 0);
if (res > -1) {
res = WIFEXITED(status) ? WEXITSTATUS(status) : -1;
break;
- } else if (errno != EINTR)
+ }
+ if (errno != EINTR) {
break;
+ }
}
- } else {
- ast_log(LOG_WARNING, "Fork failed: %s\n", strerror(errno));
- res = -1;
}
ast_unreplace_sigchld();
-#else /* !defined(HAVE_WORKING_FORK) && !defined(HAVE_WORKING_VFORK) */
- res = -1;
#endif
return res;
}
+int ast_safe_execvp(int dualfork, const char *file, char *const argv[])
+{
+ pid_t pid = safe_exec_prep(dualfork);
+
+ if (pid == 0) {
+ execvp(file, argv);
+ _exit(1);
+ /* noreturn from _exit */
+ }
+
+ return safe_exec_wait(pid);
+}
+
+int ast_safe_system(const char *s)
+{
+ pid_t pid = safe_exec_prep(0);
+
+ if (pid == 0) {
+ execl("/bin/sh", "/bin/sh", "-c", s, (char *) NULL);
+ _exit(1);
+ /* noreturn from _exit */
+ }
+
+ return safe_exec_wait(pid);
+}
+
/*!
* \brief enable or disable a logging level to a specified console
*/
diff --git a/res/res_monitor.c b/res/res_monitor.c
index 7ec1bb6c50..c4ee674f91 100644
--- a/res/res_monitor.c
+++ b/res/res_monitor.c
@@ -62,17 +62,17 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
- optional, if not set, defaults to wav
+ Optional. If not set, defaults to wav
- if set, changes the filename used to the one specified.
+ If set, changes the filename used to the one specified.