Make OpenVPN set routes on Windows Vista and later

Hi everyone

I've been working on a Windows service that enables OpenVPN to do privileged network operations on Windows Vista and later. You can find the code here:
https://sourceforge.net/p/openvpn-gui/openvpn/ci/interactive_service/tree/

Here's an overview of how it works:

0) A regular user runs the GUI and connects a VPN.
1) The GUI requests an openvpn.exe instance running with a certain config from the service.
2) The service starts openvpn as the user and provides a IPC pipe to the process for requesting defined privileged operations.
3) The GUI talks to openvpn using the management interface.
4) Along the way openvpn requests routes to be set and other things.

Since openvpn is run as the same user the GUI is run for, the hook scripts are only capable of doing operations the user is allowed to perform. The pipe can only be used by the openvpn process and is not accessible for other processes. So this code could be a nice base for further securing openvpn on Windows while keeping it fully functional.

To get a feeling of how this works you could look at the files:
include/openvpn-msg.h
src/openvpn/route.c
src/openvpnserv/interactive.c

Please review and comment.

Cheers
Heiko

Hi Heiko,

This morning I finally found some time to set up a windows build environment to cross-compile the windows builds from linux. I'm trying to find more time to really review and test. For now just some preliminary stuff I came across:

(1) open_tun(), close_tun(), etc :
Instead of passing the options struct, consider just passing the msg_channel. That would require it to be something more generic than HANDLE, but I think this model makes sense for other platforms too.

(2) includes: you should include "openvpn-msg.h", instead of "include/openvpn-msg.h" (that's how it's done for openvpn-plugin.h too):

openvpn/src/openvpn/route.c:48:33: fatal error: include/openvpn-msg.h:
No such file or directory
#include "include/openvpn-msg.h"

Attached a patch with the changes I had to make.

(3) mingw/GCC has a warning for me:

openvpn/src/openvpnserv/interactive.c:708:11: warning: enumeration value ‘_undo_type_max’ not handled in switch [-Wswitch]

           switch (type)
           ^

For others: if you have Windows build and test setups, please help out. Especially Windows users willing to test are welcome. This feature deserves more attention :)

-Steffan

From 1fa191c9edfdef1b22dc04b789162a275cc507ef Mon Sep 17 00:00:00 2001
From: Steffan Karger <[email protected]...>
Date: Sun, 14 Dec 2014 14:12:42 +0100
Subject: [PATCH] Fix openvpn-msg.h includes

---
 src/openvpn/route.c           | 2 +-
 src/openvpn/tun.c             | 2 +-
 src/openvpnserv/Makefile.am   | 3 +++
 src/openvpnserv/interactive.c | 2 +-
 4 files changed, 6 insertions(+), 3 deletions(-)

index 6d7e8e2..55ab11e 100644
@@ -45,7 +45,7 @@
 
 #include "memdbg.h"
 
-#include "include/openvpn-msg.h"
+#include "openvpn-msg.h"
 
 #ifdef WIN32
 #define METRIC_NOT_USED ((DWORD)-1)

index 9b2fa1a..0b629fa 100644
@@ -50,7 +50,7 @@
 
 #include "memdbg.h"
 
-#include "include/openvpn-msg.h"
+#include "openvpn-msg.h"
 
 #include <string.h>
 

index 6088287..4bb1c27 100644
@@ -17,6 +17,9 @@ EXTRA_DIST = \
 	openvpnserv.vcxproj \
 	openvpnserv.vcxproj.filters
 
+AM_CPPFLAGS = \
+	-I$(top_srcdir)/include
+
 if WIN32
 sbin_PROGRAMS = openvpnserv
 openvpnserv_CFLAGS = \

index 368450b..0040d85 100644
@@ -34,7 +34,7 @@
 #include <stdio.h>
 #include <sddl.h>
 
-#include "include/openvpn-msg.h"
+#include "openvpn-msg.h"
 
 #define IO_TIMEOUT  2000 /*ms*/
 

1.9.1

From: Heiko Hund <[email protected]...>

v1: Heiko Hund
- Message-ID: <[email protected]...>
- 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

v2: Steffan Karger
- Message-ID: <[email protected]...>
- include "openvpn-msg.h" not "include/openvpn-msg.h"
- add $(top_srcdir)/include to openvpnsrv build for out-of-tree builds

v3: Gert Doering, rebasing and integrating review feedback
- rebased to 417fe4a72c
- r->metric_defined is now r->flags & RT_METRIC_DEFINED (c3ef2d2333fb)
- move "openvpn-msg.h" include inside #ifdef WIN32 (windows-only right now)
- hide "msg_channel" extra option inside tt->tuntap_options, so we do not need an extra argument to all the add/del_route...() functions
- do_route_ipv6_service(): use r->adapter index (if set) for RGI6 routes

---
 configure.ac                  |    1 +
 include/Makefile.am           |    4 +-
 include/openvpn-msg.h         |  108 ++++
 src/openvpn/buffer.c          |    2 +-
 src/openvpn/init.c            |    6 +
 src/openvpn/options.c         |   18 +
 src/openvpn/options.h         |    1 +
 src/openvpn/route.c           |  317 +++++++---
 src/openvpn/route.h           |    1 +
 src/openvpn/tun.c             |  171 ++++--
 src/openvpn/tun.h             |    4 +
 src/openvpnserv/Makefile.am   |   14 +-
 src/openvpnserv/automatic.c   |  416 ++++++++++++++
 src/openvpnserv/common.c      |  211 +++++++
 src/openvpnserv/interactive.c | 1273 +++++++++++++++++++++++++++++++++++++++++
 src/openvpnserv/openvpnserv.c |  534 -----------------
 src/openvpnserv/service.c     |  861 +++++++---------------------
 src/openvpnserv/service.h     |  213 +++----
 18 files changed, 2706 insertions(+), 1449 deletions(-)
 create mode 100644 include/openvpn-msg.h
 create mode 100644 src/openvpnserv/automatic.c
 create mode 100644 src/openvpnserv/common.c
 create mode 100644 src/openvpnserv/interactive.c
 delete mode 100755 src/openvpnserv/openvpnserv.c

index 7ff2435..6597194 100644
@@ -58,6 +58,7 @@ m4_define([serial_tests], [
 AM_INIT_AUTOMAKE(foreign serial_tests) dnl NB: Do not [quote] this parameter.
 AC_CANONICAL_HOST
 AC_USE_SYSTEM_EXTENSIONS
+AM_PROG_CC_C_O
 
 AC_ARG_ENABLE(
 	[lzo],

index c5a91b1..498b3b5 100644
@@ -13,4 +13,6 @@ MAINTAINERCLEANFILES = \
 	$(srcdir)/Makefile.in \
 	$(srcdir)/openvpn-plugin.h.in
 
-include_HEADERS = openvpn-plugin.h
+include_HEADERS = \
+	openvpn-plugin.h \
+	openvpn-msg.h

index 421d60e..bc67d65 100644
@@ -254,7 +254,7 @@ buf_puts(struct buffer *buf, const char *str)
  *
  * Return false on overflow.
  *
- * This function is duplicated into service-win32/openvpnserv.c
+ * This functionality is duplicated in src/openvpnserv/common.c
  * Any modifications here should be done to the other place as well.
  */
 

index 2beec72..d0020b7 100644
@@ -1433,6 +1433,12 @@ do_open_tun (struct context *c)
       /* initialize (but do not open) tun/tap object */
       do_init_tun (c);
 
+#ifdef WIN32
+      /* store (hide) interactive service handle in tuntap_options */
+      c->c1.tuntap->options.msg_channel = c->options.msg_channel;
+      msg (D_ROUTE, "interactive service msg_channel=%u", (unsigned int) c->options.msg_channel);
+#endif
+
       /* allocate route list structure */
       do_alloc_route_list (c);
 

index b710c9c..6d97b4f 100644
@@ -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;
+        }
+      options->route_method = ROUTE_METHOD_SERVICE;
+#else
+      msg (msglevel, "--msg-channel is only supported on Windows");
+      goto err;
+#endif
+    }
 #ifdef WIN32
   else if (streq (p[0], "win-sys") && p[1] && !p[2])
     {

index 9e8a6a0..23d3992 100644
@@ -581,6 +581,7 @@ struct options
   int foreign_option_index;
 
 #ifdef WIN32
+  HANDLE msg_channel;
   const char *exit_event_name;
   bool exit_event_initial_state;
   bool show_net_up;

index 4bbcdb7..58a5748 100644
@@ -40,6 +40,7 @@
 #define ROUTE_METHOD_ADAPTIVE  0  /* try IP helper first then route.exe */
 #define ROUTE_METHOD_IPAPI     1  /* use IP helper API */
 #define ROUTE_METHOD_EXE       2  /* use route.exe */
+#define ROUTE_METHOD_SERVICE   3  /* use the privileged Windows service */
 #define ROUTE_METHOD_MASK      3
 #endif
 

index 65bacac..4e93a3f 100644
@@ -58,6 +58,10 @@ struct tuntap_options {
 # define IPW32_SET_N            5
   int ip_win32_type;
 
+#ifdef WIN32
+  HANDLE msg_channel;
+#endif
+
   /* --ip-win32 dynamic options */
   bool dhcp_masq_custom_offset;
   int dhcp_masq_offset;

index a989c25..4bb1c27 100644
@@ -17,11 +17,21 @@ EXTRA_DIST = \
 	openvpnserv.vcxproj \
 	openvpnserv.vcxproj.filters
 
+AM_CPPFLAGS = \
+	-I$(top_srcdir)/include
+
 if WIN32
 sbin_PROGRAMS = openvpnserv
+openvpnserv_CFLAGS = \
+	-municode -D_UNICODE \
+	-UNTDDI_VERSION -U_WIN32_WINNT \
+	-D_WIN32_WINNT=_WIN32_WINNT_VISTA
+openvpnserv_LDADD = -ladvapi32 -luserenv -liphlpapi -lws2_32
 endif
 
 openvpnserv_SOURCES = \
-	openvpnserv.c \
-	service.h service.c \
+        common.c \
+	automatic.c \
+	interactive.c \
+	service.c service.h \
 	openvpnserv_resources.rc

1.9.1

Part of the diff is not shown here. View the raw message.

Hi,

so, this took quite a while - barely 2.5 years, but we're getting there. v3 of the patch has been rebased to git master, and all comments from Steffan and my earlier review have been integrated into the build stuff and the openvpn side of the code - so, as far as openvpn goes, I'm fine with merging that but would welcome an independent ACK (given that I modified quite a bit of Heiko's code).

The service part has been *tested* - as in:

- compiles (mingw)
- runs on Win7 (openvpnserv -remove, copy in new binary, run "openvpnvserv -install", "openvpnserv -start interactive" [or reboot])
- does what it says on the tin:
- run openvpn.exe as the user executing the GUI
- handles adding and removing of ipv6 address config and v4/v6 routing
- enables use of openvpn gui without [X] admin checkbox as a totally unprivileged user
- openvpn log makes it clear whether netsh.exe is used or service

what I have not done is a full review of the resulting code - the changes are large and intrusive, and given the amount of code *removal* it looks like "massive cleanup" happened as well. I do not know Windows well enough to understand the intricacies, so a review from someone with a stronger Windows background would be welcome - Selva, are you still around?

There are some caveats that need to be tested better in combination with a full reinstall, like "where does the openvpn log go to, and is the destination writeable?" - that seems to relate to registry entries that my system did not have, so there will be extra work for Samuli and Heiko as well...

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

When this code gets into Git "master" the Windows buildslave will start producing installers with this functionality. We can then point people at those. I suspect enterprises in particular are interested in the this functionality so I think we'll get a fair amount of testing before we even reach 2.4.0.

Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

One question, primarily to Heiko... does the interactive service solve the use-case where the administrator/user wants to have persistent connections that come up on boot and are not closed or managed in any way in the meantime? In this use-case no user interaction is wanted, needed or expected.

Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

Hi,

As far as I understand, the interactive service is fully controlled by the GUI ("hey service, run *this* with my credentials, but permit it to talk to you and request route changes").

The "openvpnserv.exe" process can now actually run as two different things - the classic "openvpn service" which just starts openvpn.exe at service start time, and the new "interactive service" which helps the GUI for, well, "interactive" openvpn usage.

Since Heiko has reworked the "classic" code (in automatic.c now, as he called this "automatic service") maybe we can just use the opportunity to get the remaining issues fixed - is there more than "restart openvpn.exe when it dies"?

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Haven't look at the new code, but of if the "automatic service" logic is the same as before, it would take some work to make it behave like a watchdog of openvpn.exe processes. Using NSSM for "automatic" uses and this one only for interactive start ups by the GUI may be a better option. NSSM has the added advantage of one service per config which could be independently stopped and restarted etc.

The original service starts all the configs and then sits there waiting for the exit event to trigger, at which point the service will just stop. For one, the exit event will not trigger when openvpn.exe quits and even if we make the event to trigger, there is only one common event for multiple configs, and the service just stopping at such an event serves no purpose.

Instead of fixing it why not just use NSSM? Then the interactive service patch could be decoupled from openvpnserv and provided as a new feature independent of the original service.

Selva

Il 27/01/2016 00:54, Selva Nair ha scritto:

There actually is a drop-in replacement for the old openvpnserv.exe here:

<https://github.com/mattock/openvpnserv2>

It was announced here:

<https://community.openvpn.net/openvpn/ticket/595>

From now on, I will call it openvpnserv2 to distinguish it from the old crappy openvpnserv.exe and from the interactive service.

I would personally prefer openvpnserv2 over using glue and duct-tape around NSSM to make it's configuration frontend suck less. Openvpnserv2 can already restart dead connections, e.g. after the computer has resumed from suspend or hibernation. I did some very basic testing on a Windows 7 laptop and confirmed suspend/resume worked fine. Of course we'd need more extensive tests with test installers before releasing it to a wider audience.

An added bonus is that openvpnserv2 is written in C#, which means it can be developed on Linux using Mono, and the language choice probably helps getting new contributions from people not fluent with C.

Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

Hi,

I'm not totally convinced that "mixing in a new language" is a *bonus* (as it means that the core team won't be able to help unless also fluent with the other language).

Given that services run with maximum privileges, strong review is as important there as for core openvpn...

If the only reason why everyone is disliking the old openvpnserv is "it is not restarting openvpn.exe when it breaks" - *that* should be fairly easy to add. So, what is that people consider "broken"?

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Well, I just reviewed all of the code with no difficulty, so would I have no trouble co-maintaining it. Openvpnserv2 is built on top of generic Service C# classes provided by Microsoft and overrides a few methods such as OnStart, OnStop, and sets some some capabilities based on what OpenVPN processes can (be ordered to) do. For an overview of the ServiceBase class look here:

<https://msdn.microsoft.com/en-us/library/system.serviceprocess.servicebase%28v=vs.110%29.aspx>

The core of openvpnserv2 is in this file:

<https://github.com/xkjyeah/openvpnserv2/blob/master/Service.cs>

The remaining files are mostly stuff added by the IDE. The only thing that openvpnserv2 really adds on top of generic MS service classes is:

- loading of OpenVPN registry keys
- building the command-line(s) to launch the connection(s)
- monitoring the individual OpenVPN processes and restart them if they crash
- stopping the processes before suspend
- starting the processes on resume

That said, the code does not seem to handle 32/64-bit registry keys yet. This needs to be fixed, unless there is some underlying magic in there I missed. Adding support for selecting which connections to launch automatically would also be nice, e.g. via registry or a config file.

I would not be worried about current core developers not being able to help with openvpnserv2. First, they haven't really been able to help with openvpnserv.exe, either. If they had, we wouldn't be having this discussion in the first place. I would be inclined to think that instead of overloading the existing C maintainers we should try to get _new_ people to maintain the Windows service part.

I don't see a problem with moving to C# for a number of reasons:

- The codebase pretty small
- Most of the hard lifting is done in the classes maintained by MS
- There is already a maintainer for this code
- There is a volunteering co-maintainer (=me) in the core group
- C# is easier than C to understand/develop (imho), if you already know a high-level language (especially Java, but also Python etc.)

Yes, agreed. But note that most of the functionality is stock functionality provided by Microsoft in their libraries, so there's not much to review. As said, I reviewed the code already, but of course it does not hurt if an experienced C# developer has a second look.

One thing to bear in mind is that making horrible mistakes (e.g. create a gaping security hole) with C# is more difficult than with C, so while review is of course necessary, every single line does not have to picked apart for potential vulnerabilities.

It does not work at all in Windows 10 afaik, and only barely works in Windows 8.1. It does not handle suspend/resume. It is not maintained, and there are no maintainers, and I don't see any maintainers appearing. There are a bunch of tickets in Trac related to openvpnserv.exe, directly or indirectly. Interestingly openvpnserv.exe has been dropped from the OpenVPN Chocolatey package[1], apparently because it's fairly useless nowadays.

Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock


[1] <https://chocolatey.org/packages/openvpn>

Hi,

This is actually a fairly strong (I would even say "compelling") argument for the C# rewrite. "Working and reliably supported API" is a killer :-)

("Having to install .NET runtime" might not be such a big issue nowadays, as most of the machines should have it already)

[..]

In case we stay with openvpnserv2, I think some followup questions need to be asked...

- do we want to migrate "interactive service" functionality into openvpnserv2 as well? More code, but not overly complex stuff (if the network/route API in C# isn't totally different from C) - and all the arguments from above apply to the interactive service as well, of course

- should "the service(s)" live inside the main openvpn repo, in the GUI repo, in their own repo, ...?

- what code change rules apply? I tend to opt for "openvpn main repo" rules, that is, no change goes in without review & ACK - a bad or malicious commit to the service could compromise the system security about as badly as a bad commit inside openvpn. I am aware that this is a totally annoying process, especially if the code author has to wait for months for an ACK to a trivial change - but hopefully a somewhat larger active developer base can improve on this.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

(forgot to CC the mailing list earlier)

Interesting question that warrants more discussion. Other implementations (NSSM, Openvpnwinserv) implement this by creating multiple services. Personally I prefer a single service managing multiple instances (in the style of IIS, Windows file sharing, etc.). Firstly, backward compatibility. Secondly, services should only be interactive when something exceptional happens.

But to implement anything of that sort, the question is how we could implement interprocess communication? Do we watch registry values? Open some port? Security issues?

Definitely main repo. But I'm not in a position to decide.

Currently the service is a part of the openvpn repo, but there is no reason to keep it thus. In fact, once the interactive service is available, in my view, it may not be even necessary to ship the original service with the windows binary distribution. Only advanced users would need the original service or its equivalent (openvpnserv2 or NSSM) usage of which could be made available as documentation in the wiki pages.

Although it is called a service, it only works like a one time task or an rc script. However, unlike a startup script, it does not terminate after spawning openvpn.exe processes, giving it the appearance of a service. It does not keep track of the spawned processes, has no way of knowing any of the processes stopped or crashed, has no way of editing one config and then restart only that connection leaving others up etc.

Having said that, on the only windows 10 machine I have, the original service works the same way as on Windows 7. Early reports of "not working on windows 10" might have had more to do with unrelated TAP6 issues.

Selva

This would be a novel idea. The service component has very few ties to OpenVPN, and is a very simple piece of software. I don't think it _needs_ to be in the main OpenVPN repo, either. Splitting the project into smaller chunks both code- and organization-vise has been beneficial in the past by allowing us to get more people onboard. Plus we've been able to optimize the development processes per subproject, so that the strict patch review regime in the main project has not slowed down the development of less critical components.

I have no idea how large a percentage of our users use the service. Probably the only way to _really_ find that out would be to deselect the service in the installer by default and see how loudly people scream. We could have a poll, but in my experience only a few people would answer, and we wouldn't be any better off. If most people would be basically ok without the service, we could still provide it as a separate download on the official download pages. That said, I have no strong opinions one way or the other.

I believe openvpnserv2 - or rather the more generic MS code upon which it builds - can monitor the individual processes and restart them as necessary. So it definitely a more complete solution than what openvpnserv.exe provides.

Quite likely. Also some issues seem to have been related to specific problematic Windows 10 builds.

Samuli

Hi

Sorry was too busy so far to catch up reading with this thread.

As Gert put it correctly, the interactive service is just an addon service for the interactive use case (currently a GUI). However if I understood you guys correctly the main feature of the other new service (v2) is that it works with Windows 10 and that you can select which connections you start/stop.

Well that same thing could be achieved with the interactive service. All you'd need is a little helper program that starts/stops a connection talking to the interactive service. Mainly the parts of the GUI that tell the iservice to start a connection. The service itself is spawning a thread for every openvpn tunnel it is signaled to start.

I said it could be achieved, because the iservice currently doesn't handle restarts of openvpn. Instead it trusts the GUI to do so if it expects the connection to be up when a openvpn process disappears. One benefit would obviously be the shared code base and that openvpn could be run under an unprivileged account as well.

If it makes sense to extend the iservice to handle the automatic use case is a different thing, who will do it yet another one on top. Someone made the point that the automatic service is a corner use case, most ppl will not use it. I agree. Ppl. used it to overcome the routing issue since Vista, but it was never there for this use case anyway and the interactive service solves this problem.

So, having the "automatic"/"v2" service as a completely separate service or even project is fine with me. It just has some consequences that have been mentioned already. If you're all fine with these let's do it.

Heiko

Still alive, though things are a bit hectic at work. Its a fairly large chunk of code and I had only glanced through parts of it in the past. Will try to take a look.

Selva

Hi,

Cool. Was afraid you went silent after I neglected to follow up on your configure.ac patch in November (*sigh* - too busy myself)

I'll go and look at the "do network stuff" and "parse messages from the socket" bits - those are "just plain C stuff". Just the service and event handling eludes me...

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

That can wait -- may be I'll come up with version 2 on it some time..

Sure, will look through the service stuff.

Selva

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.

non-privileged

flush,

service

<n> is

be adapted

(snip)

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)

+++++++++++++++++++++++++++++++++++++++++

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

1) can be implemented by only accepting relative filenames for the --config parameter and appending it to the config_dir from the registry.
2) has been talked about but we decided that we go with what we have and implement improvements when the code has been integarted.

Currently the goal of the service is not so much to prevent a sophisticated person from setting routes. It's rather to enable ppl. to use openvpn without hacks, while providing a long term safe path of doing so.

Don't get it. Openvpn doesn't send or receive anything critical to/from the channel. It is just used to send requests for a set of predefined operations that need elevated privileges. If the passer of the channel doesn't have elevated rights as well it could fool openvpn to believe that the operation was successful while it wasn't. I don't think that's critical.

Yeah. The point is no to run the GUI as admin anymore in the first place. So we could just do (ii) and be sure that this is circumvented. This is only exploitable when a user can run the GUI as admin anyways, so there's no real need for the detour through service and openvpn. So, it's basically a safeguard for legacy installations who worked around the route issue by granting the GUI higher privileges. Good catch.

Yeah, I'll be in the meeting but probably (too) late.

Cheers
Heiko

As discussed in the IRC meeting, the critical issue was whether the GUI (running as admin) connecting to a rogue named pipe server could allow privilege escalation by that server process. I did some tests on this.

On Windows XP this is exploitable --- i.e., with a pipe server running as a limited user with the GUI started using runas admin. However, on Windows 7 and 10 attempts at elevation failed at least by the approach I imagined. And based on what I hear, Vista and above appear to be immune from this attack.

It may be better to make the GUI not connect to the service pipe if running as admin.

Thanks,

Selva

Hi,

Following up on that, a couple of comments below:

      HMODULE iphlpapi = GetModuleHandle (TEXT("iphlpapi.dll"));

Is this run-time lookup necessary?

Anyway, the iservice works well and the only real problem I ran into was lack of write access to the default log_dir. That leads to a log-jam as the stdout is now a pipe which the service is not actively reading from. Could be handled in the GUI + registry by making sure openvpn is started with a writable log file..

Anyway, the service part of the code looks good and does well what it promises to do.

Selva

Hi,

I don't think so - it might be a leftover of "we try to make this compatible with windows XP", which we decided later to be a non-goal for 2.4.

So we might replace this with just calling CreateIpForwardEntry2() directly.

Yes, I've seen this as well. As far as I understand, there's a registry key that can be changed to include the user's home in the log path - it's on a piece of paper on my desk, while I'm at work, so can't look right now... Heiko?

(And yes, this needs to be adjusted in the installer, whichever way we go - globally writeable log directory, or user-individual logging)

Thanks. I take this as an ACK from your for the service change, and Arne's mail as an ACK for the openvpn side, and merge it tonight.

Then we can go about improving things further (... while we wait for the other large 2.4 feature to fully show up, AEAD...)

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

Its the "HKLM\OpenVPN\log_dir" key set by the installer when the service is installed. For the automatic service, users may want it to stay at such a system-wide location, while the interactive service does not use it directly. But the UI (OpenVPNGUI) uses it to construct the log file path and passes it in the command line options.

To make the log location user-dependent, we could include env variables in the registry key -- say %UserProfile%/OpenVPN/log But that requires the service to read the key as REG_EXPAND_SZ and then expand it.

For now, I'll make a patch for the GUI to construct a writable log path when iservice is used. It looks ok to let the UI to decide where to write logs to. We could document that starting openvpn using iservice needs the --log or --log-append option with a writable log file. If its not writable openvpn will fallback to stdout which will not work.

Sounds good!

Selva

Yes, I think the same. I also briefly looked at the openvpn bits, and those look good to me too. I did not check the openvpnserv bits, but I think Selva has that covered (and more experience).

-Steffan

May I ask a more generic question about the implementation of the interactive service?

So I understand that the patch allows clients to instruct the service to modify the routing table. In short, it allows ordinary users (since any program could impersonate the openvpn client) to modify the routing table. Is this a good idea?

Imagine a rogue MITM client that somehow got onto a user's PC. It then uses the Interactive Service to divert all traffic to a C&C server. Now all Internet traffic can be eavesdropped upon.

Wouldn't it be better as debbie10t suggested to restrict ordinary users to only the VPN configurations that are permitted by the administrator?

Daniel

Hi,

Yes and no. It allows clients to instruct the service to run OpenVPN on a pre-installed config file - and the service will pass a pipe handle to OpenVPN on which it's willing to receive route change instructions.

Part of the discussion we had was to have a checkbox at installation time that restricts the service to only run .ovpn profiles from a well-defined and admin-only-writeable directory (and also change the argument handling between gui and service to refuse to accept arbitrary command line arguments).

Not by impersonating the openvpn client, but by running a "rogue profile" via true openvpn - but for your c&c example, you need more than "just routing" anyway.

[..]

Which is the plan.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

Yes, thanks. That's the key...

This sounds like a good plan, thanks.

Yes, this came as a bit of surprise to me - I saw quite a bit of logging in the status window, and then "it just stuck" (eventually I noticed the error about the non-writeable log file in the first line, and tried making it writeable :-) ).

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

I have no idea what this does. The docu tells me that it is obsolescent.

Other than I looked at other code that is modfied in OpenVPN itself. I have no idea about all the windows specific stuff. But from the look it looks good and the other parts it touches are good. So a cautinary ACK from me.

Arne

Your patch has been applied to the master branch.

I have taken out the "AM_PROG_CC_C_O" change to configure.ac as the autoconf docs tell me that AC_PROG_CC will automatically check what this used to do, starting with 1.14 - and *that* one seems to be auto-invoked as well... (and I do not see us compiling on a system where the compiler cannot handle -c and -o at the same time any time soon anyway):

checking for gcc... gcc
checking whether the C compiler works... yes
...
checking whether gcc understands -c and -o together... yes
...

Next steps before this is ready for 2.4:

- GUI: do not use the service pipe if running with admin privs
- GUI+interactive service: change calling convention to ensure only whitelisted openvpn.exe options can be set up, including path checking to .ovpn files
- GUI/installer/service: introduce a way to specify whether config files are restricted to "only from admin-writeable openvpn\config\ directory, or anywhere in the system" (however that might look in the end)
- GUI: log directory if program files\openvpn\log\ isn't user-writeable
- openvpn/interactive service: handle --block-outside-dns

commit a24dd2e31f196c76594666f37c130817402acb15 (master)

Author: Heiko Hund
Date:   Tue Jan 26 20:11:48 2016 +0100

     interactive service v3

     Signed-off-by: Heiko Hund <[email protected]...>
     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>        (Service changes)
     Acked-by: Arne Schwabe <[email protected]...>          (OpenVPN changes)
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11027

--
kind regards,

Gert Doering