Hi,
On Thu, Jul 18, 2019 at 7:42 AM Lev Stipakov <lstipakov@gmail.com> wrote:
> From: Lev Stipakov <lev@openvpn.net>
>
> This patch enables interactive service to open tun device.
> This is mostly needed by Wintun, which could be opened
> only by privileged process.
>
> When interactive service is used, instead of calling
> CreateFile() directly by openvpn process we pass tun device path
> into service process. There we open device, duplicate handle
> and pass it back to openvpn process.
>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
> v5:
> - futher strengthen security by passing only device guid from client
> process
>
"further"
> to service, validating guid and contructing device path on service side
>
"constructing"
>
> v4:
> - strengthen security by adding basic validation to device path
> - reorder fields in msg_open_tun_device_result for backward compatibility
>
> v3:
> - ensure that device path passed by client is null-terminated
> - support for multiple openvpn processes
> - return proper error code when device handle is invalid
>
> v2:
> - introduce send_msg_iservice_ex() instead of changing
> signature of existing send_msg_iservice()
> - use wchar_t strings in interactive service code
>
> include/openvpn-msg.h | 12 +++++++
> src/openvpn/tun.c | 83
> ++++++++++++++++++++++++++++++++++---------
> src/openvpn/win32.c | 9 ++++-
> src/openvpn/win32.h | 30 +++++++++++++---
> src/openvpnserv/interactive.c | 83
> +++++++++++++++++++++++++++++++++++++++++--
> 5 files changed, 193 insertions(+), 24 deletions(-)
diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
> index 4814bbc..8ccddc0 100644
> --- a/src/openvpn/win32.h
> +++ b/src/openvpn/win32.h
> @@ -309,14 +309,36 @@ int win32_version_info(void);
> */
> const char *win32_version_string(struct gc_arena *gc, bool add_name);
>
> -/*
> - * Send the |size| bytes in buffer |data| to the interactive service
> |pipe|
> - * and read the result in |ack|. Returns false on communication error.
> - * The string in |context| is used to prefix error messages.
> +
> +/**
> + * Send data to interactive service and receive response of type
> ack_message_t.
> + *
> + * @param pipe The handle of communication pipe
> + * @param data The data to send
> + * @param size The size of data to send
> + * @param response The pointer to ack_message_t structure to where
> response is written
>
That would be "@param ack"
> + * @param context The string used to prefix error messages
> + *
> + * @returns True on success, false on failure.
> */
> bool send_msg_iservice(HANDLE pipe, const void *data, size_t size,
> ack_message_t *ack, const char *context);
>
>
>
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 623c3ff..782c04d 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -58,7 +58,6 @@ static settings_t settings;
> static HANDLE rdns_semaphore = NULL;
> #define RDNS_TIMEOUT 600 /* seconds to wait for the semaphore */
>
> -
> openvpn_service_t interactive_service = {
> interactive,
> TEXT(PACKAGE_NAME "ServiceInteractive"),
> @@ -1198,8 +1197,67 @@ HandleEnableDHCPMessage(const enable_dhcp_message_t
> *dhcp)
> return err;
> }
>
> +static DWORD
> +HandleOpenTunDeviceMessage(const open_tun_device_message_t *open_tun,
> HANDLE ovpn_proc, HANDLE *remote_handle)
> +{
> + DWORD err = ERROR_SUCCESS;
> + HANDLE local_handle;
> + LPWSTR wguid = NULL;
> + WCHAR device_path[256] = {0};
> + const WCHAR prefix[] = L"\\\\.\\Global\\";
> + const WCHAR tap_suffix[] = L".tap";
> +
> + *remote_handle = INVALID_HANDLE_VALUE;
> +
> + wguid = utf8to16(open_tun->device_guid);
> + if (!wguid)
> + {
> + err = ERROR_OUTOFMEMORY;
> + goto out;
> + }
> +
> + /* validate device guid */
> + const size_t guid_len = wcslen(wguid);
> + for (int i = 0; i < guid_len; ++i)
> + {
> + const WCHAR ch = wguid[i];
> +
> + if (!(iswalnum(ch)) && (ch != L'-') && (ch != L'{') && (ch !=
> L'}'))
> + {
> + err = ERROR_MESSAGE_DATA;
> + MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Invalid device guild
> (%s)"), wguid);
> + goto out;
> + }
> + }
>
I think the check could be made tighter than alpha numerics (only hex
digits need be allowed) and simpler like:
if (guid_len != 38 || wcspn(wguid, L"0123456789ABCDEFabcdef-{}") !=
guid_len)
{
<handle the error and goto out>
}
I would have preferred to do a GUID parsing to validate it, but may be this
is good enough?
+
> + openvpn_swprintf(device_path, sizeof(device_path), L"%s%s%s", prefix,
> wguid, tap_suffix);
> +
> + /* open tun device */
> +
>
...
Thanks,
Selva
<div dir="ltr"><div dir="ltr">Hi,</div><div><br></div><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 18, 2019 at 7:42 AM Lev Stipakov <<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br>
<br>
This patch enables interactive service to open tun device.<br>
This is mostly needed by Wintun, which could be opened<br>
only by privileged process.<br>
<br>
When interactive service is used, instead of calling<br>
CreateFile() directly by openvpn process we pass tun device path<br>
into service process. There we open device, duplicate handle<br>
and pass it back to openvpn process.<br>
<br>
Signed-off-by: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br>
---<br>
v5:<br>
- futher strengthen security by passing only device guid from client process<br></blockquote><div><br></div><div>"further"</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
to service, validating guid and contructing device path on service side <br></blockquote><div><br></div><div>"constructing"</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
v4:<br>
- strengthen security by adding basic validation to device path<br>
- reorder fields in msg_open_tun_device_result for backward compatibility<br>
<br>
v3:<br>
- ensure that device path passed by client is null-terminated<br>
- support for multiple openvpn processes<br>
- return proper error code when device handle is invalid<br>
<br>
v2:<br>
- introduce send_msg_iservice_ex() instead of changing<br>
signature of existing send_msg_iservice()<br>
- use wchar_t strings in interactive service code<br>
<br>
include/openvpn-msg.h | 12 +++++++<br>
src/openvpn/tun.c | 83 ++++++++++++++++++++++++++++++++++---------<br>
src/openvpn/win32.c | 9 ++++-<br>
src/openvpn/win32.h | 30 +++++++++++++---<br>
src/openvpnserv/interactive.c | 83 +++++++++++++++++++++++++++++++++++++++++--<br>
5 files changed, 193 insertions(+), 24 deletions(-)</blockquote><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h<br>
index 4814bbc..8ccddc0 100644<br>
--- a/src/openvpn/win32.h<br>
+++ b/src/openvpn/win32.h<br>
@@ -309,14 +309,36 @@ int win32_version_info(void);<br>
*/<br>
const char *win32_version_string(struct gc_arena *gc, bool add_name);<br>
<br>
-/*<br>
- * Send the |size| bytes in buffer |data| to the interactive service |pipe|<br>
- * and read the result in |ack|. Returns false on communication error.<br>
- * The string in |context| is used to prefix error messages.<br>
+<br>
+/**<br>
+ * Send data to interactive service and receive response of type ack_message_t.<br>
+ *<br>
+ * @param pipe The handle of communication pipe<br>
+ * @param data The data to send<br>
+ * @param size The size of data to send<br>
+ * @param response The pointer to ack_message_t structure to where response is written<br></blockquote><div><br></div><div>That would be "@param ack"</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ * @param context The string used to prefix error messages<br>
+ *<br>
+ * @returns True on success, false on failure.<br>
*/<br>
bool send_msg_iservice(HANDLE pipe, const void *data, size_t size,<br>
ack_message_t *ack, const char *context);<br>
<br></blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c<br>
index 623c3ff..782c04d 100644<br>
--- a/src/openvpnserv/interactive.c<br>
+++ b/src/openvpnserv/interactive.c<br>
@@ -58,7 +58,6 @@ static settings_t settings;<br>
static HANDLE rdns_semaphore = NULL;<br>
#define RDNS_TIMEOUT 600 /* seconds to wait for the semaphore */<br>
<br>
-<br>
openvpn_service_t interactive_service = {<br>
interactive,<br>
TEXT(PACKAGE_NAME "ServiceInteractive"),<br>
@@ -1198,8 +1197,67 @@ HandleEnableDHCPMessage(const enable_dhcp_message_t *dhcp)<br>
return err;<br>
}<br>
<br>
+static DWORD<br>
+HandleOpenTunDeviceMessage(const open_tun_device_message_t *open_tun, HANDLE ovpn_proc, HANDLE *remote_handle)<br>
+{<br>
+ DWORD err = ERROR_SUCCESS;<br>
+ HANDLE local_handle;<br>
+ LPWSTR wguid = NULL;<br>
+ WCHAR device_path[256] = {0};<br>
+ const WCHAR prefix[] = L"\\\\.\\Global\\";<br>
+ const WCHAR tap_suffix[] = L".tap";<br>
+<br>
+ *remote_handle = INVALID_HANDLE_VALUE;<br>
+<br>
+ wguid = utf8to16(open_tun->device_guid);<br>
+ if (!wguid)<br>
+ {<br>
+ err = ERROR_OUTOFMEMORY;<br>
+ goto out;<br>
+ }<br>
+<br>
+ /* validate device guid */<br>
+ const size_t guid_len = wcslen(wguid);<br>
+ for (int i = 0; i < guid_len; ++i)<br>
+ {<br>
+ const WCHAR ch = wguid[i];<br>
+<br>
+ if (!(iswalnum(ch)) && (ch != L'-') && (ch != L'{') && (ch != L'}'))<br>
+ {<br>
+ err = ERROR_MESSAGE_DATA;<br>
+ MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Invalid device guild (%s)"), wguid);<br>
+ goto out;<br>
+ }<br>
+ }<br></blockquote><div><br></div><div>I think the check could be made tighter than alpha numerics (only hex digits need be allowed) and simpler like:</div><div><br></div><div>if (guid_len != 38 || wcspn(wguid, L"0123456789ABCDEFabcdef-{}") != guid_len)</div><div>{</div><div> <handle the error and goto out></div><div>} </div><div><br></div><div>I would have preferred to do a GUID parsing to validate it, but may be this is good enough?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ <br>
+ openvpn_swprintf(device_path, sizeof(device_path), L"%s%s%s", prefix, wguid, tap_suffix);<br>
+<br>
+ /* open tun device */<br>
+ <br></blockquote><div><br></div><div>...</div><div><br></div><div>Thanks,</div><div><br></div><div>Selva</div></div></div></div>
@@ -39,6 +39,8 @@ typedef enum {
msg_del_block_dns,
msg_register_dns,
msg_enable_dhcp,
+ msg_open_tun_device,
+ msg_open_tun_device_result,
} message_type_t;
typedef struct {
@@ -117,4 +119,14 @@ typedef struct {
interface_t iface;
} enable_dhcp_message_t;
+typedef struct {
+ message_header_t header;
+ char device_guid[36+2+1]; /* length of guid with dashes + braces + nul terminator */
+} open_tun_device_message_t;
+
+typedef struct {
+ message_header_t header;
+ int error_number;
+ HANDLE handle;
+} open_tun_device_result_message_t;
#endif /* ifndef OPENVPN_MSG_H_ */
@@ -5248,6 +5248,43 @@ out:
return ret;
}
+static HANDLE
+service_open_tun_device(const HANDLE pipe, const char* device_guid)
+{
+ open_tun_device_result_message_t result_msg;
+ struct gc_arena gc = gc_new();
+ open_tun_device_message_t open_tun_device = {
+ .header = {
+ msg_open_tun_device,
+ sizeof(open_tun_device_message_t),
+ 0
+ }
+ };
+ result_msg.handle = INVALID_HANDLE_VALUE;
+
+ strncpynt(open_tun_device.device_guid, device_guid, sizeof(open_tun_device.device_guid));
+
+ if (!send_msg_iservice_ex(pipe, &open_tun_device, sizeof(open_tun_device),
+ &result_msg, sizeof(result_msg), "Open_tun_device"))
+ {
+ goto out;
+ }
+
+ if (result_msg.error_number != NO_ERROR)
+ {
+ msg(D_TUNTAP_INFO, "TUN: opening tun handle using service failed: %s [status=%u device_path=%s]",
+ strerror_win32(result_msg.error_number, &gc), result_msg.error_number, device_guid);
+ }
+ else
+ {
+ msg(M_INFO, "Opened tun device %s using service", device_guid);
+ }
+
+out:
+ gc_free(&gc);
+ return result_msg.handle;
+}
+
/*
* Return a TAP name for netsh commands.
*/
@@ -5591,15 +5628,22 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
device_guid,
TAP_WIN_SUFFIX);
- tt->hand = CreateFile(
- device_path,
- GENERIC_READ | GENERIC_WRITE,
- 0, /* was: FILE_SHARE_READ */
- 0,
- OPEN_EXISTING,
- FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED,
- 0
+ if (tt->options.msg_channel)
+ {
+ tt->hand = service_open_tun_device(tt->options.msg_channel, device_guid);
+ }
+ else
+ {
+ tt->hand = CreateFile(
+ device_path,
+ GENERIC_READ | GENERIC_WRITE,
+ 0, /* was: FILE_SHARE_READ */
+ 0,
+ OPEN_EXISTING,
+ FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED,
+ 0
);
+ }
if (tt->hand == INVALID_HANDLE_VALUE)
{
@@ -5631,15 +5675,22 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
device_guid,
TAP_WIN_SUFFIX);
- tt->hand = CreateFile(
- device_path,
- GENERIC_READ | GENERIC_WRITE,
- 0, /* was: FILE_SHARE_READ */
- 0,
- OPEN_EXISTING,
- FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED,
- 0
+ if (tt->options.msg_channel)
+ {
+ tt->hand = service_open_tun_device(tt->options.msg_channel, device_guid);
+ }
+ else
+ {
+ tt->hand = CreateFile(
+ device_path,
+ GENERIC_READ | GENERIC_WRITE,
+ 0, /* was: FILE_SHARE_READ */
+ 0,
+ OPEN_EXISTING,
+ FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED,
+ 0
);
+ }
if (tt->hand == INVALID_HANDLE_VALUE)
{
@@ -1476,12 +1476,19 @@ bool
send_msg_iservice(HANDLE pipe, const void *data, size_t size,
ack_message_t *ack, const char *context)
{
+ return send_msg_iservice_ex(pipe, data, size, ack, sizeof(*ack), context);
+}
+
+bool
+send_msg_iservice_ex(HANDLE pipe, const void *data, size_t size,
+ void *response, size_t response_size, const char *context)
+{
struct gc_arena gc = gc_new();
DWORD len;
bool ret = true;
if (!WriteFile(pipe, data, size, &len, NULL)
- || !ReadFile(pipe, ack, sizeof(*ack), &len, NULL))
+ || !ReadFile(pipe, response, response_size, &len, NULL))
{
msg(M_WARN, "%s: could not talk to service: %s [%lu]",
context ? context : "Unknown",
@@ -309,14 +309,36 @@ int win32_version_info(void);
*/
const char *win32_version_string(struct gc_arena *gc, bool add_name);
-/*
- * Send the |size| bytes in buffer |data| to the interactive service |pipe|
- * and read the result in |ack|. Returns false on communication error.
- * The string in |context| is used to prefix error messages.
+
+/**
+ * Send data to interactive service and receive response of type ack_message_t.
+ *
+ * @param pipe The handle of communication pipe
+ * @param data The data to send
+ * @param size The size of data to send
+ * @param response The pointer to ack_message_t structure to where response is written
+ * @param context The string used to prefix error messages
+ *
+ * @returns True on success, false on failure.
*/
bool send_msg_iservice(HANDLE pipe, const void *data, size_t size,
ack_message_t *ack, const char *context);
+/**
+ * Send data to interactive service and receive response.
+ *
+ * @param pipe The handle of communication pipe
+ * @param data The data to send
+ * @param size The size of data to send
+ * @param response The buffer to where response is written
+ * @param response_size The size of response buffer
+ * @param context The string used to prefix error messages
+ *
+ * @returns True on success, false on failure.
+ */
+bool send_msg_iservice_ex(HANDLE pipe, const void *data, size_t size,
+ void *response, size_t response_size, const char *context);
+
/*
* Attempt to simulate fork/execve on Windows
*/
@@ -58,7 +58,6 @@ static settings_t settings;
static HANDLE rdns_semaphore = NULL;
#define RDNS_TIMEOUT 600 /* seconds to wait for the semaphore */
-
openvpn_service_t interactive_service = {
interactive,
TEXT(PACKAGE_NAME "ServiceInteractive"),
@@ -1198,8 +1197,67 @@ HandleEnableDHCPMessage(const enable_dhcp_message_t *dhcp)
return err;
}
+static DWORD
+HandleOpenTunDeviceMessage(const open_tun_device_message_t *open_tun, HANDLE ovpn_proc, HANDLE *remote_handle)
+{
+ DWORD err = ERROR_SUCCESS;
+ HANDLE local_handle;
+ LPWSTR wguid = NULL;
+ WCHAR device_path[256] = {0};
+ const WCHAR prefix[] = L"\\\\.\\Global\\";
+ const WCHAR tap_suffix[] = L".tap";
+
+ *remote_handle = INVALID_HANDLE_VALUE;
+
+ wguid = utf8to16(open_tun->device_guid);
+ if (!wguid)
+ {
+ err = ERROR_OUTOFMEMORY;
+ goto out;
+ }
+
+ /* validate device guid */
+ const size_t guid_len = wcslen(wguid);
+ for (int i = 0; i < guid_len; ++i)
+ {
+ const WCHAR ch = wguid[i];
+
+ if (!(iswalnum(ch)) && (ch != L'-') && (ch != L'{') && (ch != L'}'))
+ {
+ err = ERROR_MESSAGE_DATA;
+ MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Invalid device guild (%s)"), wguid);
+ goto out;
+ }
+ }
+
+ openvpn_swprintf(device_path, sizeof(device_path), L"%s%s%s", prefix, wguid, tap_suffix);
+
+ /* open tun device */
+ local_handle = CreateFileW(device_path, GENERIC_READ | GENERIC_WRITE, 0, 0,
+ OPEN_EXISTING, FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED, 0);
+ if (local_handle == INVALID_HANDLE_VALUE)
+ {
+ err = GetLastError();
+ MsgToEventLog(M_SYSERR, TEXT("Error opening tun device (%s)"), device_path);
+ goto out;
+ }
+
+ /* duplicate tun device handle to openvpn process */
+ if (!DuplicateHandle(GetCurrentProcess(), local_handle, ovpn_proc, remote_handle, 0, FALSE,
+ DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS))
+ {
+ err = GetLastError();
+ MsgToEventLog(M_SYSERR, TEXT("Error duplicating tun device handle"));
+ }
+
+out:
+ free(wguid);
+
+ return err;
+}
+
static VOID
-HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events, undo_lists_t *lists)
+HandleMessage(HANDLE pipe, HANDLE ovpn_proc, DWORD bytes, DWORD count, LPHANDLE events, undo_lists_t *lists)
{
DWORD read;
union {
@@ -1210,6 +1268,7 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events, undo_lists
block_dns_message_t block_dns;
dns_cfg_message_t dns;
enable_dhcp_message_t dhcp;
+ open_tun_device_message_t open_tun;
} msg;
ack_message_t ack = {
.header = {
@@ -1277,6 +1336,24 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events, undo_lists
}
break;
+ case msg_open_tun_device:
+ if (msg.header.size == sizeof(msg.open_tun))
+ {
+ open_tun_device_result_message_t res = {
+ .header = {
+ .type = msg_open_tun_device_result,
+ .size = sizeof(res),
+ .message_id = msg.header.message_id
+ }
+ };
+ /* make sure that string passed from user process is null-terminated */
+ msg.open_tun.device_guid[sizeof(msg.open_tun.device_guid) - 1] = '\0';
+ res.error_number = HandleOpenTunDeviceMessage(&msg.open_tun, ovpn_proc, &res.handle);
+ WritePipeAsync(pipe, &res, sizeof(res), count, events);
+ return;
+ }
+ break;
+
default:
ack.error_number = ERROR_MESSAGE_TYPE;
MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Unknown message type %d"), msg.header.type);
@@ -1611,7 +1688,7 @@ RunOpenvpn(LPVOID p)
break;
}
- HandleMessage(ovpn_pipe, bytes, 1, &exit_event, &undo_lists);
+ HandleMessage(ovpn_pipe, proc_info.hProcess, bytes, 1, &exit_event, &undo_lists);
}
WaitForSingleObject(proc_info.hProcess, IO_TIMEOUT);