Properly quote command and arguments passed to system() in down-root.

[gw]

Copied from pull request: ​https://github.com/OpenVPN/openvpn/pull/9

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.

[David Sommerseth]

A patch resolving this has been sent to the -devel mailing list for review:
​http://thread.gmane.org/gmane.network.openvpn.devel/9238

Together which this patch, which enhances error messages:
​http://thread.gmane.org/gmane.network.openvpn.devel/9247

Once the patches receives the needed ACKs, they will be applied to at least the master branch, prepared for the next major release.

[David Sommerseth]

commit b0f2c521303b7bceb6806680363bc4b9d225e5b8 (master)
commit b4a14914e03c23bc84607e7b956bba5b5d54522b (release/2.3)

Author: David Sommerseth
Date:   Mon Nov 17 23:14:20 2014 +0100

     down-root: Improve error messages

     Signed-off-by: David Sommerseth <davids <at> redhat.com>
     Acked-by: Steffan Karger <steffan.karger <at> fox-it.com>
     Message-Id: <1416262460-9158-1-git-send-email-openvpn.list <at> topphemmelig.net>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/9247
     Signed-off-by: Gert Doering <gert <at> greenie.muc.de>

commit f87b1beccb817e1633bc95bd5dd19deec35c7edc (master)
commit 07d61fee2047686e5bec237a6cd7467244f91e13 (release/2.3)

Author: David Sommerseth
Date:   Sun Nov 16 15:31:02 2014 +0100

     down-root plugin: Replaced system() calls with execve()

     Signed-off-by: David Sommerseth <davids <at> redhat.com>
     Acked-by: Steffan Karger <steffan.karger <at> fox-it.com>
     Message-Id: <1416148262-20978-1-git-send-email-openvpn.list <at> topphemmelig.net>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/9238
     Signed-off-by: Gert Doering <gert <at> greenie.muc.de>