Re: [Openvpn-devel] [PATCH v3] openvpnserv: enable interactive service to open tun
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! -- -Lev ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] openvpnserv: enable interactive service to open tun
Hi On Wed, Jul 17, 2019 at 8:20 AM Lev Stipakov 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. ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] openvpnserv: enable interactive service to open tun
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. -- -Lev ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] openvpnserv: enable interactive service to open tun
Hi,
On Thu, Jun 27, 2019 at 8:08 AM Lev Stipakov wrote:
>
> From: Lev Stipakov
>
> 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
> ---
> 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
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
