Restrict options/configs for startup through interactive service

Hi,

The pacth is in the next email.

This is not yet tested extensively, but has passed some quick tests.
A simple approach of parsing the options string is used instead of passing structs:
the latter will break the GUI everytime a new option is added to the white-list.

Current white-list is just what the GUI needs, but its easy to extend.

- Only limited tests could be done with the stock GUI:

- Because of GUI limitations, check the event viewer for startup errors (the GUI will just hang for 15 seconds and say connection failed)

Thanks,

Selva

Windows only:

- Allow only a set of whitelisted options in the command line options passed by interactive service clients unless
(i) user is the local Adminsitrator group
AND/OR
(ii) in a predefined group (see below)
Only the group membership is checked, the client process need not be running with any elevated privileges available to those groups.

- Restrict config files to config_dir or it sub directories unless (i) and/or (ii) above is truei. (config_dir is as defined in HKLM\Software\OpenVPN\config_dir)

- The predefined group may be set in the registry HKLM\Software\OpenVPN\ovpn_admin_group (default: "OpenVPN Administrators")

- The white-list of options is a simple flat array of option strings (without leading --) defined in validate.c

- Further options may be added to the whitelist without breaking the GUI -- the startup data is passed from the GUI to the service the same way as before.

Notes to GUI developers:
(i) If the user is an administrator, the service will grant all privileges even if the GUI is not running elevated. This is practically equivalent to 'highestAvailable' without the risks of running the GUI elevated.

(ii) If the option checks fail, openvpn is not started, but an error message is passed back to the service pipe and written to event log. Currently the GUI does not read from the service pipe -- this needs fixing.

---
 src/openvpnserv/Makefile.am   |    3 +-
 src/openvpnserv/common.c      |   13 ++-
 src/openvpnserv/interactive.c |   95 +++++++++++++++++++
 src/openvpnserv/service.h     |    2 +
 src/openvpnserv/validate.c    |  207 +++++++++++++++++++++++++++++++++++++++++
 src/openvpnserv/validate.h    |   49 ++++++++++
 6 files changed, 365 insertions(+), 4 deletions(-)
 create mode 100644 src/openvpnserv/validate.c
 create mode 100644 src/openvpnserv/validate.h

index 4bb1c27..5aba53a 100644
@@ -26,7 +26,7 @@ openvpnserv_CFLAGS = \
 	-municode -D_UNICODE \
 	-UNTDDI_VERSION -U_WIN32_WINNT \
 	-D_WIN32_WINNT=_WIN32_WINNT_VISTA
-openvpnserv_LDADD = -ladvapi32 -luserenv -liphlpapi -lws2_32
+openvpnserv_LDADD = -ladvapi32 -luserenv -liphlpapi -lshlwapi -lnetapi32 -lws2_32
 endif
 
 openvpnserv_SOURCES = \

@@ -34,4 +34,5 @@ openvpnserv_SOURCES = \
 	automatic.c \
 	interactive.c \
 	service.c service.h \
+        validate.c validate.h \
 	openvpnserv_resources.rc

index a293796..dba4724 100644
@@ -23,7 +23,7 @@
  */
 
 #include <service.h>
-
+#include <validate.h>
 /*
  * These are necessary due to certain buggy implementations of (v)snprintf,
  * that don't guarantee null termination for size > 0.

@@ -53,7 +53,6 @@ openvpn_sntprintf (LPTSTR str, size_t size, LPCTSTR format, ...)
   return len;
 }
 
-
 #define REG_KEY  TEXT("SOFTWARE\\" PACKAGE_NAME)
 
 static DWORD

@@ -114,6 +113,13 @@ GetOpenvpnSettings (settings_t *s)
   if (error != ERROR_SUCCESS)
     goto out;
 
+  /* read if present, else use default */
+  error = GetRegString (key, TEXT("ovpn_admin_group"), s->ovpn_admin_group, sizeof (s->ovpn_admin_group));
+  if (error != ERROR_SUCCESS)
+  {
+    openvpn_sntprintf(s->ovpn_admin_group, _countof(s->ovpn_admin_group), OVPN_ADMIN_GROUP);
+    error = 0; /* this error is not fatal */
+  }
   /* set process priority */
   if (!_tcsicmp (priority, TEXT("IDLE_PRIORITY_CLASS")))
     s->priority = IDLE_PRIORITY_CLASS;

@@ -194,7 +200,8 @@ MsgToEventLog (DWORD flags, LPCTSTR format, ...)
   if (hEventSource != NULL)
     {
       openvpn_sntprintf (msg[0], _countof (msg[0]),
-                         TEXT("%s error: %s"), APPNAME, err_msg);
+                         TEXT("%s%s: %s"), APPNAME,
+                         (flags & MSG_FLAGS_ERROR) ? TEXT(" error") : TEXT(""), err_msg);
 
       va_start (arglist, format);
       openvpn_vsntprintf (msg[1], _countof (msg[1]), format, arglist);

index 5249b31..94bfb07 100644
@@ -61,11 +61,13 @@ typedef struct {
   DWORD start_type;
 } openvpn_service_t;
 
+#define MAX_NAME 256
 typedef struct {
   TCHAR exe_path[MAX_PATH];
   TCHAR config_dir[MAX_PATH];
   TCHAR ext_string[16];
   TCHAR log_dir[MAX_PATH];
+  TCHAR ovpn_admin_group[MAX_NAME];
   DWORD priority;
   BOOL append;
 } settings_t;

1.7.10.4

Hi,

Re-thinking the original argument, I agree that this is easier than a fixed structure (if only because you'd need to ensure that GUI and service are talking the same structure version...).

I like what your patch is doing - it's more than "just the whitelist" but also the administrative restrictions ("if you are not administrator or part of the OpenVPN Admin group, only configs from a well-known directory, otherwise, do what you want").

Hi,

Thanks for the quick feedback.

And the admin (or at install time) could even change the "wheel" group name to "Users" and bless every user too.

String conversions are indeed a pain. Could eliminate them if the code is guaranteed to be compiled with -municode -D_UNICODE. Does anyone build a non-unicode version on windows these days?

The whole of the service code will then need only one string conversion -- for passing the management password to stdin of openvpn.

Selva

Windows only:

- Allow only a set of whitelisted options in the command line options passed by interactive service clients unless
(i) user is the local Adminsitrator group
AND/OR
(ii) in a predefined group (see below)
Only the group membership is checked, the client process need not be running with any elevated privileges available to those groups.

- Restrict config files to config_dir or it sub directories unless (i) and/or (ii) above is true (config_dir is as defined in HKLM\Software\OpenVPN\config_dir)

- The predefined group may be set in the registry HKLM\Software\OpenVPN\ovpn_admin_group (default: "OpenVPN Administrators")

- The white-list of options is a simple flat array of option strings (without leading --) defined in validate.c

- Further options may be added to the whitelist without breaking the GUI -- the startup data is passed from the GUI to the service the same way as before.

Notes to GUI developers:
(i) If the user is an administrator, the service will grant all privileges even if the GUI is not running elevated. This is practically equivalent to 'highestAvailable' without the risks of running the GUI elevated.

(ii) If the option checks fail, openvpn is not started, but an error message is passed back to the service pipe and written to event log. Currently the GUI does not read from the service pipe -- this needs fixing.

v2 changes:
- checked non-unicode build and fixed an error -- in case anyone builds non-unicode
- added an info message to event log when user auth succeeds

---
 src/openvpnserv/Makefile.am   |    3 +-
 src/openvpnserv/common.c      |   13 ++-
 src/openvpnserv/interactive.c |   96 +++++++++++++++++++
 src/openvpnserv/service.h     |    2 +
 src/openvpnserv/validate.c    |  208 +++++++++++++++++++++++++++++++++++++++++
 src/openvpnserv/validate.h    |   49 ++++++++++
 6 files changed, 367 insertions(+), 4 deletions(-)
 create mode 100644 src/openvpnserv/validate.c
 create mode 100644 src/openvpnserv/validate.h

index 4bb1c27..5aba53a 100644
@@ -26,7 +26,7 @@ openvpnserv_CFLAGS = \
 	-municode -D_UNICODE \
 	-UNTDDI_VERSION -U_WIN32_WINNT \
 	-D_WIN32_WINNT=_WIN32_WINNT_VISTA
-openvpnserv_LDADD = -ladvapi32 -luserenv -liphlpapi -lws2_32
+openvpnserv_LDADD = -ladvapi32 -luserenv -liphlpapi -lshlwapi -lnetapi32 -lws2_32
 endif
 
 openvpnserv_SOURCES = \

@@ -34,4 +34,5 @@ openvpnserv_SOURCES = \
 	automatic.c \
 	interactive.c \
 	service.c service.h \
+        validate.c validate.h \
 	openvpnserv_resources.rc

index a293796..dba4724 100644
@@ -23,7 +23,7 @@
  */
 
 #include <service.h>
-
+#include <validate.h>
 /*
  * These are necessary due to certain buggy implementations of (v)snprintf,
  * that don't guarantee null termination for size > 0.

@@ -53,7 +53,6 @@ openvpn_sntprintf (LPTSTR str, size_t size, LPCTSTR format, ...)
   return len;
 }
 
-
 #define REG_KEY  TEXT("SOFTWARE\\" PACKAGE_NAME)
 
 static DWORD

@@ -114,6 +113,13 @@ GetOpenvpnSettings (settings_t *s)
   if (error != ERROR_SUCCESS)
     goto out;
 
+  /* read if present, else use default */
+  error = GetRegString (key, TEXT("ovpn_admin_group"), s->ovpn_admin_group, sizeof (s->ovpn_admin_group));
+  if (error != ERROR_SUCCESS)
+  {
+    openvpn_sntprintf(s->ovpn_admin_group, _countof(s->ovpn_admin_group), OVPN_ADMIN_GROUP);
+    error = 0; /* this error is not fatal */
+  }
   /* set process priority */
   if (!_tcsicmp (priority, TEXT("IDLE_PRIORITY_CLASS")))
     s->priority = IDLE_PRIORITY_CLASS;

@@ -194,7 +200,8 @@ MsgToEventLog (DWORD flags, LPCTSTR format, ...)
   if (hEventSource != NULL)
     {
       openvpn_sntprintf (msg[0], _countof (msg[0]),
-                         TEXT("%s error: %s"), APPNAME, err_msg);
+                         TEXT("%s%s: %s"), APPNAME,
+                         (flags & MSG_FLAGS_ERROR) ? TEXT(" error") : TEXT(""), err_msg);
 
       va_start (arglist, format);
       openvpn_vsntprintf (msg[1], _countof (msg[1]), format, arglist);

index 5249b31..94bfb07 100644
@@ -61,11 +61,13 @@ typedef struct {
   DWORD start_type;
 } openvpn_service_t;
 
+#define MAX_NAME 256
 typedef struct {
   TCHAR exe_path[MAX_PATH];
   TCHAR config_dir[MAX_PATH];
   TCHAR ext_string[16];
   TCHAR log_dir[MAX_PATH];
+  TCHAR ovpn_admin_group[MAX_NAME];
   DWORD priority;
   BOOL append;
 } settings_t;

1.7.10.4

ACK. Needed functionality, and the implementation looks good to me (I wouldn't mind more closed scrutiny by one of the more experienced windows programmers, but since they are all too busy, my reading of the code will have to do). I fixed one typo ("Retrurns") in a comment.

Your patch has been applied to the master branch.

commit f3c8a04d60216d532f239c951a4694d7c1d5eaa8

Author: Selva Nair
Date:   Fri Feb 19 22:13:08 2016 -0500

     Restrict options/configs for startup through interactive service

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11225
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

Mmmh. It breaks compilation on Samuli's buildbot...

/bin/bash ../../libtool --tag=CC --mode=link i686-w64-mingw32-gcc -municode -D_UNICODE -UNTDDI_VERSION -U_WIN32_WINNT -D_WIN32_WINNT=_WIN32_WINNT_VISTA -o openvpnserv.exe openvpnserv-common.o openvpnserv-automatic.o openvpnserv-interactive.o openvpnserv-service.o openvpnserv-validate.o openvpnserv_resources.o -ladvapi32 -luserenv -liphlpapi -lshlwapi -lnetapi32 -lws2_32
libtool: link: i686-w64-mingw32-gcc -municode -D_UNICODE -UNTDDI_VERSION -U_WIN32_WINNT -D_WIN32_WINNT=_WIN32_WINNT_VISTA -o .libs/openvpnserv.exe openvpnserv-common.o openvpnserv-automatic.o openvpnserv-interactive.o openvpnserv-service.o openvpnserv-validate.o openvpnserv_resources.o -ladvapi32 -luserenv -liphlpapi -lshlwapi -lnetapi32 -lws2_32 openvpnserv-validate.o:validate.c:(.text+0x54): undefined reference to `PathIsRelativeW'
openvpnserv-validate.o:validate.c:(.text+0x31f): undefined reference to `NetApiBufferFree'

the microsoft documentation says, PathIsRelativeW() is in "Shlwapi.dll", and I can see the link line pulling that in... "NetApiBufferFree()" is in "Netapi32.dll", and that is *also* linked...

I'm a bit at a loss why it is bombing here...

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]...

MinGW fails linking after f3c8a04d6021 if the right header files (<lm.h> and <shlwapi.h>) are not included.

---
 src/openvpnserv/validate.c | 2 ++
 1 file changed, 2 insertions(+)

index d7446de..b5809b3 100644
@@ -25,6 +25,8 @@
 #include "validate.h"
 
 #include <lmaccess.h>
+#include <shlwapi.h>
+#include <lm.h>
 
 static const WCHAR *white_list[] =
     {

1.9.1

Hi,

ACK

Thanks for cleaning up my sloppiness..

Selva

Patch has been applied to the master branch.

commit 9b9187031b742258b518dbde648326b3e3a8d8d8

Author: Gert Doering
Date:   Thu Feb 25 15:58:32 2016 +0100

     Fix openserv/validate.o linking issues on mingw.

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11255

--
kind regards,

Gert Doering

Ah, sorry I should have checked 32 bit compilation. Its due to missing includes, not a big problem for 64 bit as dll import names are not mangled, but in 32 bit linker may be looking for _PathIsRelativeW etc.. (I think..) Anyway my mistake.. Will do a patch..

Selva

Hi,

Ah! Learned something new today.

(Unixy systems usually only break on missing includes if functions are not real "functions" but macros instead...)

Thanks for the quick explanation and ACK, buildbot should be happy again now - and I can push stuff again without being hit by build failure complaints :-)

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]...