Re: [Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[Jonathan K. Bullard]

On Wed, Jul 18, 2012 at 10:10 AM, David Sommerseth <
openvpn.l...@topphemmelig.net> wrote:

> * The computer is configured to allow OpenVPN to run without root
>   password
>

Yes. The vulnerability requires configuring the computer to allow *the
user*to start OpenVPN
*as root* without entering the root password.

However, since configuration file paths and script paths can also be
arbitrary paths, they can be used to escalate privileges in this computer
configuration, too. (The configuration file could contain an "up" to an
attack script, or an attack script could be used as the argument to --up.)

So "closing" this plugin vulnerability isn't much help because there are
other similar attack vectors.

If at some time OpenVPN is modified to disallow absolute paths to scripts
and/or the configuration file, that would be a major problem for
Tunnelblick for the reasons outlined earlier. It's one thing to not be able
to use down-root, especially if it becomes unnecessary, but OpenVPN can't
be used at all without scripts and configuration files!

Instead of disallowing absolute paths, how about verifying at execution
time that the plugin (or configuration file, or script) cannot be modified
except by a privileged user? That is, instead of restricting the location
of script/plugin/configuration files to an arbitrary protected place or
places, instead making sure that the files themselves are protected.

(Note: Tunnelblick doesn't contain the vulnerability because it doesn't
allow the user to start OpenVPN as root. Instead, a suid "helper" program
is used to launch OpenVPN as root. That helper program controls the options
(including the path to the configuration file and scripts) that OpenVPN is
started with, and the configuration file and scripts cannot be modified by
an unprivileged user.)

Re: [Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 18/07/12 14:44, Jonathan K. Bullard wrote:
> On Tue, Jun 26, 2012 at 1:05 PM, Alon Bar-Lev
> > wrote:
> 
> Currently openvpn requires/endorses specifying full path in plugin 
> parameter. As build system already aware of plugin location, it is 
> possible to load plugin relative to this directory, so full path is
> not required nor more secured.
> 
> Windows is a little more complex as user may change installation 
> directory at install time, so we read plugindir location from the 
> registry, installer will set this value.
> 
> 
> (Sorry Alon, I must have overlooked this when it was first
> posted.)
> 
> I do not understand the privilege escalation risk:
> 
> * A protected (not writable by an unprivileged user) part of 
> Tunnelblick controls the options sent to any instance of OpenVPN 
> that runs as a privileged user, so it sends only acceptable paths. 
> (A computer administrator is the only one who can modify a 
> configuration file, which is the other means of specifying a
> plugin path.) * If a user runs an instance of OpenVPN from the
> OpenVPN binary inside of Tunnelblick, it runs as the user. So it's
> not very different form running it from the command line.
> 
> Aren't other installations of OpenVPN similarly protected? Or are
> you trying to protect against poorly-protected installations?
> 
> Or can the server "push" a plugin path? If allowed (I don't think
> it should be) that is a much bigger problem.

I was about to write about how this can slightly enhance the security
on a system, avoiding random plug-ins to abuse the plug-in interface
to gain control over a system ... until I realised, we already ship
code which can do that.  All you need to do is to have
openvpn-down-root.so available and a shell script which it can use.

So the attack vector is:

* A unprivileged user writes a little script which requires root access
* The computer is configured to allow OpenVPN to run without root
  password
* Configuration file is modified (or command line executing openvpn is
  extended) to add --plugin openvpn-down-root.so with a pointer to his
  script
* This external plug-in openvpn_plugin_open_v*() function in
  openvpn-down-root.so  will be executed with root privileges when
  openvpn is started.
* OpenVPN is closed
* Script runs with root privileges

What the down-root plug-in does, is to fork out a process while
openvpn is still running as root, and wait until the OpenVPN is
closing down loaded plug-ins.  At this point the code script will be
run, with root privileges.  Which makes it a perfectly good jail-break
approach.

This is possible as OpenVPN normally needs root privileges to
configure the network, like TUN/TAP adapter and modifying the routing
table.

A I see to avoid this is to completely run OpenVPN unprivileged and
use OS dependent APIs.

 - Linux:
   NetworkManager has a DBUS interface useful to configure the network,
   or you can use sudo together with iproute2 wrapper scripts.

 - Android have a special API which must be used to get access to a
   tun device and to configure the network

 - A Windows solution is in the works, which tries to provide a
   privilege separation possibility

But I don't know what possibilities *BSDs or OSX may provide, except
the sudo approach.

With an improved privilege separation solutions in place, the need for
the down-root plugin goes away too, which often is used to restore
/etc/resolv.conf changes on *nix platforms.


Okay, enough ramblings for now ...


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlAGw9MACgkQDC186MBRfrrTGwCeNvIZ7+5ICsiM++JuAhzaC+Fg
jwoAn2HNMoPPNcrumiOci+MN21FwnWMo
=Znxo
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[Jonathan K. Bullard]

On Wed, Jul 18, 2012 at 9:37 AM, Alon Bar-Lev  wrote:

> Nobody disables the absolute path use.
> This patch permits relative use.
>

I'm sorry, I misunderstood. So a relative path will now be interpreted as
relative to the plugins directory specified a build time, rather than
whatever it is relative to now (current working directory?)

So please consider my response to have been responding to Heiko's post:

On Wed, Jul 18, 2012 at 7:44 AM, Heiko Hund  wrote:

> Specifying a custom full path is probably something we need to ban in the
> (near) future, as it imposes an attack vector for privilege escalation by
> code
> injection when openvpn is not running as another user or has access to
> privilege escalation by other means (like the yet to be submitted Windows
> interactive service).
>

Re: [Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[Alon Bar-Lev]

Nobody disables the absolute path use.
This patch permits relative use.

On Wed, Jul 18, 2012 at 3:44 PM, Jonathan K. Bullard
 wrote:
> On Tue, Jun 26, 2012 at 1:05 PM, Alon Bar-Lev  wrote:
>>
>> Currently openvpn requires/endorses specifying full path in plugin
>> parameter. As build system already aware of plugin location, it is
>> possible to load plugin relative to this directory, so full path is not
>> required nor more secured.
>>
>> Windows is a little more complex as user may change installation
>> directory at install time, so we read plugindir location from the
>> registry, installer will set this value.
>
>
> (Sorry Alon, I must have overlooked this when it was first posted.)
>
> I do not understand the privilege escalation risk:
>
> A protected (not writable by an unprivileged user) part of Tunnelblick
> controls the options sent to any instance of OpenVPN that runs as a
> privileged user, so it sends only acceptable paths. (A computer
> administrator is the only one who can modify a configuration file, which is
> the other means of specifying a plugin path.)
> If a user runs an instance of OpenVPN from the OpenVPN binary inside of
> Tunnelblick, it runs as the user. So it's not very different form running it
> from the command line.
>
> Aren't other installations of OpenVPN similarly protected? Or are you trying
> to protect against poorly-protected installations?
>
> Or can the server "push" a plugin path? If allowed (I don't think it should
> be) that is a much bigger problem.
>
> Disallowing absolute paths complicates things enormously for Tunnelblick
> (OpenVPN GUI for OS X) for two reasons:
>
> You can have multiple, different copies of Tunnelblick (it is
> self-contained), and
> Each copy of Tunnelblick contains multiple versions of OpenVPN, each with
> its own copy of the down-root plugin.
>
> Currently, Tunnelblick just sends a plugin path which points to the
> appropriate plugin file inside the application itself (an OS X application
> like Tunnelblick is basically a folder with stuff inside it).
>
> If absolute paths are not allowed, Tunnelblick will apparently have to start
> "installing" plugins in a system-wide directory, and track separate copies
> of the down-root plugin by their source (i.e., the specific copy of
> Tunnelblick, and the specific version of OpenVPN the down-root plugin comes
> from. It would also be impossible to deal with a user moving the Tunnelblick
> application somewhere else in the file hierarchy (Most OS X applications,
> including Tunnelblick can be moved around as the user pleases.)
>
> That seems like so much trouble (and has a risk of creating a DLL-hell
> equivalent) that I will probably drop support for the down-root plugin from
> Tunnelblick. I don't think that's a big problem, but it will be for some
> users, and they won't be able to script their way around it.
>

Re: [Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[Alon Bar-Lev]

On Wed, Jul 18, 2012 at 4:34 PM, Alon Bar-Lev  wrote:
> Hi!
>
> On Wed, Jul 18, 2012 at 2:44 PM, Heiko Hund  wrote:
>> Hi Alon
>>
>> On Tuesday 26 June 2012 20:05:02 Alon Bar-Lev wrote:
>>> Currently openvpn requires/endorses specifying full path in plugin
>>> parameter.
>>
>> Specifying a custom full path is probably something we need to ban in the
>> (near) future, as it imposes an attack vector for privilege escalation by 
>> code
>> injection when openvpn is not running as another user or has access to
>> privilege escalation by other means (like the yet to be submitted Windows
>> interactive service).
>>
>> This patch is a step towards this so I think it's absolutely worth a feature
>> ACK.
>>
>>>  AM_CFLAGS = \
>>> + -DOPENVPN_PLUGINDIR="\"$(plugindir)\"" \
>>
>> Couldn't OPENVPN_PLUGINDIR be an AC_DEFINE in config.h instead? Would keep 
>> the
>> command line shorter and more readable.

No... autoconf has issues with *dir during its run, automake is the
one that actually resolve the directories into something usable.

>>
>>> + if (ExpandEnvironmentStringsW(plugindir, _plugindir_expanded,
>> SIZE(_plugindir_expanded)) != 0)
>>
>> There's _countof defined in the Windows headers that could replace the 
>> non-std
>> SIZE here. I had to look SIZE() up. It does the right thing, though.

Oh... I just wanted to be consistent with openvpn styles.

>>> +  _snwprintf(_abs_so_pathname, SIZE(_abs_so_pathname), L"%s%C%s",
>>> plugindir, OS_SPECIFIC_DIRSEP, wide_string(p->so_pathname, ));
>>
>> You need to make sure _abs_so_pathname is zero terminated here. However, I
>> found that _snwprintf does not return a negative value when the buffer is too
>> small. Contrary to http://msdn.microsoft.com/en-us/library/2ts7cx93.aspx
>> So, this could need more research to get right.
>
> Thank you so much!
> I did not know sn*printf of MS are not C99 complaint!
>
> Alon.

Re: [Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[Alon Bar-Lev]

Hi!

On Wed, Jul 18, 2012 at 2:44 PM, Heiko Hund  wrote:
> Hi Alon
>
> On Tuesday 26 June 2012 20:05:02 Alon Bar-Lev wrote:
>> Currently openvpn requires/endorses specifying full path in plugin
>> parameter.
>
> Specifying a custom full path is probably something we need to ban in the
> (near) future, as it imposes an attack vector for privilege escalation by code
> injection when openvpn is not running as another user or has access to
> privilege escalation by other means (like the yet to be submitted Windows
> interactive service).
>
> This patch is a step towards this so I think it's absolutely worth a feature
> ACK.
>
>>  AM_CFLAGS = \
>> + -DOPENVPN_PLUGINDIR="\"$(plugindir)\"" \
>
> Couldn't OPENVPN_PLUGINDIR be an AC_DEFINE in config.h instead? Would keep the
> command line shorter and more readable.
>
>> + if (ExpandEnvironmentStringsW(plugindir, _plugindir_expanded,
> SIZE(_plugindir_expanded)) != 0)
>
> There's _countof defined in the Windows headers that could replace the non-std
> SIZE here. I had to look SIZE() up. It does the right thing, though.
>
>> +  _snwprintf(_abs_so_pathname, SIZE(_abs_so_pathname), L"%s%C%s",
>> plugindir, OS_SPECIFIC_DIRSEP, wide_string(p->so_pathname, ));
>
> You need to make sure _abs_so_pathname is zero terminated here. However, I
> found that _snwprintf does not return a negative value when the buffer is too
> small. Contrary to http://msdn.microsoft.com/en-us/library/2ts7cx93.aspx
> So, this could need more research to get right.

Thank you so much!
I did not know sn*printf of MS are not C99 complaint!

Alon.

Re: [Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[Jonathan K. Bullard]

On Tue, Jun 26, 2012 at 1:05 PM, Alon Bar-Lev  wrote:

> Currently openvpn requires/endorses specifying full path in plugin
> parameter. As build system already aware of plugin location, it is
> possible to load plugin relative to this directory, so full path is not
> required nor more secured.
>
> Windows is a little more complex as user may change installation
> directory at install time, so we read plugindir location from the
> registry, installer will set this value.
>

(Sorry Alon, I must have overlooked this when it was first posted.)

I do not understand the privilege escalation risk:

   - A protected (not writable by an unprivileged user) part of Tunnelblick
   controls the options sent to any instance of OpenVPN that runs as a
   privileged user, so it sends only acceptable paths. (A computer
   administrator is the only one who can modify a configuration file, which is
   the other means of specifying a plugin path.)
   - If a user runs an instance of OpenVPN from the OpenVPN binary inside
   of Tunnelblick, it runs as the user. So it's not very different form
   running it from the command line.

Aren't other installations of OpenVPN similarly protected? Or are you
trying to protect against poorly-protected installations?

Or can the server "push" a plugin path? If allowed (I don't think it should
be) that is a much bigger problem.

Disallowing absolute paths complicates things enormously for Tunnelblick
(OpenVPN GUI for OS X) for two reasons:

   - You can have multiple, different copies of Tunnelblick (it is
   self-contained), and
   - Each copy of Tunnelblick contains multiple versions of OpenVPN, each
   with its own copy of the down-root plugin.

Currently, Tunnelblick just sends a plugin path which points to the
appropriate plugin file inside the application itself (an OS X application
like Tunnelblick is basically a folder with stuff inside it).

If absolute paths are not allowed, Tunnelblick will apparently have to
start "installing" plugins in a system-wide directory, and track separate
copies of the down-root plugin by their source (i.e., the specific copy of
Tunnelblick, and the specific version of OpenVPN the down-root plugin comes
from. It would also be impossible to deal with a user moving the
Tunnelblick application somewhere else in the file hierarchy (Most OS X
applications, including Tunnelblick can be moved around as the user
pleases.)

That seems like so much trouble (and has a risk of creating a DLL-hell
equivalent) that I will probably drop support for the down-root plugin from
Tunnelblick. I don't think that's a big problem, but it will be for some
users, and they won't be able to script their way around it.

Re: [Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[Heiko Hund]

On Wednesday 18 July 2012 13:44:41 Heiko Hund wrote:
> code injection when openvpn is not running as another user or has access to

Scratch the "not" please, typo.

Heiko
-- 
Heiko Hund | Sr. Software Engineer | Tel +49-721-25516-237 | Fax -200
SOPHOS NSG | Amalienbadstr. 41 Bau 52 | 76227 Karlsruhe | Germany

Re: [Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[Heiko Hund]

Hi Alon

On Tuesday 26 June 2012 20:05:02 Alon Bar-Lev wrote:
> Currently openvpn requires/endorses specifying full path in plugin
> parameter. 

Specifying a custom full path is probably something we need to ban in the 
(near) future, as it imposes an attack vector for privilege escalation by code 
injection when openvpn is not running as another user or has access to 
privilege escalation by other means (like the yet to be submitted Windows 
interactive service).

This patch is a step towards this so I think it's absolutely worth a feature 
ACK.

>  AM_CFLAGS = \
> + -DOPENVPN_PLUGINDIR="\"$(plugindir)\"" \

Couldn't OPENVPN_PLUGINDIR be an AC_DEFINE in config.h instead? Would keep the 
command line shorter and more readable.

> + if (ExpandEnvironmentStringsW(plugindir, _plugindir_expanded, 
SIZE(_plugindir_expanded)) != 0) 

There's _countof defined in the Windows headers that could replace the non-std 
SIZE here. I had to look SIZE() up. It does the right thing, though.

> +  _snwprintf(_abs_so_pathname, SIZE(_abs_so_pathname), L"%s%C%s",
> plugindir, OS_SPECIFIC_DIRSEP, wide_string(p->so_pathname, )); 

You need to make sure _abs_so_pathname is zero terminated here. However, I 
found that _snwprintf does not return a negative value when the buffer is too 
small. Contrary to http://msdn.microsoft.com/en-us/library/2ts7cx93.aspx
So, this could need more research to get right.

Heiko

Re: [Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[Alon Bar-Lev]

On Tue, Jun 26, 2012 at 8:05 PM, Alon Bar-Lev  wrote:
> Currently openvpn requires/endorses specifying full path in plugin
> parameter. As build system already aware of plugin location, it is
> possible to load plugin relative to this directory, so full path is not
> required nor more secured.
>
> Windows is a little more complex as user may change installation
> directory at install time, so we read plugindir location from the
> registry, installer will set this value.
>
> This replaces the old unused logic of the PLUGIN_LIBDIR macro.
>
> The old unused logic of  ENABLE_PLUGIN_SEARCH is also removed as plugins
> are relative to the plugindir.
>
> Signed-off-by: Alon Bar-Lev 
> ---
>  src/openvpn/Makefile.am |    1 +
>  src/openvpn/options.c   |    3 +
>  src/openvpn/plugin.c    |  102 
> +++
>  3 files changed, 72 insertions(+), 34 deletions(-)

Yes,
I don't know why I keep doing this to my-self so with the current attitude.
But every place I look I find stuff that should be modified.

[Openvpn-devel] [PATCH] plugin: load plugin relative to plugindir

[Alon Bar-Lev]

Currently openvpn requires/endorses specifying full path in plugin
parameter. As build system already aware of plugin location, it is
possible to load plugin relative to this directory, so full path is not
required nor more secured.

Windows is a little more complex as user may change installation
directory at install time, so we read plugindir location from the
registry, installer will set this value.

This replaces the old unused logic of the PLUGIN_LIBDIR macro.

The old unused logic of  ENABLE_PLUGIN_SEARCH is also removed as plugins
are relative to the plugindir.

Signed-off-by: Alon Bar-Lev 
---
 src/openvpn/Makefile.am |1 +
 src/openvpn/options.c   |3 +
 src/openvpn/plugin.c|  102 +++
 3 files changed, 72 insertions(+), 34 deletions(-)

diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
index d090d67..a2af0d4 100644
--- a/src/openvpn/Makefile.am
+++ b/src/openvpn/Makefile.am
@@ -23,6 +23,7 @@ INCLUDES = \
-I$(top_srcdir)/src/compat

 AM_CFLAGS = \
+   -DOPENVPN_PLUGINDIR="\"$(plugindir)\"" \
$(TAP_CFLAGS) \
$(OPTIONAL_CRYPTO_CFLAGS) \
$(OPTIONAL_LZO_CFLAGS) \
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 367c1bc..d2e8e51 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3458,6 +3458,9 @@ usage_version (void)
   msg (M_INFO|M_NOPREFIX, "git revision: %s", CONFIGURE_GIT_REVISION);
 #endif
 #endif
+#ifdef OPENVPN_PLUGINDIR
+  msg (M_INFO|M_NOPREFIX, "build plugindir: %s", OPENVPN_PLUGINDIR);
+#endif
   openvpn_exit (OPENVPN_EXIT_STATUS_USAGE); /* exit point */
 }

diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
index 7ce2f5e..9cdf982 100644
--- a/src/openvpn/plugin.c
+++ b/src/openvpn/plugin.c
@@ -196,46 +196,83 @@ static void
 plugin_init_item (struct plugin *p, const struct plugin_option *o)
 {
   struct gc_arena gc = gc_new ();
-  bool rel = false;

   p->so_pathname = o->so_pathname;
   p->plugin_type_mask = plugin_supported_types ();

 #ifndef WIN32
-
-  p->handle = NULL;
-#if defined(PLUGIN_LIBDIR)
-  if (!absolute_pathname (p->so_pathname))
-{
-  char full[PATH_MAX];
-
-  openvpn_snprintf (full, sizeof(full), "%s/%s", PLUGIN_LIBDIR, 
p->so_pathname);
-  p->handle = dlopen (full, RTLD_NOW);
-#if defined(ENABLE_PLUGIN_SEARCH)
-  if (!p->handle)
-   {
- rel = true;
- p->handle = dlopen (p->so_pathname, RTLD_NOW);
-   }
-#endif
-}
-  else
-#endif
-{
-  rel = !absolute_pathname (p->so_pathname);
-  p->handle = dlopen (p->so_pathname, RTLD_NOW);
-}
-  if (!p->handle)
-msg (M_ERR, "PLUGIN_INIT: could not load plugin shared object %s: %s", 
p->so_pathname, dlerror());
+  {
+char _abs_so_pathname[PATH_MAX];
+const char *abs_so_pathname;
+
+if (absolute_pathname (p->so_pathname))
+  abs_so_pathname = p->so_pathname;
+else
+  {
+   openvpn_snprintf(_abs_so_pathname, SIZE(_abs_so_pathname), "%s%c%s", 
OPENVPN_PLUGINDIR, OS_SPECIFIC_DIRSEP, p->so_pathname);
+abs_so_pathname = _abs_so_pathname;
+  }
+
+p->handle = dlopen (abs_so_pathname, RTLD_NOW);
+if (!p->handle)
+  msg (M_ERR, "PLUGIN_INIT: could not load plugin shared object '%s': %s", 
abs_so_pathname, dlerror());

 # define PLUGIN_SYM(var, name, flags) libdl_resolve_symbol (p->handle, 
(void*)>var, name, p->so_pathname, flags)

+  }
 #else
-
-  rel = !absolute_pathname (p->so_pathname);
-  p->module = LoadLibraryW (wide_string (p->so_pathname, ));
-  if (!p->module)
-msg (M_ERR, "PLUGIN_INIT: could not load plugin DLL: %s", p->so_pathname);
+  {
+WCHAR _abs_so_pathname[PATH_MAX];
+WCHAR *abs_so_pathname;
+
+if (absolute_pathname (p->so_pathname))
+  abs_so_pathname = wide_string(p->so_pathname, );
+else
+  {
+ WCHAR _plugindir[PATH_MAX];
+ WCHAR _plugindir_expanded[PATH_MAX];
+ WCHAR *plugindir = NULL;
+ HKEY key;
+
+ if (RegOpenKeyExW(
+ HKEY_LOCAL_MACHINE,
+ L"SOFTWARE\\OpenVPN",
+ 0,
+ KEY_READ,
+ 
+   ) == ERROR_SUCCESS)
+   {
+ DWORD len = sizeof (_plugindir);
+ DWORD data_type;
+
+ if (RegQueryValueExW(
+ key,
+ L"plugindir",
+ NULL,
+ _type,
+ (PBYTE)_plugindir,
+ ) == ERROR_SUCCESS &&
+ data_type == REG_SZ)
+   {
+ plugindir = _plugindir;
+   }
+   }
+
+ if (plugindir == NULL)
+   plugindir = wide_string (OPENVPN_PLUGINDIR, );
+
+ if (ExpandEnvironmentStringsW(plugindir, _plugindir_expanded, 
SIZE(_plugindir_expanded)) != 0)
+  plugindir = _plugindir_expanded;
+
+_snwprintf(_abs_so_pathname, SIZE(_abs_so_pathname), L"%s%C%s", 
plugindir, OS_SPECIFIC_DIRSEP, wide_string(p->so_pathname,