[Openvpn-devel,v3] openvpnserv: enable interactive service to open tun

  From: Lev Stipakov <lev@openvpn.net>

This patch enables interactive service to open tun device.
This is mostly needed by Wintun, which could be opened
only by privileged process.

When interactive service is used, instead of calling
CreateFile() directly by openvpn process we pass tun device path
into service process. There we open device, duplicate handle
and pass it back to openvpn process.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 v3:
  - ensure that device path passed by client is null-terminated
  - support for multiple openvpn processes
  - return proper error code when device handle is invalid

 v2:
  - introduce send_msg_iservice_ex() instead of changing
  signature of existing send_msg_iservice()
  - use wchar_t strings in interactive service code

 include/openvpn-msg.h         | 12 +++++++
 src/openvpn/tun.c             | 83 ++++++++++++++++++++++++++++++++++---------
 src/openvpn/win32.c           |  9 ++++-
 src/openvpn/win32.h           | 30 +++++++++++++---
 src/openvpnserv/interactive.c | 64 +++++++++++++++++++++++++++++++--
 5 files changed, 174 insertions(+), 24 deletions(-)
  

Hi,

On Thu, Jun 27, 2019 at 8:08 AM Lev Stipakov <lstipakov@gmail.com> wrote:
>
> From: Lev Stipakov <lev@openvpn.net>
>
> This patch enables interactive service to open tun device.
> This is mostly needed by Wintun, which could be opened
> only by privileged process.
>
> When interactive service is used, instead of calling
> CreateFile() directly by openvpn process we pass tun device path
> into service process. There we open device, duplicate handle
> and pass it back to openvpn process.
>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  v3:
>   - ensure that device path passed by client is null-terminated
>   - support for multiple openvpn processes
>   - return proper error code when device handle is invalid

This works but there are two general concerns:

(i) The new message is named message_open_tun, but it allows opening
any file using the service. This is not secure. We need to restrict it
to open tun/tap device nodes only.

(ii) Should we allow all users to open tap6 adapters irrespective of
any other access restrictions that may be present? I'm conflicted
about this as, on closer look, access control in tap-windows6 appears
broken.

> @@ -117,4 +119,14 @@ typedef struct {
>      interface_t iface;
>  } enable_dhcp_message_t;
> +typedef struct {
> +    message_header_t header;
> +    char device_path[256];
> +} open_tun_device_message_t;
> +
> +typedef struct {
> +    message_header_t header;
> +    HANDLE handle;
> +    int error_number;
> +} open_tun_device_result_message_t;

Defining this struct with error_number followed by handle would be
better (makes its head match in memory with ack_message_t). That makes
it possible to read a normal ack into it and resolve the error number.
Can happen if openvpn.exe is upgraded but service stays at an old
version -- such a service will respond with ack and
error_number=ERROR_MESSAGE_TYPE.

Selva
  

Hi,

Sorry for delay - I was on vacation.

(i) The new message is named message_open_tun, but it allows opening
> any file using the service. This is not secure.


I am thinking of possible vector of attack here.

In our case it is service which launches openvpn process using
path set in registry, opens pipe and passes pipe handle to launched process.

To make service run malicious process one needs either to replace
executable or
modify registry. For both actions you need to be administrator, assuming
default security policy.

Alternatively, can random process hijack the pipe handle from another
process?


> We need to restrict it to open tun/tap device nodes only.
>

Without adding too much code, I can think of:

 - check that path starts with \\\\.\\Global\\ to make sure we open device,
not file

and

 - check that path starts with \\\\.\\Global\\WINTUN or ends with .tap

Is this good enough or do you have better ideas?

(ii) Should we allow all users to open tap6 adapters irrespective of
> any other access restrictions that may be present? I'm conflicted
> about this as, on closer look, access control in tap-windows6 appears
> broken.
>

I'll pass on this one. Maybe "Strengthening access control for
tap-windows6" could be another patch.


> Defining this struct with error_number followed by handle would be
> better (makes its head match in memory with ack_message_t). That makes
> it possible to read a normal ack into it and resolve the error number.
>

Agreed, will do.
  

Hi

On Wed, Jul 17, 2019 at 8:20 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> Hi,
>
> Sorry for delay - I was on vacation.
>
> (i) The new message is named message_open_tun, but it allows opening
>> any file using the service. This is not secure.
>
>
> I am thinking of possible vector of attack here.
>
> In our case it is service which launches openvpn process using
> path set in registry, opens pipe and passes pipe handle to launched
> process.
>
> To make service run malicious process one needs either to replace
> executable or
> modify registry. For both actions you need to be administrator, assuming
> default security policy.
>
> Alternatively, can random process hijack the pipe handle from another
> process?
>

Because the pipe handle is targeted for OpenVPN process id and not
inheritable that may not be possible. But an attack is very simple:

Just write a plugin and use the service to open files from it. Passing the
pipe handle to the plugin is trivial. Anyone in OpenVPNAdministrators group
can then use a custom config file and get access any file on the system.
Membership in that group is not supposed to provide any privileges to
limited users beyond setting up routes or manipulating some interface
parameters.


>
>> We need to restrict it to open tun/tap device nodes only.
>>
>
> Without adding too much code, I can think of:
>
>  - check that path starts with \\\\.\\Global\\ to make sure we open
> device, not file
>
> and
>
>  - check that path starts with \\\\.\\Global\\WINTUN or ends with .tap
>
> Is this good enough or do you have better ideas?
>

That'll probably work with some extra sanity checks on the file name.
Ideally we should just pass the dev-node (empty if unspecified) and type of
device (TAP6 or WINTUN), but that will require a lot of  duplication of
code in the service, as you noted.

One option is to pass the device guid in case of tap or the index in case
of wintun and construct the path in the service. That requires very little
extra code. Otherwise a thorough sanitization of the path is required as
there could be obscure ways of breaking out using "..\" or otherwise,
though I'm not sure. Things like \\.\C:\..\D:\ works on Windows so I won't
take any chances.

Selva

PS. Just noticed you've already posted a v4 -- I haven't looked at it yet.
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 17, 2019 at 8:20 AM Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">Hi,<div><br></div><div>Sorry for delay - I was on vacation.</div><div><br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
(i) The new message is named message_open_tun, but it allows opening<br>
any file using the service. This is not secure. </blockquote><div><br></div><div>I am thinking of possible vector of attack here.</div><div><br></div><div>In our case it is service which launches openvpn process using</div><div>path set in registry, opens pipe and passes pipe handle to launched process.</div><div><br></div><div>To make service run malicious process one needs either to replace executable or</div><div>modify registry. For both actions you need to be administrator, assuming default security policy.</div><div><br></div><div>Alternatively, can random process hijack the pipe handle from another process?</div></div></div></blockquote><div><br></div><div>Because the pipe handle is targeted for OpenVPN process id and not inheritable that may not be possible. But an attack is very simple: <br></div><div><br></div><div>Just write a plugin and use the service to open files from it. Passing the pipe handle to the plugin is trivial. Anyone in OpenVPNAdministrators group can then use a custom config file and get access any file on the system. Membership in that group is not supposed to provide any privileges to limited users beyond setting up routes or manipulating some interface parameters.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">We need to restrict it to open tun/tap device nodes only.<br></blockquote><div><br></div><div>Without adding too much code, I can think of:</div><div><br></div><div> - check that path starts with \\\\.\\Global\\ to make sure we open device, not file</div><div><br></div><div>and</div><div> </div><div> - check that path starts with \\\\.\\Global\\WINTUN or ends with .tap</div><div><br></div><div>Is this good enough or do you have better ideas?</div></div></div></blockquote><div><br></div><div>That&#39;ll probably work with some extra sanity checks on the file name. Ideally we should just pass the dev-node (empty if unspecified) and type of device (TAP6 or WINTUN), but that will require a lot of  duplication of code in the service, as you noted.<br></div><div><br></div><div>One option is to pass the device guid in case of tap or the index in case of wintun and construct the path in the service. That requires very little extra code. Otherwise a thorough sanitization of the path is required as there could be obscure ways of breaking out using &quot;..\&quot; or otherwise, though I&#39;m not sure. Things like \\.\C:\..\D:\ works on Windows so I won&#39;t take any chances.<br></div><div>  <br></div><div>Selva</div><div><br></div><div>PS. Just noticed you&#39;ve already posted a v4 -- I haven&#39;t looked at it yet.<br></div></div></div>
  

Hi,


> That'll probably work with some extra sanity checks on the file name.
> Ideally we should just pass the dev-node (empty if unspecified) and type of
> device (TAP6 or WINTUN), but that will require a lot of  duplication of
> code in the service, as you noted.
>
> One option is to pass the device guid in case of tap or the index in case
> of wintun and construct the path in the service. That requires very little
> extra code. Otherwise a thorough sanitization of the path is required as
> there could be obscure ways of breaking out using "..\" or otherwise,
> though I'm not sure. Things like \\.\C:\..\D:\ works on Windows so I won't
> take any chances.
>

You are right, just tested and one can escape global like this:

\\.\Global\..\C:\lol.tap

I'll do as you've proposed - pass a string which is either guid or number,
a boolean flag (wintun/tap6) and add some validation.


> PS. Just noticed you've already posted a v4 -- I haven't looked at it yet.
>

v5 is coming!