Via GitHub Pull Request https://github.com/OpenVPN/openvpn/pull/9
Attached patch file for convenience
------------------------------------------------------
Suppose the following command:
openvpn --config config \
--plugin /usr/lib/openvpn/openvpn-plugin-down-root.so "bash -c
\"script_type=down cmd\""
One might suspect that this would eventually call execve with an array
like: ["bash", "-c", "script_type=down cmd"]. However the array passed
would currently be: ["bash", "-c", "script_type=down", "cmd"]
What happens is the plugin argument processing will (eventually) pass
bash -c "script_type=down cmd" to parse_line, which will parse the
string to an array as ["bash", "-c", "script_type=down cmd"] and pass it
as the argv parameter to down-root's openvpn_plugin_open_v1. So far so
good.
Then down-root flattens out the array to a single string by joining the
elements with a space using build_command_line. So then we get bash -c
script_type=down cmd. This will be fed as the argument to system(),
which internally runs sh -c 'bash -c script_type=down cmd'. So bash
would then run the command script_type=down with the first argument of
its argument array being cmd, not what we had wanted.
The problem is down-root plugin's build_command_line assumes that no
quoting of arguments needs to be done. The current implementation can be
hacked around to get the semantics we want here, however providing the
right escaping requires either luck or knowledge of how down-root is
implemented.
This pull request fixes this by assuming that all elements of the array
passed to build_command_line require quoting. If no quoting is required,
extra quoting will essentially be ignored.
Perhaps a better solution would be to use execvp/execvpe, instead of
flattening the the args to a string, only then have them parsed back
into arrays. That would be a more invasive change though, and I don't
know if there are any cross-platform issues there.
This patch will make a backwards incompatible change for those have
hacked around this issue to get it to work. I would suspect that to be
few.
---
src/plugins/down-root/down-root.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/plugins/down-root/down-root.c
b/src/plugins/down-root/down-root.c
index d51d0e5..9594e4c 100644
--- a/src/plugins/down-root/down-root.c
+++ b/src/plugins/down-root/down-root.c
@@ -233,7 +233,7 @@ build_command_line (const char *argv[])
{
for (i = 0; argv[i]; ++i)
{
- size += (strlen (argv[i]) + 1); /* string length plus trailing space */
+ size += (strlen (argv[i]) + 3); /* string length plus quotes and
trailing space */
++n;
}
}
@@ -251,7 +251,9 @@ build_command_line (const char *argv[])
/* build string */
for (i = 0; i < n; ++i)
{
+ strcat (string, "\"");
strcat (string, argv[i]);
+ strcat (string, "\"");
if (i + 1 < n)
strcat (string, " ");
}
@@ -455,7 +457,6 @@ openvpn_plugin_abort_v1 (openvpn_plugin_handle_t handle)
static void
down_root_server (const int fd, char *command, const char *argv[], const
char *envp[], const int verb)
{
- const char *p[3];
char *command_line = NULL;
char *argv_cat = NULL;
int i;
@@ -482,10 +483,12 @@ down_root_server (const int fd, char *command, const
char *argv[], const char *e
argv_cat = build_command_line (&argv[1]);
else
argv_cat = build_command_line (NULL);
- p[0] = command;
- p[1] = argv_cat;
- p[2] = NULL;
- command_line = build_command_line (p);
+
+ command_line = (char *) malloc (strlen(command) + strlen(argv_cat) + 2);
+ *command_line = '\0';
+ strcat(command_line, command);
+ strcat(command_line, " ");
+ strcat(command_line, argv_cat);
/*
* Save envp in environment
--
1.7.10.4
0001-Properly-quote-command-and-arguments-passed-to-syste.patch
Description: Binary data
