[Openvpn-devel,v6,4/7] wintun: ring buffers based I/O

Message ID	20191213191901.300-1-lstipakov@gmail.com
State	Superseded
Headers	show Return-Path: <openvpn-devel-bounces@lists.sourceforge.net> From: Lev Stipakov <lstipakov@gmail.com> To: openvpn-devel@lists.sourceforge.net Date: Fri, 13 Dec 2019 21:19:01 +0200 Message-Id: <20191213191901.300-1-lstipakov@gmail.com> In-Reply-To: <20191213191634.250-1-lstipakov@gmail.com> References: <20191213191634.250-1-lstipakov@gmail.com> NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [URIs: openvpn.net] -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [209.85.208.194 listed in list.dnswl.org] 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (lstipakov[at]gmail.com) -0.0 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [209.85.208.194 listed in wl.mailspike.net] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid Subject: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O Precedence: list Cc: Lev Stipakov <lev@openvpn.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox
Series	[Openvpn-devel,v6] wintun: ring buffers based I/O \| expand [Openvpn-devel,v6] wintun: ring buffers based I/O [Openvpn-devel,v6,4/7] wintun: ring buffers based I/O

Lev Stipakov Dec. 13, 2019, 8:19 a.m. UTC

From: Lev Stipakov <lev@openvpn.net>

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 <lev@openvpn.net>
---
 v6:
  - added a sanity check to write_wintun() to avoid 
    writing malformed IPv4/6 packet, which causes
    "ring buffer is out of capacity" error.

 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     | 169 +++++++++++++++++++++++++++++++++++++++++-
 src/openvpn/win32.c   | 122 ++++++++++++++++++++++++++++++
 src/openvpn/win32.h   |  51 +++++++++++++
 10 files changed, 487 insertions(+), 11 deletions(-)

Steffan Karger Dec. 15, 2019, 2:45 a.m. UTC | #1

Hi Lev,

Thanks for the update. Even with the latest added fixes and sanity
checks, the resulting code is now 30 lines shorter *and* more readable.

Some last questions / comments inline:

On 13-12-2019 20:19, Lev Stipakov wrote:
> @@ -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

Now that the code is refactored, it is clear that for the tuntap case
the code will ignore any of the earlier set flags. Can you maybe add a
comment explaining why the other flags are being ignored?

> +            /* there is a data in wintun ring buffer, read it immediately */

Grammar nit: "there is data" (remove 'a').

Otherwise, as far as stare-at-code goes, I'm fine with this patch now. I
haven't tested it, nor have I reviewed Windows-specific interaction. So
this needs at least one other review from someone able to review those
parts.

Regarding the integration into OpenVPN:

Acked-by: Steffan Karger <steffan@karger.me>

-Steffan

Selva Nair Dec. 16, 2019, 5:11 a.m. UTC | #2

Hi,

I was reluctant to review this as I do not understand
the event processing in OpenVPN well enough. Now that Stefann
has reviewed those bits and given an Ack, here are some
comments on the rest of the code.

TLDR:
(i) stealing SYSTEM access from winlogon.exe is not a good thing to do
(ii) with a small change we can support multiple tunnels and  provide better
diagnostics when there are no free wintun adapters -- but this could also
be a follow up patch.

On Fri, Dec 13, 2019 at 2:20 PM Lev Stipakov <lstipakov@gmail.com> wrote:
>
> From: Lev Stipakov <lev@openvpn.net>
>
> 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 <lev@openvpn.net>
> ---
>  v6:
>   - added a sanity check to write_wintun() to avoid
>     writing malformed IPv4/6 packet, which causes
>     "ring buffer is out of capacity" error.
>
>  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     | 169 +++++++++++++++++++++++++++++++++++++++++-
>  src/openvpn/win32.c   | 122 ++++++++++++++++++++++++++++++
>  src/openvpn/win32.h   |  51 +++++++++++++
>  10 files changed, 487 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, &c->c2.buf);
> +    if (c->c1.tuntap->wintun)
> +    {
> +        read_wintun(c->c1.tuntap, &c->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, &c->c2.buf);
> +    }
>  #else
>      ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame)));
>      ASSERT(buf_safe(&c->c2.buf, MAX_RW_SIZE_TUN(&c->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 = &mtcp->tun_rwflags;
>      socket_set_listen_persistent(c->c2.link_socket, mtcp->es, MTCP_SOCKET);
> -    tun_set(c->c1.tuntap, mtcp->es, EVENT_READ, MTCP_TUN, &mtcp->tun_rwflags);
> +
> +#ifdef _WIN32
> +    if (tuntap_is_wintun(c->c1.tuntap))
> +    {
> +        if (!tuntap_ring_empty(c->c1.tuntap))
> +        {
> +            /* there is a data in wintun ring buffer, read it immediately */
> +            mtcp->esr[0].arg = MTCP_TUN;
> +            mtcp->esr[0].rwflags = EVENT_READ;
> +            mtcp->n_esr = 1;
> +            return 1;
> +        }
> +        persistent = NULL;
> +    }
> +#endif
> +    tun_set(c->c1.tuntap, mtcp->es, EVENT_READ, MTCP_TUN, persistent);
> +
>  #ifdef ENABLE_MANAGEMENT
>      if (management)
>      {
> diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
> index b7f061a2..6a29ccc8 100644
> --- a/src/openvpn/mudp.c
> +++ b/src/openvpn/mudp.c
> @@ -278,7 +278,12 @@ p2mp_iow_flags(const struct multi_context *m)
>      {
>          flags |= IOW_READ;
>      }
> -
> +#ifdef _WIN32
> +    if (tuntap_ring_empty(m->top.c1.tuntap))
> +    {
> +        flags &= ~IOW_READ_TUN;
> +    }
> +#endif
>      return flags;
>  }
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 49affc29..cebcbb07 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -3016,10 +3016,10 @@ options_postprocess_mutate_invariant(struct options *options)
>          options->ifconfig_noexec = false;
>      }
>
> -    /* for wintun kernel doesn't send DHCP requests, so use ipapi to set IP address and netmask */
> +    /* for wintun kernel doesn't send DHCP requests, so use netsh to set IP address and netmask */
>      if (options->wintun)
>      {
> -        options->tuntap_options.ip_win32_type = IPW32_SET_IPAPI;
> +        options->tuntap_options.ip_win32_type = IPW32_SET_NETSH;

This shouldn't be required. I would prefer to not use netsh
for tasks where API calls are available. We know IPAPI works
as we use it in the service.

>      }
>
>      remap_redirect_gateway_flags(options);
> diff --git a/src/openvpn/syshead.h b/src/openvpn/syshead.h
> index 899aa59e..e9accb52 100644
> --- a/src/openvpn/syshead.h
> +++ b/src/openvpn/syshead.h
> @@ -39,6 +39,7 @@
>  #ifdef _WIN32
>  #include <windows.h>
>  #include <winsock2.h>
> +#include <tlhelp32.h>
>  #define sleep(x) Sleep((x)*1000)
>  #define random rand
>  #define srandom srand
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 599fd817..3f66b216 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -775,9 +775,27 @@ init_tun_post(struct tuntap *tt,
>  #ifdef _WIN32
>      overlapped_io_init(&tt->reads, frame, FALSE, true);
>      overlapped_io_init(&tt->writes, frame, TRUE, true);
> -    tt->rw_handle.read = tt->reads.overlapped.hEvent;
> -    tt->rw_handle.write = tt->writes.overlapped.hEvent;
>      tt->adapter_index = TUN_ADAPTER_INDEX_INVALID;
> +
> +    if (tt->wintun)
> +    {
> +        tt->wintun_send_ring = malloc(sizeof(struct tun_ring));
> +        tt->wintun_receive_ring = malloc(sizeof(struct tun_ring));
> +        if ((tt->wintun_send_ring == NULL) || (tt->wintun_receive_ring == NULL))
> +        {
> +            msg(M_FATAL, "Cannot allocate memory for ring buffer");
> +        }
> +        ZeroMemory(tt->wintun_send_ring, sizeof(struct tun_ring));
> +        ZeroMemory(tt->wintun_receive_ring, sizeof(struct tun_ring));
> +
> +        tt->rw_handle.read = CreateEvent(NULL, FALSE, FALSE, NULL);
> +        tt->rw_handle.write = CreateEvent(NULL, FALSE, FALSE, NULL);
> +    }
> +    else
> +    {
> +        tt->rw_handle.read = tt->reads.overlapped.hEvent;
> +        tt->rw_handle.write = tt->writes.overlapped.hEvent;
> +    }
>  #endif
>  }
>
> @@ -6177,6 +6195,34 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
>              tt->ipapi_context_defined = true;
>          }
>      }
> +
> +    if (tt->wintun)
> +    {
> +        if (tt->options.msg_channel)
> +        {
> +            /* TODO */
> +        }
> +        else
> +        {
> +            if (!impersonate_as_system())
> +            {
> +                msg(M_FATAL, "ERROR:  Failed to impersonate as SYSTEM, make sure process is running under privileged account");
> +            }
> +            if (!register_ring_buffers(tt->hand,
> +                                       tt->wintun_send_ring,
> +                                       tt->wintun_receive_ring,
> +                                       tt->rw_handle.read,
> +                                       tt->rw_handle.write))
> +            {
> +                msg(M_FATAL, "ERROR:  Failed to register ring buffers: %lu", GetLastError());

The correct error here is, very likely, the adapter is
already in use. To trigger that, why not do register_ring_buffers
soon after opening the device and on failure move on to the
next device (if any).

That will also allow running multiple tunnels with no further changes
provided the user manually creates wintum adapters.

> +            }
> +            if (!RevertToSelf())
> +            {
> +                msg(M_FATAL, "ERROR:  RevertToSelf error: %lu", GetLastError());
> +            }
> +        }
> +    }
> +
>      /*netcmd_semaphore_release ();*/
>      gc_free(&gc);
>  }
> @@ -6315,6 +6361,18 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
>          free(tt->actual_name);
>      }
>
> +    if (tt->wintun)
> +    {
> +        CloseHandle(tt->rw_handle.read);
> +        CloseHandle(tt->rw_handle.write);
> +    }
> +
> +    free(tt->wintun_receive_ring);
> +    free(tt->wintun_send_ring);
> +
> +    tt->wintun_receive_ring = NULL;
> +    tt->wintun_send_ring = NULL;
> +
>      clear_tuntap(tt);
>      free(tt);
>      gc_free(&gc);
> diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
> index 66b75d93..10f687c5 100644
> --- a/src/openvpn/tun.h
> +++ b/src/openvpn/tun.h
> @@ -182,6 +182,9 @@ struct tuntap
>
>      bool wintun; /* true if wintun is used instead of tap-windows6 */
>      int standby_iter;
> +
> +    struct tun_ring *wintun_send_ring;
> +    struct tun_ring *wintun_receive_ring;
>  #else  /* ifdef _WIN32 */
>      int fd; /* file descriptor for TUN/TAP dev */
>  #endif
> @@ -211,6 +214,20 @@ tuntap_defined(const struct tuntap *tt)
>  #endif
>  }
>
> +#ifdef _WIN32
> +inline bool
> +tuntap_is_wintun(struct tuntap *tt)
> +{
> +    return tt && tt->wintun;
> +}
> +
> +inline bool
> +tuntap_ring_empty(struct tuntap *tt)
> +{
> +    return tuntap_is_wintun(tt) && (tt->wintun_send_ring->head == tt->wintun_send_ring->tail);
> +}
> +#endif
> +
>  /*
>   * Function prototypes
>   */
> @@ -478,10 +495,158 @@ read_tun_buffered(struct tuntap *tt, struct buffer *buf)
>      return tun_finalize(tt->hand, &tt->reads, buf);
>  }
>
> +static inline ULONG
> +wintun_ring_packet_align(ULONG size)
> +{
> +    return (size + (WINTUN_PACKET_ALIGN - 1)) & ~(WINTUN_PACKET_ALIGN - 1);
> +}
> +
> +static inline ULONG
> +wintun_ring_wrap(ULONG value)
> +{
> +    return value & (WINTUN_RING_CAPACITY - 1);
> +}
> +
> +static inline void
> +read_wintun(struct tuntap *tt, struct buffer* buf)
> +{
> +    struct tun_ring *ring = tt->wintun_send_ring;
> +    ULONG head = ring->head;
> +    ULONG tail = ring->tail;
> +    ULONG content_len;
> +    struct TUN_PACKET *packet;
> +    ULONG aligned_packet_size;
> +
> +    *buf = tt->reads.buf_init;
> +    buf->len = 0;
> +
> +    if ((head >= WINTUN_RING_CAPACITY) || (tail >= WINTUN_RING_CAPACITY))
> +    {
> +        msg(M_INFO, "Wintun: ring capacity exceeded");
> +        buf->len = -1;
> +        return;
> +    }
> +
> +    if (head == tail)
> +    {
> +        /* nothing to read */
> +        return;
> +    }
> +
> +    content_len = wintun_ring_wrap(tail - head);
> +    if (content_len < sizeof(struct TUN_PACKET_HEADER))
> +    {
> +        msg(M_INFO, "Wintun: incomplete packet header in send ring");
> +        buf->len = -1;
> +        return;
> +    }
> +
> +    packet = (struct TUN_PACKET *) &ring->data[head];
> +    if (packet->size > WINTUN_MAX_PACKET_SIZE)
> +    {
> +        msg(M_INFO, "Wintun: packet too big in send ring");
> +        buf->len = -1;
> +        return;
> +    }
> +
> +    aligned_packet_size = wintun_ring_packet_align(sizeof(struct TUN_PACKET_HEADER) + packet->size);
> +    if (aligned_packet_size > content_len)
> +    {
> +        msg(M_INFO, "Wintun: incomplete packet in send ring");
> +        buf->len = -1;
> +        return;
> +    }
> +
> +    buf_write(buf, packet->data, packet->size);
> +
> +    head = wintun_ring_wrap(head + aligned_packet_size);
> +    ring->head = head;
> +}
> +
> +static inline bool
> +is_ip_packet_valid(const struct buffer *buf)
> +{
> +    const struct openvpn_iphdr* ih = (const struct openvpn_iphdr *)BPTR(buf);
> +
> +    if (OPENVPN_IPH_GET_VER(ih->version_len) == 4)
> +    {
> +        if (BLEN(buf) < sizeof(struct openvpn_iphdr))
> +        {
> +            return false;
> +        }
> +    }
> +    else if (OPENVPN_IPH_GET_VER(ih->version_len) == 6)
> +    {
> +        if (BLEN(buf) < sizeof(struct openvpn_ipv6hdr))
> +        {
> +            return false;
> +        }
> +    }
> +    else
> +    {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static inline int
> +write_wintun(struct tuntap *tt, struct buffer *buf)
> +{
> +    struct tun_ring *ring = tt->wintun_receive_ring;
> +    ULONG head = ring->head;
> +    ULONG tail = ring->tail;
> +    ULONG aligned_packet_size;
> +    ULONG buf_space;
> +    struct TUN_PACKET *packet;
> +
> +    /* wintun marks ring as corrupted (overcapacity) if it receives invalid IP packet */
> +    if (!is_ip_packet_valid(buf))
> +    {
> +        msg(D_LOW, "write_wintun(): drop invalid IP packet");
> +        return 0;
> +    }
> +
> +    if ((head >= WINTUN_RING_CAPACITY) || (tail >= WINTUN_RING_CAPACITY))
> +    {
> +        msg(M_INFO, "write_wintun(): head/tail value is over capacity");
> +        return -1;
> +    }
> +
> +    aligned_packet_size = wintun_ring_packet_align(sizeof(struct TUN_PACKET_HEADER) + BLEN(buf));
> +    buf_space = wintun_ring_wrap(head - tail - WINTUN_PACKET_ALIGN);
> +    if (aligned_packet_size > buf_space)
> +    {
> +        msg(M_INFO, "write_wintun(): ring is full");
> +        return 0;
> +    }
> +
> +    /* copy packet size and data into ring */
> +    packet = (struct TUN_PACKET* )&ring->data[tail];
> +    packet->size = BLEN(buf);
> +    memcpy(packet->data, BPTR(buf), BLEN(buf));
> +
> +    /* move ring tail */
> +    ring->tail = wintun_ring_wrap(tail + aligned_packet_size);
> +    if (ring->alertable != 0)
> +    {
> +        SetEvent(tt->rw_handle.write);
> +    }
> +
> +    return BLEN(buf);
> +}
> +
>  static inline int
>  write_tun_buffered(struct tuntap *tt, struct buffer *buf)
>  {
> -    return tun_write_win32(tt, buf);
> +    if (tt->wintun)
> +    {
> +        return write_wintun(tt, buf);
> +    }
> +    else
> +    {
> +        return tun_write_win32(tt, buf);
> +    }
>  }
>
>  #else  /* ifdef _WIN32 */
> @@ -544,7 +709,7 @@ tun_set(struct tuntap *tt,
>              }
>          }
>  #ifdef _WIN32
> -        if (rwflags & EVENT_READ)
> +        if (!tt->wintun && (rwflags & EVENT_READ))
>          {
>              tun_read_queue(tt, 0);
>          }
> diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
> index eb4c0307..5c168b3b 100644
> --- a/src/openvpn/win32.c
> +++ b/src/openvpn/win32.c
> @@ -1493,4 +1493,126 @@ send_msg_iservice(HANDLE pipe, const void *data, size_t size,
>      return ret;
>  }
>
> +bool
> +impersonate_as_system()
> +{

This is implemented by stealing the access token from
winlogon.exe. I don't think such tricks belong to OpenVPN.
It may also trip some anti-virus software.

That said, probably there are no "legitimate" ways of getting
LOCAL SYSTEM rights on Windows without running a service.

Why does wintun require SYSTEM for using it? If there is a
good reason for that, we should not let every admin
user bypass it.

I would suggest to not elevate to system as the code will still
work in two important cases
(i) started by automatic service
(ii) started using interactive service (e.g., through the GUI)

Those who really need to test OpenVPN with wintun from
command prompt can use diagnostic tools available to get
a cmd prompt as system (e.g., psexec). That also  makes
it explicit that SYSTEM privilege is required.

In the longer run, we could provide a script to launch
openvpn.exe using the interactive service. Modifying the
automatic service to use interactive service for launching
looks easy to do as well. Then, all privileged operations could
be removed from openvpn core.

> +    HANDLE thread_token, process_snapshot, winlogon_process, winlogon_token, duplicated_token;
> +    PROCESSENTRY32 entry;
> +    BOOL ret;
> +    DWORD pid = 0;
> +    TOKEN_PRIVILEGES privileges;
> +
> +    CLEAR(entry);
> +    CLEAR(privileges);
> +
> +    entry.dwSize = sizeof(PROCESSENTRY32);
> +
> +    privileges.PrivilegeCount = 1;
> +    privileges.Privileges->Attributes = SE_PRIVILEGE_ENABLED;
> +
> +    if (!LookupPrivilegeValue(NULL, SE_DEBUG_NAME, &privileges.Privileges[0].Luid))
> +    {
> +        return false;

In case we keep this function, please log such
errors. Here and several such cases below.

> +    }
> +
> +    if (!ImpersonateSelf(SecurityImpersonation))
> +    {
> +        return false;
> +    }
> +
> +    if (!OpenThreadToken(GetCurrentThread(), TOKEN_ADJUST_PRIVILEGES, FALSE, &thread_token))
> +    {
> +        RevertToSelf();
> +        return false;
> +    }
> +    if (!AdjustTokenPrivileges(thread_token, FALSE, &privileges, sizeof(privileges), NULL, NULL))
> +    {
> +        CloseHandle(thread_token);
> +        RevertToSelf();
> +        return false;
> +    }
> +    CloseHandle(thread_token);
> +
> +    process_snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
> +    if (process_snapshot == INVALID_HANDLE_VALUE)
> +    {
> +        RevertToSelf();
> +        return false;
> +    }
> +    for (ret = Process32First(process_snapshot, &entry); ret; ret = Process32Next(process_snapshot, &entry))
> +    {
> +        if (!_stricmp(entry.szExeFile, "winlogon.exe"))
> +        {
> +            pid = entry.th32ProcessID;
> +            break;
> +        }
> +    }
> +    CloseHandle(process_snapshot);
> +    if (!pid)
> +    {
> +        RevertToSelf();
> +        return false;
> +    }
> +
> +    winlogon_process = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);
> +    if (!winlogon_process)
> +    {
> +        RevertToSelf();
> +        return false;
> +    }
> +
> +    if (!OpenProcessToken(winlogon_process, TOKEN_IMPERSONATE | TOKEN_DUPLICATE, &winlogon_token))
> +    {
> +        CloseHandle(winlogon_process);
> +        RevertToSelf();
> +        return false;
> +    }
> +    CloseHandle(winlogon_process);
> +
> +    if (!DuplicateToken(winlogon_token, SecurityImpersonation, &duplicated_token))
> +    {
> +        CloseHandle(winlogon_token);
> +        RevertToSelf();
> +        return false;
> +    }
> +    CloseHandle(winlogon_token);
> +
> +    if (!SetThreadToken(NULL, duplicated_token))
> +    {
> +        CloseHandle(duplicated_token);
> +        RevertToSelf();
> +        return false;
> +    }
> +    CloseHandle(duplicated_token);
> +
> +    return true;
> +}
> +
> +bool
> +register_ring_buffers(HANDLE device,
> +                      struct tun_ring* send_ring,
> +                      struct tun_ring* receive_ring,
> +                      HANDLE send_tail_moved,
> +                      HANDLE receive_tail_moved)
> +{
> +    struct tun_register_rings rr;
> +    BOOL res;
> +    DWORD bytes_returned;
> +
> +    ZeroMemory(&rr, sizeof(rr));
> +
> +    rr.send.ring = send_ring;
> +    rr.send.ring_size = sizeof(send_ring->data);
> +    rr.send.tail_moved = send_tail_moved;
> +
> +    rr.receive.ring = receive_ring;
> +    rr.receive.ring_size = sizeof(receive_ring->data);
> +    rr.receive.tail_moved = receive_tail_moved;
> +
> +    res = DeviceIoControl(device, TUN_IOCTL_REGISTER_RINGS, &rr, sizeof(rr),
> +                          NULL, 0, &bytes_returned, NULL);
> +
> +    return res == TRUE;
> +}
> +
>  #endif /* ifdef _WIN32 */
> diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
> index 4814bbc5..f6d45bdc 100644
> --- a/src/openvpn/win32.h
> +++ b/src/openvpn/win32.h
> @@ -25,6 +25,8 @@
>  #ifndef OPENVPN_WIN32_H
>  #define OPENVPN_WIN32_H
>
> +#include <winioctl.h>
> +
>  #include "mtu.h"
>  #include "openvpn-msg.h"
>  #include "argv.h"
> @@ -323,5 +325,54 @@ bool send_msg_iservice(HANDLE pipe, const void *data, size_t size,
>  int
>  openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned int flags);
>
> +/*
> + * Values below are taken from Wireguard Windows client
> + * https://github.com/WireGuard/wireguard-go/blob/master/tun/wintun/ring_windows.go#L14
> + */
> +#define WINTUN_RING_CAPACITY 0x800000
> +#define WINTUN_RING_TRAILING_BYTES 0x10000
> +#define WINTUN_RING_FRAMING_SIZE 12
> +#define WINTUN_MAX_PACKET_SIZE 0xffff
> +#define WINTUN_PACKET_ALIGN 4
> +
> +struct tun_ring
> +{
> +    volatile ULONG head;
> +    volatile ULONG tail;
> +    volatile LONG alertable;
> +    UCHAR data[WINTUN_RING_CAPACITY + WINTUN_RING_TRAILING_BYTES + WINTUN_RING_FRAMING_SIZE];
> +};
> +
> +struct tun_register_rings
> +{
> +    struct
> +    {
> +        ULONG ring_size;
> +        struct tun_ring *ring;
> +        HANDLE tail_moved;
> +    } send, receive;
> +};
> +
> +struct TUN_PACKET_HEADER
> +{
> +    uint32_t size;
> +};
> +
> +struct TUN_PACKET
> +{
> +    uint32_t size;
> +    UCHAR data[WINTUN_MAX_PACKET_SIZE];
> +};
> +
> +#define TUN_IOCTL_REGISTER_RINGS CTL_CODE(51820U, 0x970U, METHOD_BUFFERED, FILE_READ_DATA | FILE_WRITE_DATA)
> +
> +bool impersonate_as_system();
> +
> +bool register_ring_buffers(HANDLE device,
> +                           struct tun_ring *send_ring,
> +                           struct tun_ring *receive_ring,
> +                           HANDLE send_tail_moved,
> +                           HANDLE receive_tail_moved);
> +
>  #endif /* ifndef OPENVPN_WIN32_H */
>  #endif /* ifdef _WIN32 */
> --

Selva

Lev Stipakov Dec. 16, 2019, 9 a.m. UTC | #3

Hi,

Thanks for looking into this. See my comments below.

TLDR:
> (i) stealing SYSTEM access from winlogon.exe is not a good thing to do
>

This doesn't happen for the majority of use cases - only when iservice is
not used. We also
elevate only for the single DeviceIOControl call.

Below you mentioned psexec as a one of workarounds, but I think those will
make user experience worse.
Also consider the case when OpenVPN GUI is running as Administrator.

(ii) with a small change we can support multiple tunnels and  provide better
> diagnostics when there are no free wintun adapters -- but this could also
> be a follow up patch.
>

This is already implemented by Simon on top of my patches - see
https://github.com/rozmansi/openvpn/commits/wintun

> -    /* for wintun kernel doesn't send DHCP requests, so use ipapi to set
> IP address and netmask */

> +    /* for wintun kernel doesn't send DHCP requests, so use netsh to set
> IP address and netmask */
> >      if (options->wintun)
> >      {
> > -        options->tuntap_options.ip_win32_type = IPW32_SET_IPAPI;
> > +        options->tuntap_options.ip_win32_type = IPW32_SET_NETSH;
>
> This shouldn't be required. I would prefer to not use netsh
> for tasks where API calls are available. We know IPAPI works
> as we use it in the service.
>

I remember having some issues with IPAPI which got resolved when I switched
to netsh.

However, I did some testing and I cannot reproduce IPAPI specific issues
anymore. Too bad I didn't investigate
it further back then. The problem I see now can also be reproduced also
with netsh:

 - connect with tap adapter
 - kill openvpn process
 - connect with wintun adapter

IPAPI / netsh fails to set IP address, since it is already assigned to
another adapter.

If this is OK, I would prefer to submit a separate patch which switches
back to netsh. tun.c code
has changed in follow-up patches (mostly here
https://patchwork.openvpn.net/patch/918/), those patches
are based on an assumption that wintun uses netsh.


> > +                msg(M_FATAL, "ERROR:  Failed to register ring buffers:
> %lu", GetLastError());
>
> The correct error here is, very likely, the adapter is
> already in use. To trigger that, why not do register_ring_buffers
> soon after opening the device and on failure move on to the
> next device (if any).
>
> That will also allow running multiple tunnels with no further changes
> provided the user manually creates wintum adapters.
>

As mentioned before, this is already implemented by Simon:
https://github.com/rozmansi/openvpn/commit/5bc09580e8771d8422feae4504fe2a74b7412fd1


> > +bool

> +impersonate_as_system()
> > +{
>
> This is implemented by stealing the access token from
> winlogon.exe. I don't think such tricks belong to OpenVPN.
> It may also trip some anti-virus software.
>
> That said, probably there are no "legitimate" ways of getting
> LOCAL SYSTEM rights on Windows without running a service.
>
> Why does wintun require SYSTEM for using it? If there is a
> good reason for that, we should not let every admin
> user bypass it.
>

I'll defer it to Simon.


>
> I would suggest to not elevate to system as the code will still
> work in two important cases
> (i) started by automatic service
> (ii) started using interactive service (e.g., through the GUI)
>
> Those who really need to test OpenVPN with wintun from
> command prompt can use diagnostic tools available to get
> a cmd prompt as system (e.g., psexec). That also  makes
> it explicit that SYSTEM privilege is required.
>
> In the longer run, we could provide a script to launch
> openvpn.exe using the interactive service. Modifying the
> automatic service to use interactive service for launching
> looks easy to do as well. Then, all privileged operations could
> be removed from openvpn core.
>

I think it is good not to break user experience and allow run openvpn as
an administrator without iservice using wintun at the expense on elevation
to system for single API call.

In case we keep this function, please log such
> errors. Here and several such cases below.
>

Sure.

Selva Nair Dec. 16, 2019, 9:17 a.m. UTC | #4

Hi

On Mon, Dec 16, 2019 at 3:01 PM Lev Stipakov <lstipakov@gmail.com> wrote:
>
> Hi,
>
> Thanks for looking into this. See my comments below.
>
>> TLDR:
>> (i) stealing SYSTEM access from winlogon.exe is not a good thing to do
>
>
> This doesn't happen for the majority of use cases - only when iservice is not used. We also
> elevate only for the single DeviceIOControl call.

I understand. But stealing access token from winlogon.exe is a hack and not
something I would expect to see in a trustworthy executable. Diagnostic
and forensic tools may be justified in doing such things.

>
> Below you mentioned psexec as a one of workarounds, but I think
> those will make user experience worse.

I was not suggesting it as an approach for "users" -- only for testing
or for those
who insist running OpenVPN from command line.

> Also consider the case when OpenVPN GUI is running as Administrator.

That would be a wrong way of using the GUI in 2.4 + releases. We need not and
should not (IMO) support the GUI running as admin. That said, now that vista is
no longer supported we can enable the use of iservice with GUI running as admin.

>
>> (ii) with a small change we can support multiple tunnels and  provide better
>> diagnostics when there are no free wintun adapters -- but this could also
>> be a follow up patch.
>
>
> This is already implemented by Simon on top of my patches - see https://github.com/rozmansi/openvpn/commits/wintun
>
>> > -    /* for wintun kernel doesn't send DHCP requests, so use ipapi to set IP address and netmask */
>>
>> > +    /* for wintun kernel doesn't send DHCP requests, so use netsh to set IP address and netmask */
>> >      if (options->wintun)
>> >      {
>> > -        options->tuntap_options.ip_win32_type = IPW32_SET_IPAPI;
>> > +        options->tuntap_options.ip_win32_type = IPW32_SET_NETSH;
>>
>> This shouldn't be required. I would prefer to not use netsh
>> for tasks where API calls are available. We know IPAPI works
>> as we use it in the service.
>
>
> I remember having some issues with IPAPI which got resolved when I switched to netsh.
>
> However, I did some testing and I cannot reproduce IPAPI specific issues anymore. Too bad I didn't investigate
> it further back then. The problem I see now can also be reproduced also with netsh:
>
>  - connect with tap adapter
>  - kill openvpn process
>  - connect with wintun adapter
>
> IPAPI / netsh fails to set IP address, since it is already assigned to another adapter.
>
> If this is OK, I would prefer to submit a separate patch which switches back to netsh. tun.c code
> has changed in follow-up patches (mostly here https://patchwork.openvpn.net/patch/918/), those patches
> are based on an assumption that wintun uses netsh.
>
>>
>> > +                msg(M_FATAL, "ERROR:  Failed to register ring buffers: %lu", GetLastError());
>>
>> The correct error here is, very likely, the adapter is
>> already in use. To trigger that, why not do register_ring_buffers
>> soon after opening the device and on failure move on to the
>> next device (if any).
>>
>> That will also allow running multiple tunnels with no further changes
>> provided the user manually creates wintum adapters.
>
>
> As mentioned before, this is already implemented by Simon:  https://github.com/rozmansi/openvpn/commit/5bc09580e8771d8422feae4504fe2a74b7412fd1
>
>>
>> > +bool
>>
>> > +impersonate_as_system()
>> > +{
>>
>> This is implemented by stealing the access token from
>> winlogon.exe. I don't think such tricks belong to OpenVPN.
>> It may also trip some anti-virus software.
>>
>> That said, probably there are no "legitimate" ways of getting
>> LOCAL SYSTEM rights on Windows without running a service.
>>
>> Why does wintun require SYSTEM for using it? If there is a
>> good reason for that, we should not let every admin
>> user bypass it.
>
>
> I'll defer it to Simon.
>
>>
>>
>> I would suggest to not elevate to system as the code will still
>> work in two important cases
>> (i) started by automatic service
>> (ii) started using interactive service (e.g., through the GUI)
>>
>> Those who really need to test OpenVPN with wintun from
>> command prompt can use diagnostic tools available to get
>> a cmd prompt as system (e.g., psexec). That also  makes
>> it explicit that SYSTEM privilege is required.
>>
>> In the longer run, we could provide a script to launch
>> openvpn.exe using the interactive service. Modifying the
>> automatic service to use interactive service for launching
>> looks easy to do as well. Then, all privileged operations could
>> be removed from openvpn core.
>
>
> I think it is good not to break user experience and allow run openvpn as
> an administrator without iservice using wintun at the expense on elevation
> to system for single API call.

I have already said what I think of it. As an admin I wouldn't like to see
users running processes that elevate to SYSTEM like this.

Selva

Lev Stipakov Dec. 16, 2019, 10:31 a.m. UTC | #5

>
> I have already said what I think of it. As an admin I wouldn't like to see
> users running processes that elevate to SYSTEM like this.
>

Would it be too much if

 - openvpn.exe process detects that it is not started by interactive service
 - interactive service process is running
 - wintun is used as --windows-driver

then openvpn behaves as a command-line version of openvpn-gui - connects to
service, which
runs openvpn.exe, and then prints output, received via management
interface, to console?

Simon Rozman Dec. 16, 2019, 11:18 a.m. UTC | #6

Hi,

>>> TLDR:
>>> (i) stealing SYSTEM access from winlogon.exe is not a good thing to do
>> 
>> 
>> This doesn't happen for the majority of use cases - only when iservice is not used. We also
>> elevate only for the single DeviceIOControl call.
> 
> I understand. But stealing access token from winlogon.exe is a hack and not
> something I would expect to see in a trustworthy executable. Diagnostic
> and forensic tools may be justified in doing such things.

Wintun has a hardcoded check to allow ring registration from the SYSTEM user only.

To be honest: I am using a Windows 10 VPC in test mode with a modified Wintun driver installed that also allows ring registration for the members of Administrators group. This allows me to quickly test ideas while reviewing/upgrading Lev's work. I can run Visual Studio to compile openvpn.exe on my computer as an unprivileged user. Then have an elevated Remote Debugger running on the VPC and Visual Studio to remotely run openvpn.exe with debugger attached with a single F5 click.

Having to use OpenVPN GUI and iservice, or running openvpn.exe as service would require a lot more clicks to replace the binary, attach debugger etc.

As far as I am concerned, this elevation hack may be removed from the OpenVPN source code in the final release. However, mind that this would prevent people from running openvpn.exe+Wintun from the command line.

>>> +bool
>>> 
>>>> +impersonate_as_system()
>>>> +{
>>> 
>>> This is implemented by stealing the access token from
>>> winlogon.exe. I don't think such tricks belong to OpenVPN.
>>> It may also trip some anti-virus software.
>>> 
>>> That said, probably there are no "legitimate" ways of getting
>>> LOCAL SYSTEM rights on Windows without running a service.
>>> 
>>> Why does wintun require SYSTEM for using it? If there is a
>>> good reason for that, we should not let every admin
>>> user bypass it.
>> 
>> 
>> I'll defer it to Simon.

Unfortunately, I don't do security decisions in Wintun.

Wintun was originally designed for WireGuard. WireGuard is architectured to run all its tunnels as Windows services running as the SYSTEM user. Wintun's security is as tight as possible so the WireGuard can barely use it. I know a guy who is tempted to introduce a userspace binary code signature check to the Wintun. :)

Given the relative ease to get SYSTEM token just by being an elevated process — mind there's also a hack to get from non-elevated to elevated completely bypassing the UAC prompt as long as you are a member of Administrators — this SYSTEM restriction really doesn't provide considerable additional security compared to being a member of Administrators group.

>>> Those who really need to test OpenVPN with wintun from
>>> command prompt can use diagnostic tools available to get
>>> a cmd prompt as system (e.g., psexec). That also  makes
>>> it explicit that SYSTEM privilege is required.
>>> 
>>> In the longer run, we could provide a script to launch
>>> openvpn.exe using the interactive service. Modifying the
>>> automatic service to use interactive service for launching
>>> looks easy to do as well. Then, all privileged operations could
>>> be removed from openvpn core.
>> 
>> 
>> I think it is good not to break user experience and allow run openvpn as
>> an administrator without iservice using wintun at the expense on elevation
>> to system for single API call.
> 
> I have already said what I think of it. As an admin I wouldn't like to see
> users running processes that elevate to SYSTEM like this.

Selva, Windows is full of such hacks internally. :( This is no excuse for us doing the same of course. Just saying Windows is far from ideal world.

I am running OpenVPN on Windows using NSSM wrapper for years. I had a brief discussion on the Hackathon with Samuli about integrating SCM support directly into openvpn.exe (imagine --daemon for Windows):

sc create OpenVPN$MyTunnel binpath= "C:\Program Files\OpenVPN\bin\openvpn" --daemon --config "C:\Program Files\OpenVPN\config\MyTunnel.ovpn" --log "C:\Program Files\OpenVPN\log\MyTunnel.log" start= auto depend= dhcp
sc start OpenVPN$MyTunnel

This would install openvpn.exe as a Windows service and run it as the SYSTEM user — no need for iservice, no need for SYSTEM token hack. Other than me, perhaps it could cover at least some of the users now running openvpn.exe directly.

Regards,
Simon
<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi,<br class=""><div class="">
<div><br class=""></div></div><div><blockquote type="cite" class=""><div class=""><blockquote type="cite" style="font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" class="">TLDR:<br class="">(i) stealing SYSTEM access from winlogon.exe is not a good thing to do<br class=""></blockquote><br class=""><br class="">This doesn't happen for the majority of use cases - only when iservice is not used. We also<br class="">elevate only for the single DeviceIOControl call.<br class=""></blockquote><br style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">I understand. But stealing access token from winlogon.exe is a hack and not</span><br style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">something I would expect to see in a trustworthy executable. Diagnostic</span><br style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">and forensic tools may be justified in doing such things.</span><br style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""></div></blockquote><div><br class=""></div><div>Wintun has a hardcoded check to allow ring registration from the SYSTEM user only.</div><div><br class=""></div><div>To be honest: I am using a Windows 10 VPC in test mode with a modified Wintun driver installed that also allows ring registration for the members of Administrators group. This allows me to quickly test ideas&nbsp;<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">while reviewing/upgrading Lev's work. I can run Visual Studio to </span>compile openvpn.exe on my computer as an unprivileged user. Then have an elevated Remote Debugger running on the VPC and Visual Studio to&nbsp;<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">remotely</span>&nbsp;run openvpn.exe with debugger attached with a single F5 click.</div><div><br class=""></div><div>Having to use OpenVPN GUI and iservice, or running openvpn.exe as service would require a lot more clicks to replace the binary, attach debugger etc.</div><div><br class=""></div><div><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">As far as I am concerned, this elevation hack may be removed from the OpenVPN source code i</span><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">n the final release. However, mind that&nbsp;</span><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">this would prevent people from running openvpn.exe+Wintun from the command line.</span></div><div><br class=""></div><blockquote type="cite" class=""><div class=""><blockquote type="cite" style="font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" class="">+bool<br class=""><br class=""><blockquote type="cite" class="">+impersonate_as_system()<br class="">+{<br class=""></blockquote><br class="">This is implemented by stealing the access token from<br class="">winlogon.exe. I don't think such tricks belong to OpenVPN.<br class="">It may also trip some anti-virus software.<br class=""><br class="">That said, probably there are no "legitimate" ways of getting<br class="">LOCAL SYSTEM rights on Windows without running a service.<br class=""><br class="">Why does wintun require SYSTEM for using it? If there is a<br class="">good reason for that, we should not let every admin<br class="">user bypass it.<br class=""></blockquote><br class=""><br class="">I'll defer it to Simon.<br class=""></blockquote></div></blockquote><div><br class=""></div><div><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">Unfortunately, I don't do security decisions in Wintun.</span></div><div><span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class=""><br class=""></span></div><div>Wintun was originally designed for WireGuard. WireGuard is architectured to run all its tunnels as Windows services running as the SYSTEM user. Wintun's security is as tight as possible so the WireGuard can barely use it. I know a guy who is tempted to introduce a&nbsp;<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">userspace binary</span>&nbsp;code signature check to the Wintun. :)</div><div><br class=""></div><div>Given the relative ease to get SYSTEM token just by being an elevated process — mind there's also a hack to&nbsp;<font color="#000000" class=""><span style="caret-color: rgb(0, 0, 0);" class="">get from&nbsp;non-elevated to elevated completely </span></font>bypassing the UAC prompt as long as you are a member of Administrators&nbsp;<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">—</span>&nbsp;this SYSTEM restriction really doesn't provide considerable additional security compared to being a member of Administrators group.</div><div><br class=""></div><blockquote type="cite" class=""><div class=""><blockquote type="cite" style="font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" class="">Those who really need to test OpenVPN with wintun from<br class="">command prompt can use diagnostic tools available to get<br class="">a cmd prompt as system (e.g., psexec). That also &nbsp;makes<br class="">it explicit that SYSTEM privilege is required.<br class=""><br class="">In the longer run, we could provide a script to launch<br class="">openvpn.exe using the interactive service. Modifying the<br class="">automatic service to use interactive service for launching<br class="">looks easy to do as well. Then, all privileged operations could<br class="">be removed from openvpn core.<br class=""></blockquote><br class=""><br class="">I think it is good not to break user experience and allow run openvpn as<br class="">an administrator without iservice using wintun at the expense on elevation<br class="">to system for single API call.<br class=""></blockquote><br style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">I have already said what I think of it. As an admin I wouldn't like to see</span><br style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">users running processes that elevate to SYSTEM like this.</span><br style="caret-color: rgb(0, 0, 0); font-family: Menlo-Regular; font-size: 14px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""></div></blockquote><div><br class=""></div><div>Selva, Windows is full of such hacks internally. :( This is no excuse for us doing the same of course. Just saying Windows is far from ideal world.</div><div><br class=""></div><div>I am running OpenVPN on Windows using NSSM wrapper for years. I had a brief discussion on the Hackathon with Samuli about integrating SCM support directly into openvpn.exe (imagine --daemon for Windows):</div><div><br class=""></div><div>sc create OpenVPN$MyTunnel binpath= "C:\Program Files\OpenVPN\bin\openvpn" --daemon --config "<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">C:\Program Files\OpenVPN\config\</span>MyTunnel.ovpn" --log "<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">C:\Program Files\OpenVPN</span>\log\MyTunnel.log"&nbsp;start=&nbsp;auto&nbsp;depend= dhcp</div><div>sc start&nbsp;<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">OpenVPN$MyTunnel</span></div><div><br class=""></div><div>This would install openvpn.exe as a Windows service and run it as the SYSTEM user&nbsp;<span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0);" class="">—</span>&nbsp;no need for iservice, no need for SYSTEM token hack. Other than me, perhaps it could cover at least some of the users now running openvpn.exe directly.</div><div><br class=""></div><div>Regards,</div><div>Simon</div></div></body></html>

Selva Nair Dec. 16, 2019, 11:20 a.m. UTC | #7

Hi

On Mon, Dec 16, 2019 at 4:31 PM Lev Stipakov <lstipakov@gmail.com> wrote:
>>
>> I have already said what I think of it. As an admin I wouldn't like to see
>> users running processes that elevate to SYSTEM like this.
>
>
> Would it be too much if
>
>  - openvpn.exe process detects that it is not started by interactive service
>  - interactive service process is running
>  - wintun is used as --windows-driver
>

I think this is possible and in fact I had something like this in mind
as well I wrote we could provide an openvpn launcher script.

And I would like to do this even if wintun is in use as ability to run
openvpn.exe as a regular would be a plus for everyone.

> then openvpn behaves as a command-line version of openvpn-gui - connects to service, which
> runs openvpn.exe, and then prints output, received via management interface, to console?

It may be better to redirect the output to the stdout or file as usual
so that no
user visible changes and easier to implement too. Just keep the process id
returned from the service and when the user kills the front-end, terminate the
background process started by the service too.

This should also allow to run the automatic service as LocalService or a
special service user as many services do.

Selva

>
> --
> -Lev

Selva Nair Dec. 16, 2019, 11:38 a.m. UTC | #8

Hi,

On Mon, Dec 16, 2019 at 5:18 PM Simon Rozman <simon@rozman.si> wrote:
>
> Hi,
>
> TLDR:
> (i) stealing SYSTEM access from winlogon.exe is not a good thing to do
>
>
>
> This doesn't happen for the majority of use cases - only when iservice is not used. We also
> elevate only for the single DeviceIOControl call.
>
>
> I understand. But stealing access token from winlogon.exe is a hack and not
> something I would expect to see in a trustworthy executable. Diagnostic
> and forensic tools may be justified in doing such things.
>
>
> Wintun has a hardcoded check to allow ring registration from the SYSTEM user only.
>
> To be honest: I am using a Windows 10 VPC in test mode with a modified Wintun driver installed that also allows ring registration for the members of Administrators group. This allows me to quickly test ideas while reviewing/upgrading Lev's work. I can run Visual Studio to compile openvpn.exe on my computer as an unprivileged user. Then have an elevated Remote Debugger running on the VPC and Visual Studio to remotely run openvpn.exe with debugger attached with a single F5 click.
>
> Having to use OpenVPN GUI and iservice, or running openvpn.exe as service would require a lot more clicks to replace the binary, attach debugger etc.
>
> As far as I am concerned, this elevation hack may be removed from the OpenVPN source code in the final release. However, mind that this would prevent people from running openvpn.exe+Wintun from the command line.
>
> +bool
>
> +impersonate_as_system()
> +{
>
>
> This is implemented by stealing the access token from
> winlogon.exe. I don't think such tricks belong to OpenVPN.
> It may also trip some anti-virus software.
>
> That said, probably there are no "legitimate" ways of getting
> LOCAL SYSTEM rights on Windows without running a service.
>
> Why does wintun require SYSTEM for using it? If there is a
> good reason for that, we should not let every admin
> user bypass it.
>
>
>
> I'll defer it to Simon.
>
>
> Unfortunately, I don't do security decisions in Wintun.
>
> Wintun was originally designed for WireGuard. WireGuard is architectured to run all its tunnels as Windows services running as the SYSTEM user. Wintun's security is as tight as possible so the WireGuard can barely use it. I know a guy who is tempted to introduce a userspace binary code signature check to the Wintun. :)
>
> Given the relative ease to get SYSTEM token just by being an elevated process — mind there's also a hack to get from non-elevated to elevated completely bypassing the UAC prompt as long as you are a member of Administrators — this SYSTEM restriction really doesn't provide considerable additional security compared to being a member of Administrators group.

That's the point. It probably provides no extra security because we
are forced to take the blame for using hacks to cross security
boundaries. And, bypassing UAC is quite different from acquiring
rights you were not supposed to have. Does openvpn wants to be doing
that?

>
> Those who really need to test OpenVPN with wintun from
> command prompt can use diagnostic tools available to get
> a cmd prompt as system (e.g., psexec). That also  makes
> it explicit that SYSTEM privilege is required.
>
> In the longer run, we could provide a script to launch
> openvpn.exe using the interactive service. Modifying the
> automatic service to use interactive service for launching
> looks easy to do as well. Then, all privileged operations could
> be removed from openvpn core.
>
>
>
> I think it is good not to break user experience and allow run openvpn as
> an administrator without iservice using wintun at the expense on elevation
> to system for single API call.
>
>
> I have already said what I think of it. As an admin I wouldn't like to see
> users running processes that elevate to SYSTEM like this.
>
>
> Selva, Windows is full of such hacks internally. :( This is no excuse for us doing the same of course. Just saying Windows is far from ideal world.

In this particular case this has nothing to do with Windows -- its
just a stupid decision by wintun that requires the use of undocumented
hacks to elevate to SYSTEM.

>
> I am running OpenVPN on Windows using NSSM wrapper for years. I had a brief discussion on the Hackathon with Samuli about integrating SCM support directly into openvpn.exe (imagine --daemon for Windows):
>
> sc create OpenVPN$MyTunnel binpath= "C:\Program Files\OpenVPN\bin\openvpn" --daemon --config "C:\Program Files\OpenVPN\config\MyTunnel.ovpn" --log "C:\Program Files\OpenVPN\log\MyTunnel.log" start= auto depend= dhcp
> sc start OpenVPN$MyTunnel
>
> This would install openvpn.exe as a Windows service and run it as the SYSTEM user — no need for iservice, no need for SYSTEM token hack. Other than me, perhaps it could cover at least some of the users now running openvpn.exe directly.

This is not the direction I want to see us moving towards. Instead I
want to see us daemonizing OpenVPN.exe as LOCAL SERVICE or a custom
service user and delegate privileged operations to a separate service
running as SYSTEM. And we already have the latter: interactive
service. So, not even admin rights is needed in openvpn.exe, let alone
SYSTEM.

IMO, the right approach on Windows is to run a bare minimal code as a
service to get SYSTEM rights and the rest with limited privileges.

Selva

Lev Stipakov Dec. 16, 2019, 10:34 p.m. UTC | #9

How about compromise - let's add  "--enable-system-elevation" windows
specific option.

 - When it is set, we print warning and elevate to SYSTEM for the single
DeviceIOControl call

 - When it is not set and wintun is used, we run openvpn from command line
via iservice

 - If service is missing, we print error "either use iservice or
--enable-system-elevation (experts only)"

The ones who want to run openvpn from command line with wintun still could
do it via psexec and it is
assumed that they know what they are doing. So why not make it simpler and
add a option
"yes I know what I am doing and I am willing to take risks"? Also, it is
safer to use system privilege
just for single call rather than run whole process with it.

This also will make debugging / development easier.


ti 17. jouluk. 2019 klo 0.38 Selva Nair (selva.nair@gmail.com) kirjoitti:

> Hi,
>
> On Mon, Dec 16, 2019 at 5:18 PM Simon Rozman <simon@rozman.si> wrote:
> >
> > Hi,
> >
> > TLDR:
> > (i) stealing SYSTEM access from winlogon.exe is not a good thing to do
> >
> >
> >
> > This doesn't happen for the majority of use cases - only when iservice
> is not used. We also
> > elevate only for the single DeviceIOControl call.
> >
> >
> > I understand. But stealing access token from winlogon.exe is a hack and
> not
> > something I would expect to see in a trustworthy executable. Diagnostic
> > and forensic tools may be justified in doing such things.
> >
> >
> > Wintun has a hardcoded check to allow ring registration from the SYSTEM
> user only.
> >
> > To be honest: I am using a Windows 10 VPC in test mode with a modified
> Wintun driver installed that also allows ring registration for the members
> of Administrators group. This allows me to quickly test ideas while
> reviewing/upgrading Lev's work. I can run Visual Studio to compile
> openvpn.exe on my computer as an unprivileged user. Then have an elevated
> Remote Debugger running on the VPC and Visual Studio to remotely run
> openvpn.exe with debugger attached with a single F5 click.
> >
> > Having to use OpenVPN GUI and iservice, or running openvpn.exe as
> service would require a lot more clicks to replace the binary, attach
> debugger etc.
> >
> > As far as I am concerned, this elevation hack may be removed from the
> OpenVPN source code in the final release. However, mind that this would
> prevent people from running openvpn.exe+Wintun from the command line.
> >
> > +bool
> >
> > +impersonate_as_system()
> > +{
> >
> >
> > This is implemented by stealing the access token from
> > winlogon.exe. I don't think such tricks belong to OpenVPN.
> > It may also trip some anti-virus software.
> >
> > That said, probably there are no "legitimate" ways of getting
> > LOCAL SYSTEM rights on Windows without running a service.
> >
> > Why does wintun require SYSTEM for using it? If there is a
> > good reason for that, we should not let every admin
> > user bypass it.
> >
> >
> >
> > I'll defer it to Simon.
> >
> >
> > Unfortunately, I don't do security decisions in Wintun.
> >
> > Wintun was originally designed for WireGuard. WireGuard is architectured
> to run all its tunnels as Windows services running as the SYSTEM user.
> Wintun's security is as tight as possible so the WireGuard can barely use
> it. I know a guy who is tempted to introduce a userspace binary code
> signature check to the Wintun. :)
> >
> > Given the relative ease to get SYSTEM token just by being an elevated
> process — mind there's also a hack to get from non-elevated to elevated
> completely bypassing the UAC prompt as long as you are a member of
> Administrators — this SYSTEM restriction really doesn't provide
> considerable additional security compared to being a member of
> Administrators group.
>
> That's the point. It probably provides no extra security because we
> are forced to take the blame for using hacks to cross security
> boundaries. And, bypassing UAC is quite different from acquiring
> rights you were not supposed to have. Does openvpn wants to be doing
> that?
>
> >
> > Those who really need to test OpenVPN with wintun from
> > command prompt can use diagnostic tools available to get
> > a cmd prompt as system (e.g., psexec). That also  makes
> > it explicit that SYSTEM privilege is required.
> >
> > In the longer run, we could provide a script to launch
> > openvpn.exe using the interactive service. Modifying the
> > automatic service to use interactive service for launching
> > looks easy to do as well. Then, all privileged operations could
> > be removed from openvpn core.
> >
> >
> >
> > I think it is good not to break user experience and allow run openvpn as
> > an administrator without iservice using wintun at the expense on
> elevation
> > to system for single API call.
> >
> >
> > I have already said what I think of it. As an admin I wouldn't like to
> see
> > users running processes that elevate to SYSTEM like this.
> >
> >
> > Selva, Windows is full of such hacks internally. :( This is no excuse
> for us doing the same of course. Just saying Windows is far from ideal
> world.
>
> In this particular case this has nothing to do with Windows -- its
> just a stupid decision by wintun that requires the use of undocumented
> hacks to elevate to SYSTEM.
>
> >
> > I am running OpenVPN on Windows using NSSM wrapper for years. I had a
> brief discussion on the Hackathon with Samuli about integrating SCM support
> directly into openvpn.exe (imagine --daemon for Windows):
> >
> > sc create OpenVPN$MyTunnel binpath= "C:\Program
> Files\OpenVPN\bin\openvpn" --daemon --config "C:\Program
> Files\OpenVPN\config\MyTunnel.ovpn" --log "C:\Program
> Files\OpenVPN\log\MyTunnel.log" start= auto depend= dhcp
> > sc start OpenVPN$MyTunnel
> >
> > This would install openvpn.exe as a Windows service and run it as the
> SYSTEM user — no need for iservice, no need for SYSTEM token hack. Other
> than me, perhaps it could cover at least some of the users now running
> openvpn.exe directly.
>
> This is not the direction I want to see us moving towards. Instead I
> want to see us daemonizing OpenVPN.exe as LOCAL SERVICE or a custom
> service user and delegate privileged operations to a separate service
> running as SYSTEM. And we already have the latter: interactive
> service. So, not even admin rights is needed in openvpn.exe, let alone
> SYSTEM.
>
> IMO, the right approach on Windows is to run a bare minimal code as a
> service to get SYSTEM rights and the rest with limited privileges.
>
> Selva
>

Simon Rozman Dec. 17, 2019, 12:09 a.m. UTC | #10

Hi,

> > I am running OpenVPN on Windows using NSSM wrapper for years. I had a
> brief discussion on the Hackathon with Samuli about integrating SCM
> support directly into openvpn.exe (imagine --daemon for Windows):
> >
> > sc create OpenVPN$MyTunnel binpath= "C:\Program
> > Files\OpenVPN\bin\openvpn" --daemon --config "C:\Program
> > Files\OpenVPN\config\MyTunnel.ovpn" --log "C:\Program
> > Files\OpenVPN\log\MyTunnel.log" start= auto depend= dhcp sc start
> > OpenVPN$MyTunnel
> >
> > This would install openvpn.exe as a Windows service and run it as the
> SYSTEM user — no need for iservice, no need for SYSTEM token hack. Other
> than me, perhaps it could cover at least some of the users now running
> openvpn.exe directly.
> 
> This is not the direction I want to see us moving towards. Instead I
> want to see us daemonizing OpenVPN.exe as LOCAL SERVICE or a custom
> service user and delegate privileged operations to a separate service
> running as SYSTEM. And we already have the latter: interactive service.
> So, not even admin rights is needed in openvpn.exe, let alone SYSTEM.
> 
> IMO, the right approach on Windows is to run a bare minimal code as a
> service to get SYSTEM rights and the rest with limited privileges.

Selva, those are two different use-cases. And none is "right" or "wrong". OpenVPN can or should have both. :)

1. I need to run VPN tunnel as a persistent service - something that comes up with computer (Group Policy Client service waits for about 30 seconds on boot to get network access to AD server). And stays on all the time - any user signed in or not. I connect computers with VPN.

2. You need an openvpn.exe (or introduce some openvpn-tui.exe) to be a command line version of openvpn-gui.exe. Something to be run by a regular user or a batch script in an unprivileged context. You connect users with VPN.

If we implement both, we can obsolete SYSTEM token hack completely.

I have been playing with Lev's patches for the past few days. Tested them, debugged them, did some fixes. There are things to be desired like netsh=>ipcfg, remove or #ifdef the SYSTEM token hack... But those are design choices we should pursue in the future. I believe patches are mature enough to ack them. They should be merged into master to provide wider testing and easier development progress.

Regards,
Simon

Simon Rozman Dec. 17, 2019, 12:18 a.m. UTC | #11

Hi,



Lev, unfortunately, the openvpn.exe binary will still contain this hack's 
machine code and might really rise some eyebrows with anti-virus software.



Before the final release the entire SYSTEM token hack should be removed from 
the OpenVPN source.



I think it's okay to make a note in README that "--windows-driver wintun" 
works with iservice only. (Or, if user is responsible to run openvpn.exe as 
SYSTEM user himself somehow.)



Regards,

Simon



From: Lev Stipakov <lstipakov@gmail.com>
Sent: Tuesday, December 17, 2019 10:35 AM
To: Selva Nair <selva.nair@gmail.com>
Cc: Simon Rozman <simon@rozman.si>; Lev Stipakov <lev@openvpn.net>; 
openvpn-devel <openvpn-devel@lists.sourceforge.net>
Subject: Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O



How about compromise - let's add  "--enable-system-elevation" windows specific 
option.



 - When it is set, we print warning and elevate to SYSTEM for the single 
DeviceIOControl call



 - When it is not set and wintun is used, we run openvpn from command line via 
iservice



 - If service is missing, we print error "either use iservice 
or --enable-system-elevation (experts only)"



The ones who want to run openvpn from command line with wintun still could do 
it via psexec and it is

assumed that they know what they are doing. So why not make it simpler and add 
a option

"yes I know what I am doing and I am willing to take risks"? Also, it is safer 
to use system privilege

just for single call rather than run whole process with it.



This also will make debugging / development easier.
<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 15 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
	{font-family:"Cambria Math";
	panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
	{font-family:Calibri;
	panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
	{margin:0cm;
	margin-bottom:.0001pt;
	font-size:11.0pt;
	font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
	{mso-style-priority:99;
	color:blue;
	text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
	{mso-style-priority:99;
	color:purple;
	text-decoration:underline;}
p.Code, li.Code, div.Code
	{mso-style-name:Code;
	margin:0cm;
	margin-bottom:.0001pt;
	font-size:11.0pt;
	font-family:"Courier New";}
p.msonormal0, li.msonormal0, div.msonormal0
	{mso-style-name:msonormal;
	mso-margin-top-alt:auto;
	margin-right:0cm;
	mso-margin-bottom-alt:auto;
	margin-left:0cm;
	font-size:11.0pt;
	font-family:"Calibri",sans-serif;}
span.EmailStyle19
	{mso-style-type:personal-reply;
	font-family:"Calibri",sans-serif;
	color:windowtext;}
.MsoChpDefault
	{mso-style-type:export-only;
	font-family:"Calibri",sans-serif;
	mso-fareast-language:EN-US;}
@page WordSection1
	{size:612.0pt 792.0pt;
	margin:70.85pt 70.85pt 70.85pt 70.85pt;}
div.WordSection1
	{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body lang=SL link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'>Hi,<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'><o:p>&nbsp;</o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'>Lev, unfortunately, the openvpn.exe binary will still contain this hack's machine code and might really rise some eyebrows with anti-virus software.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'><o:p>&nbsp;</o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'>Before the final release the entire SYSTEM token hack should be removed from the OpenVPN source.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'><o:p>&nbsp;</o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'>I think it's okay to make a note in README that &quot;--windows-driver wintun&quot; works with iservice only. (Or, if user is responsible to run openvpn.exe as SYSTEM user himself somehow.)<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'><o:p>&nbsp;</o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'>Regards,<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'>Simon<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'><o:p>&nbsp;</o:p></span></p><div style='border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt'><div><div style='border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm'><p class=MsoNormal><b><span lang=EN-GB>From:</span></b><span lang=EN-GB> Lev Stipakov &lt;lstipakov@gmail.com&gt; <br><b>Sent:</b> Tuesday, December 17, 2019 10:35 AM<br><b>To:</b> Selva Nair &lt;selva.nair@gmail.com&gt;<br><b>Cc:</b> Simon Rozman &lt;simon@rozman.si&gt;; Lev Stipakov &lt;lev@openvpn.net&gt;; openvpn-devel &lt;openvpn-devel@lists.sourceforge.net&gt;<br><b>Subject:</b> Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O<o:p></o:p></span></p></div></div><p class=MsoNormal><span lang=EN-GB><o:p>&nbsp;</o:p></span></p><div><p class=MsoNormal><span lang=EN-GB>How about compromise - let's add&nbsp; &quot;--enable-system-elevation&quot; windows specific option.<o:p></o:p></span></p><div><p class=MsoNormal><span lang=EN-GB><o:p>&nbsp;</o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB>&nbsp;- When it is set, we print warning and elevate to SYSTEM for the single DeviceIOControl call<o:p></o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB><o:p>&nbsp;</o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB>&nbsp;- When it is not set and wintun is used, we run openvpn from command line via iservice<o:p></o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB><o:p>&nbsp;</o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB>&nbsp;- If service is missing, we print error &quot;either use iservice or --enable-system-elevation (experts only)&quot;<o:p></o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB><o:p>&nbsp;</o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB>The ones who want to run openvpn from command line with wintun still could do it via psexec and it is<o:p></o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB>assumed that they know what they are doing. So why not make it simpler and add a option<o:p></o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB>&quot;yes I know what I am doing and I am willing to take risks&quot;? Also, it is safer to use system privilege<o:p></o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB>just for single call rather than run whole process with it.<o:p></o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB><o:p>&nbsp;</o:p></span></p></div><div><p class=MsoNormal><span lang=EN-GB>This also will make debugging / development easier.<o:p></o:p></span></p></div></div></div></div></body></html>

Lev Stipakov Dec. 17, 2019, 1:21 a.m. UTC | #12

I don't want to give up :)

Let's at least wrap that code inside #define ENABLE_SYSTEM_ELEVATION
(because we don't have enough defines) to not to complicate development /
debugging.

ti 17. jouluk. 2019 klo 13.18 Simon Rozman (simon@rozman.si) kirjoitti:

> Hi,
>
>
>
> Lev, unfortunately, the openvpn.exe binary will still contain this hack's
> machine code and might really rise some eyebrows with anti-virus software.
>
>
>
> Before the final release the entire SYSTEM token hack should be removed
> from the OpenVPN source.
>
>
>
> I think it's okay to make a note in README that "--windows-driver wintun"
> works with iservice only. (Or, if user is responsible to run openvpn.exe as
> SYSTEM user himself somehow.)
>
>
>
> Regards,
>
> Simon
>
>
>
> *From:* Lev Stipakov <lstipakov@gmail.com>
> *Sent:* Tuesday, December 17, 2019 10:35 AM
> *To:* Selva Nair <selva.nair@gmail.com>
> *Cc:* Simon Rozman <simon@rozman.si>; Lev Stipakov <lev@openvpn.net>;
> openvpn-devel <openvpn-devel@lists.sourceforge.net>
> *Subject:* Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based
> I/O
>
>
>
> How about compromise - let's add  "--enable-system-elevation" windows
> specific option.
>
>
>
>  - When it is set, we print warning and elevate to SYSTEM for the single
> DeviceIOControl call
>
>
>
>  - When it is not set and wintun is used, we run openvpn from command line
> via iservice
>
>
>
>  - If service is missing, we print error "either use iservice or
> --enable-system-elevation (experts only)"
>
>
>
> The ones who want to run openvpn from command line with wintun still could
> do it via psexec and it is
>
> assumed that they know what they are doing. So why not make it simpler and
> add a option
>
> "yes I know what I am doing and I am willing to take risks"? Also, it is
> safer to use system privilege
>
> just for single call rather than run whole process with it.
>
>
>
> This also will make debugging / development easier.
>

Selva Nair Dec. 17, 2019, 2:34 a.m. UTC | #13

Hi Simon,

A quick reply:

> > IMO, the right approach on Windows is to run a bare minimal code as a
> > service to get SYSTEM rights and the rest with limited privileges.
>
> Selva, those are two different use-cases. And none is "right" or "wrong". OpenVPN can or should have both. :)
>
> 1. I need to run VPN tunnel as a persistent service - something that comes up with computer (Group Policy Client service waits for about 30 seconds on boot to get network access to AD server). And stays on all the time - any user signed in or not. I connect computers with VPN.

I too use OpenVPN like this, so I do understand the use case. And, the
point was that the exe can be started through interactive service even
in this case. That would allow running openvpn.exe at boot from a
service with low privileges that delegates all privileged actions to
iservice. Years ago when iservice was introduced we did briefly
discuss this (with Heiko) but left it as a future enhancement which of
course no one had time for.

Unless I'm missing some scenario where this wont work. Anyway, this is
beyond the scope of the current patch.

Selva

Selva Nair Dec. 17, 2019, 4:15 a.m. UTC | #14

Hi

On Tue, Dec 17, 2019 at 6:09 AM Simon Rozman <simon@rozman.si> wrote:
>
> I have been playing with Lev's patches for the past few days. Tested them, debugged them, did some fixes. There are things to be desired like netsh=>ipcfg, remove or #ifdef the SYSTEM token hack... But those are design choices we should pursue in the future. I believe patches are mature enough to ack them. They should be merged into master to provide wider testing and easier development progress.

I agree. And, if we wont release official binaries with the system
hack, the patch looks good to me too.

Selva

[Openvpn-devel,v6,4/7] wintun: ring buffers based I/O

Commit Message

Comments

Patch