Hi,
I did some review over the weekend, but haven't yet run the code, yet
thought of sending these comments in light of the Windows meeting today.
On Tue, Jan 26, 2016 at 2:11 PM, Gert Doering <g...@greenie.muc.de> wrote:
>
> From: Heiko Hund <heiko.h...@sophos.com>
>
> v1: Heiko Hund
> - Message-ID: <2215306.x9ci9DhAZ9@de-gn-40970>
> - extend openvpn service to provide "automatic service" and "interactive
> service" (which is used by GUI and OpenVPN to run openvpn
non-privileged
> and still be able to install routes and configure IPv6 addresses)
> - add --msg-channel <n> option to openvpn to tell it which pipe to use
> to talk to the interactive service (used in tun.c for ifconfig + ARP
flush,
> and route.c for routing)
> - add openvpn-msg.h with message definitions for talking to interactive
service
> - routing in openvpn uses message-pipe automatically if --msg-channel
<n> is
> configured, no other option needed
> - today, the integration in route.c and tun.c is windows-only, but could
be adapted
> to other platforms
(snip)
> src/openvpnserv/Makefile.am | 14 +-
> src/openvpnserv/automatic.c | 416 ++++++++++++++
The "automatic service" part is essentially the same code as the original
except for some mostly cosmetic changes and code re-organization. The
changes are all good and makes the code leaner and cleaner.
But it fixes none of the weaknesses of the original service.
As suggested before, it would be better to make the interactive service a
new executable without the automatic service functionality. Then the
original service could be independently improved or replaced by something
better (openvpnserv2, or whatever)
> src/openvpnserv/common.c | 211 +++++++
> src/openvpnserv/interactive.c | 1273
+++++++++++++++++++++++++++++++++++++++++
The new code implementing the interactive service looks good except for
some possible security concerns.
First, the service starts openvpn.exe as user using a command line supplied
by the user (say by a UI like OpenVPNGUI). The executable name (openvpn.exe)
itself is taken from the registry entry (exe_path) which is good, but the UI
can pass arbitrary options and specify the working directory. This means
arbitrary config files can be passed as arguments to openvpn.exe enabling
arbitrary routes to be set by an unprivileged user using config file
directives or command line options.
While the whole purpose of the interactive service is to allow unprivileged
users to set routes or do ifconfig, admin users should have some ability to
control what routes will be set or which remote servers will be connected
to by unprivileged users. This requires two changes:
(1) the config file should reside in some pre-defined location(s)
controlled by, say, a registry key that only an admin user can change
(2) only a limited set of "safe" options may be allowed on the command line
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index b710c9c..6d97b4f 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -6015,6 +6015,24 @@ add_option (struct options *options,
> }
> #endif
> #endif
> + else if (streq (p[0], "msg-channel") && p[1])
> + {
> +#ifdef WIN32
> + VERIFY_PERMISSION (OPT_P_GENERAL);
> + HANDLE process = GetCurrentProcess ();
> + HANDLE handle = (HANDLE) atoi (p[1]);
> + if (!DuplicateHandle (process, handle, process,
> &options->msg_channel, 0,
> + FALSE, DUPLICATE_CLOSE_SOURCE |
> DUPLICATE_SAME_ACCESS))
> + {
> + msg (msglevel, "could not duplicate service pipe handle");
> + goto err;
> + }
>
The msg_channel handle is passed on to openvpn.exe as a command line
option, so it cannot trust it (could be that of a named pipe opened by a
rogue process). This looks insecure ---- such a pipe server could
impersonate the user.
A more serious problem is related to the the service requiring connections
from UI with impersonation allowed. Again, an unprivileged process
pretending to be the service could escalate privileges if an OpenVPNGUI
instance running as admin connects to it. This looks easy to exploit.
Possible mitigation:
(i) The GUI should not connect to the interactive service if running with
elevated privileges
(ii) openvpn.exe should not honor the --msg-channel option if running with
elevated privileges
Other comments I have are more specific to code snippets and of minor
consequence. Will send them after compile/test runs.
Selva
