Hi,
On Thu, Jul 18, 2019 at 7:42 AM Lev Stipakov <lstipa...@gmail.com> wrote:
> From: Lev Stipakov <l...@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 <l...@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
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
