Re: [Openvpn-devel] [PATCH 8/7] wintun: fix ringbuffer registration call

2019-11-20 Thread Lev Stipakov 
Please disregard this patch - I made it as a fix to
"wintun: ring buffers based I/O", which was resent as v5.

ti 19. marrask. 2019 klo 21.10 Lev Stipakov (lstipa...@gmail.com) kirjoitti:

> From: Lev Stipakov 
>
> As MS documentation says:
>
>  > If lpOverlapped is NULL, lpBytesReturned cannot be NULL
>
> While on Windows 10 passing NULL works by accident,
> on Windows 7 it crashes.
>
> Reported-by: kitsune1
> Signed-off-by: Lev Stipakov 
> ---
>  src/openvpn/ring_buffer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpn/ring_buffer.c b/src/openvpn/ring_buffer.c
> index 482e3336..d384f497 100644
> --- a/src/openvpn/ring_buffer.c
> +++ b/src/openvpn/ring_buffer.c
> @@ -35,6 +35,7 @@ register_ring_buffers(HANDLE device,
>  {
>  struct tun_register_rings rr;
>  BOOL res;
> +DWORD bytes_returned;
>
>  ZeroMemory(, sizeof(rr));
>
> @@ -46,7 +47,8 @@ register_ring_buffers(HANDLE device,
>  rr.receive.ring_size = sizeof(receive_ring->data);
>  rr.receive.tail_moved = receive_tail_moved;
>
> -res = DeviceIoControl(device, TUN_IOCTL_REGISTER_RINGS, ,
> sizeof(rr), NULL, 0, NULL, NULL);
> +res = DeviceIoControl(device, TUN_IOCTL_REGISTER_RINGS, ,
> sizeof(rr),
> +NULL, 0, _returned, NULL);
>
>  return res == TRUE;
>  }
> --
> 2.17.1
>
>

-- 
-Lev
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH v4 5/7] wintun: interactive service support

2019-11-20 Thread Lev Stipakov 
From: Lev Stipakov 

Wintun requires ring buffers registration to be
performed by privileged process. In order to use
openvpn with wintun by non-Administrator, we
need to use interactive service and shared memory
to register buffers.

Openvpn process creates memory mapping object and event
for send and receive ring and passes handles to interactive
service. There handles are duplicated and memory mapped
object is mapped into the address space of service process.
Then address of mapped view and event handle is passed to
wintun kernel driver.

After interactive service preformed registration,
openvpn process maps memory mapped object into
own address space. Thus mapped views in openvpn
and service process represent the same memory region.

Signed-off-by: Lev Stipakov 
---
 While v2 has been ACKed, it had to be rebased on
 top of previous patch in series, which required some
 manual work.

 v4:
  - rebased on top of [PATCH v5 4/7] "wintun: ring buffers based I/O"
 
 v3:
  - rebased on top of [PATCH v4 4/7] "wintun: ring buffers based I/O"
  - added doxygen comments to ring_buffer.h

 v2:
  - rebased on top of master
 

 include/openvpn-msg.h   |  10 ++
 src/openvpn/Makefile.am |   2 +-
 src/openvpn/openvpn.vcxproj |   2 +
 src/openvpn/openvpn.vcxproj.filters |   8 +-
 src/openvpn/ring_buffer.c   |  56 
 src/openvpn/ring_buffer.h   | 105 +++
 src/openvpn/tun.c   |  82 ++--
 src/openvpn/tun.h   |   3 +
 src/openvpn/win32.c |  27 
 src/openvpn/win32.h |  47 ---
 src/openvpnserv/Makefile.am |   3 +-
 src/openvpnserv/interactive.c   | 141 +++-
 src/openvpnserv/openvpnserv.vcxproj |   2 +
 src/openvpnserv/openvpnserv.vcxproj.filters |   6 +
 14 files changed, 400 insertions(+), 94 deletions(-)
 create mode 100644 src/openvpn/ring_buffer.c
 create mode 100644 src/openvpn/ring_buffer.h

diff --git a/include/openvpn-msg.h b/include/openvpn-msg.h
index 66177a21..3ed62069 100644
--- a/include/openvpn-msg.h
+++ b/include/openvpn-msg.h
@@ -39,6 +39,7 @@ typedef enum {
 msg_del_block_dns,
 msg_register_dns,
 msg_enable_dhcp,
+msg_register_ring_buffers
 } message_type_t;
 
 typedef struct {
@@ -117,4 +118,13 @@ typedef struct {
 interface_t iface;
 } enable_dhcp_message_t;
 
+typedef struct {
+message_header_t header;
+HANDLE device;
+HANDLE send_ring_handle;
+HANDLE receive_ring_handle;
+HANDLE send_tail_moved;
+HANDLE receive_tail_moved;
+} register_ring_buffers_message_t;
+
 #endif /* ifndef OPENVPN_MSG_H_ */
diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
index a091ffc2..d1bb99c2 100644
--- a/src/openvpn/Makefile.am
+++ b/src/openvpn/Makefile.am
@@ -138,6 +138,6 @@ openvpn_LDADD = \
$(OPTIONAL_SYSTEMD_LIBS) \
$(OPTIONAL_DL_LIBS)
 if WIN32
-openvpn_SOURCES += openvpn_win32_resources.rc block_dns.c block_dns.h
+openvpn_SOURCES += openvpn_win32_resources.rc block_dns.c block_dns.h 
ring_buffer.c ring_buffer.h
 openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm 
-lfwpuclnt -lrpcrt4 -lncrypt -lsetupapi
 endif
diff --git a/src/openvpn/openvpn.vcxproj b/src/openvpn/openvpn.vcxproj
index 7446d97d..614d720a 100644
--- a/src/openvpn/openvpn.vcxproj
+++ b/src/openvpn/openvpn.vcxproj
@@ -181,6 +181,7 @@
 
 
 
+
 
 
 
@@ -265,6 +266,7 @@
 
 
 
+
 
 
 
diff --git a/src/openvpn/openvpn.vcxproj.filters 
b/src/openvpn/openvpn.vcxproj.filters
index 653e892c..41e62d14 100644
--- a/src/openvpn/openvpn.vcxproj.filters
+++ b/src/openvpn/openvpn.vcxproj.filters
@@ -240,6 +240,9 @@
 
   Source Files
 
+
+  Source Files
+
   
   
 
@@ -500,10 +503,13 @@
 
   Header Files
 
+
+  Header Files
+
   
   
 
   Resource Files
 
   
-
\ No newline at end of file
+
diff --git a/src/openvpn/ring_buffer.c b/src/openvpn/ring_buffer.c
new file mode 100644
index ..5c81e0d2
--- /dev/null
+++ b/src/openvpn/ring_buffer.c
@@ -0,0 +1,56 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ *  Copyright (C) 2002-2019 OpenVPN Inc 
+ *2019 Lev Stipakov 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or

[Openvpn-devel] [PATCH v5 4/7] wintun: ring buffers based I/O

2019-11-20 Thread Lev Stipakov 
From: Lev Stipakov 

Implemented according to Wintun documentation
and reference client code.

Wintun uses ring buffers to communicate between
kernel driver and user process. Client allocates
send and receive ring buffers, creates events
and passes it to kernel driver under LocalSystem
privileges.

When data is available for read, wintun modifies
"tail" pointer of send ring and signals via event.
User process reads data from "head" to "tail" and
updates "head" pointer.

When user process is ready to write, it writes
to receive ring, updates "tail" pointer and signals
to kernel via event.

In openvpn code we add send ring's event to event loop.
Before performing io wait, we compare "head" and "tail"
pointers of send ring and if they're different, we skip
io wait and perform read.

This also adds ring buffers support to tcp and udp
server code.

Signed-off-by: Lev Stipakov 
---
 v5:
  - fix crash at ring buffer registration on Win7
(passing NULL to DeviceIOControl, reported by kitsune1)

 v4:
  - added helper function tuntap_ring_empty()
  - refactored event handling, got rid of separate
event_ctl() call for wintun and send/receive_tail_moved
members
  - added wintun_ prefix for ring buffer variables
  - added a comment explaining the size of wintun-specific buffers

 v3:
  - simplified convoluted #ifdefs
  - replaced "greater than" with "greater or equal than"

 v2;
  - rebased on top of master

 src/openvpn/forward.c |  25 +++-
 src/openvpn/forward.h |  38 +++-
 src/openvpn/mtcp.c|  19 +-
 src/openvpn/mudp.c|   7 ++-
 src/openvpn/options.c |   4 +-
 src/openvpn/syshead.h |   1 +
 src/openvpn/tun.c |  62 ++-
 src/openvpn/tun.h | 135 +-
 src/openvpn/win32.c   | 122 ++
 src/openvpn/win32.h   |  51 
 10 files changed, 453 insertions(+), 11 deletions(-)

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 8451706b..3dc04a40 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1256,8 +1256,24 @@ read_incoming_tun(struct context *c)
 perf_push(PERF_READ_IN_TUN);
 
 c->c2.buf = c->c2.buffers->read_tun_buf;
+
 #ifdef _WIN32
-read_tun_buffered(c->c1.tuntap, >c2.buf);
+if (c->c1.tuntap->wintun)
+{
+read_wintun(c->c1.tuntap, >c2.buf);
+if (c->c2.buf.len == -1)
+{
+register_signal(c, SIGHUP, "tun-abort");
+c->persist.restart_sleep_seconds = 1;
+msg(M_INFO, "Wintun read error, restarting");
+perf_pop();
+return;
+}
+}
+else
+{
+read_tun_buffered(c->c1.tuntap, >c2.buf);
+}
 #else
 ASSERT(buf_init(>c2.buf, FRAME_HEADROOM(>c2.frame)));
 ASSERT(buf_safe(>c2.buf, MAX_RW_SIZE_TUN(>c2.frame)));
@@ -2099,6 +2115,13 @@ io_wait_dowork(struct context *c, const unsigned int 
flags)
 tuntap |= EVENT_READ;
 }
 
+#ifdef _WIN32
+if (tuntap_is_wintun(c->c1.tuntap))
+{
+tuntap = EVENT_READ;
+}
+#endif
+
 /*
  * Configure event wait based on socket, tuntap flags.
  */
diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
index 48202c07..b711ff00 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -375,6 +375,12 @@ p2p_iow_flags(const struct context *c)
 {
 flags |= IOW_TO_TUN;
 }
+#ifdef _WIN32
+if (tuntap_ring_empty(c->c1.tuntap))
+{
+flags &= ~IOW_READ_TUN;
+}
+#endif
 return flags;
 }
 
@@ -403,8 +409,36 @@ io_wait(struct context *c, const unsigned int flags)
 }
 else
 {
-/* slow path */
-io_wait_dowork(c, flags);
+#ifdef _WIN32
+bool skip_iowait = flags & IOW_TO_TUN;
+if (flags & IOW_READ_TUN)
+{
+/*
+ * don't read from tun if we have pending write to link,
+ * since every tun read overwrites to_link buffer filled
+ * by previous tun read
+ */
+skip_iowait = !(flags & IOW_TO_LINK);
+}
+if (tuntap_is_wintun(c->c1.tuntap) && skip_iowait)
+{
+unsigned int ret = 0;
+if (flags & IOW_TO_TUN)
+{
+ret |= TUN_WRITE;
+}
+if (flags & IOW_READ_TUN)
+{
+ret |= TUN_READ;
+}
+c->c2.event_set_status = ret;
+}
+else
+#endif
+{
+/* slow path */
+io_wait_dowork(c, flags);
+}
 }
 }
 
diff --git a/src/openvpn/mtcp.c b/src/openvpn/mtcp.c
index abe20593..30a13f73 100644
--- a/src/openvpn/mtcp.c
+++ b/src/openvpn/mtcp.c
@@ -269,8 +269,25 @@ multi_tcp_wait(const struct context *c,
struct multi_tcp *mtcp)
 {
 int status;
+unsigned int *persistent = >tun_rwflags;
 socket_set_listen_persistent(c->c2.link_socket, mtcp->es, MTCP_SOCKET);
-tun_set(c->c1.tuntap, mtcp->es, EVENT_READ,

Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device

2019-11-20 Thread Lev Stipakov 
Hi,


> We have more options here:
>

Can we combine B and C?

1) During installation, we create one adapter, as we do now.

2) On connection we enumerate adapters and pick the first one which is not
connected.

3) If we could not find free adapter (another VPN connection is already
running), we create one on demand.

-- 
-Lev
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device

2019-11-20 Thread Simon Rozman 
Hi,

 

The Wintun doesn't create its own communication I/O device. Running a separate 
NdisRegisterDeviceEx() device came with a big can of worms, so we decided not 
to run our own, but rather piggy-back on the existing NDIS one from the 
NdisMRegisterMiniportDriver() that we tap on. Technically, we could limit it to 
a single client handle, but this device is primarily used by Windows to manage 
the adapter, therefore we must not tweak it in this direction.

 

As Lev said: Wintun adds an IOCTL call to register ring buffers and it is this 
IOCTL that has a refcounting implemented to deny secondary connection attempts. 
Unfortunately for OpenVPN, this IOCTL is restricted to privileged processes 
only. You cannot use it to detect which Wintun adapters are in use by an 
unprivileged openvpn.exe process. WireGuard is very strict about which TUN 
adapter it will use – it will never ever use a lottery to pick one.

 

We have more options here:

 

A.  Wintun adapter can be explicitly chosen by configuration only (by name 
or GUID). With tapctl.exe utility users can make an adapter with predefined 
name and put this name into .ovpn. If the Wintun adapter is not specified 
explicitly => configuration error. Less lottery; predictable tunnel-adapter 
mapping when running multiple tunnels; disturbed users because there is an 
inconsistency between TAP-Windows6 and Wintun adapter selection logic.
B.  The tapctl.exe code is in OpenVPN repo already and could be integrated 
in openvpn.exe and interactive service to create Wintun adapter on demand. But, 
you have to take responsibility to clean what you have created. Pretty much the 
same as A), but nicer for user. In the long term, I'd suggest this "create 
adapter if doesn't exist" approach even for TAP-Windows6 adapters.
C.  Use adapter media state to see if particular Wintun adapter is already 
in use or not. Mind that registering buffer also sets 
MediaConnectStateConnected, and stays connected until its client closes handle 
or dies. This allows OpenVPN to extend its "use first available adapter" 
approach to Wintun adapters.

 

Best regards,

Simon

 

From: Selva Nair  
Sent: Tuesday, November 19, 2019 7:03 PM
To: Lev Stipakov 
Cc: Lev Stipakov ; openvpn-devel 

Subject: Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun 
device

 

Hi Lev,

 

On Tue, Nov 19, 2019 at 12:23 PM Lev Stipakov mailto:lstipa...@gmail.com> > wrote:

Hi,

 

Apart from the error message, there is a larger issue especially when we use 
iservice. In that case, we have to preserve privilege separation and allowing a 
user to open a device handle in use by another has to be avoided.

 

Do you see it as a security issue when handle can be opened by another process?

 

I don't know the internals of wintun to know that for sure. 

  

 

To read / write to tunnel one needs to register ring buffers, and this call 
will fail for any other process. Am I missing something here?

 

Hopefully, Simon can confirm whether that provides a sufficient safety net.

 

Selva



smime.p7s
Description: S/MIME cryptographic signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel