| Message ID | 1561491241-16380-1-git-send-email-lstipakov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] openvpnserv: enable interactive service to open tun | expand |
Hi, On Tue, Jun 25, 2019 at 10:34:01PM +0300, Lev Stipakov wrote: > ack_message_t ack; > struct gc_arena gc = gc_new(); > > - if (!send_msg_iservice(pipe, rt, size, &ack, "ROUTE")) > + if (!send_msg_iservice(pipe, rt, size, &ack, sizeof(ack), "ROUTE")) I do not like this. Please find another way to send the request message "with length" than to add an extra parameter to every single caller of send_msg_iservice(). Possibly introduce a wrapper for the "standard" case which calls a new function send_msg_iservice_ex() that takes a length field for the return data type. And the open wintun / return handle would then use _ex(). gert
Hi On Tue, Jun 25, 2019 at 3:49 PM Gert Doering <gert@greenie.muc.de> wrote: > > Hi, > > On Tue, Jun 25, 2019 at 10:34:01PM +0300, Lev Stipakov wrote: > > ack_message_t ack; > > struct gc_arena gc = gc_new(); > > > > - if (!send_msg_iservice(pipe, rt, size, &ack, "ROUTE")) > > + if (!send_msg_iservice(pipe, rt, size, &ack, sizeof(ack), "ROUTE")) > > I do not like this. Please find another way to send the request message > "with length" than to add an extra parameter to every single caller of > send_msg_iservice(). Gert beat me to it :) Anyway, me too! The way interactive service structures are coded should not require this at all, does it? The size and message type are already in the header, so why do we need to pass it? The result here is a new kind of ack message with a different size and type and that could be checked by accessing the header element. Unless I'm missing something. > > + HANDLE local_handle = CreateFileA(open_tun->device_path, GENERIC_READ | GENERIC_WRITE, 0, 0, > + OPEN_EXISTING, FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED, 0); > + > + if (local_handle == INVALID_HANDLE_VALUE) > + { > + WCHAR *device_path_wchar = NULL; > + int size = sizeof(open_tun->device_path); > + err = GetLastError(); > + > + device_path_wchar = malloc(size * sizeof(WCHAR)); > + if (device_path_wchar) > + { > + MultiByteToWideChar(CP_UTF8, 0, open_tun->device_path, size, device_path_wchar, size); > + device_path_wchar[size - 1] = 0; > + } > + MsgToEventLog(M_SYSERR, TEXT("Error opening tun device (%s)"), device_path_wchar); > + free(device_path_wchar); > + return err; > + } > Also this one -- I think we should just use the wide version of CreateFile -- all strings in OpenVPN.exe are supposed to be in utf8, so convert to widechar and call CreateFileW. Selva
Hi, On Tue, Jun 25, 2019 at 03:57:18PM -0400, Selva Nair wrote: > The way interactive service structures are coded should not require > this at all, does it? The size and message type are already in the > header, so why do we need to pass it? The result here is a new kind of > ack message with a different size and type and that could be checked > by accessing the header element. Unless I'm missing something. You could, indeed, do this inside send_msg_iservice() by looking at "what request did we sent? so what should be coming back?" but I'm not sure I find this safe enough (caller allocates memory, and maybe we shouldn't rely on "it being big enough" unless explicitly told). It would work, yes, but leaves me with a bit uneasy feeling. gert
Hi, > The way interactive service structures are coded should not require > this at all, does it? The size and message type are already in the > header, so why do we need to pass it? But we need to know the response size in send_msg_iservice() since we pass it to ReadFile(). So far we assumed that response is always ask_message_t and we could do sizeof(*ack). With new response type this assumption doesn't hold so as Gert suggested I added another version which accepts arbitrary response type and size. 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) { Will send V2 tomorrow with this and CreateFileW changes.
Hi On Tue, Jun 25, 2019 at 4:34 PM Gert Doering <gert@greenie.muc.de> wrote: > > Hi, > > On Tue, Jun 25, 2019 at 03:57:18PM -0400, Selva Nair wrote: > > The way interactive service structures are coded should not require > > this at all, does it? The size and message type are already in the > > header, so why do we need to pass it? The result here is a new kind of > > ack message with a different size and type and that could be checked > > by accessing the header element. Unless I'm missing something. > > You could, indeed, do this inside send_msg_iservice() by looking > at "what request did we sent? so what should be coming back?" but > I'm not sure I find this safe enough (caller allocates memory, and > maybe we shouldn't rely on "it being big enough" unless explicitly > told). It would work, yes, but leaves me with a bit uneasy feeling. I was thinking of dereferening the response pointer as a union and check the header size which the caller is supposed to have set. So change ack * to void * in send_msg_iservice() as in the patch, and treat it as ack or and extended ack depending on the specified size. Further, when reading from the pipe one should also check the size of data received matches what is expected. It may be also useful to make all reply messages made up of an ack message plus optional additional data -- ie., all start with a header and an error code. Selva > > gert > -- > "If was one thing all people took for granted, was conviction that if you > feed honest figures into a computer, honest figures come out. Never doubted > it myself till I met a computer with a sense of humor." > Robert A. Heinlein, The Moon is a Harsh Mistress > > Gert Doering - Munich, Germany gert@greenie.muc.de
Hi, On Tue, Jun 25, 2019 at 4:38 PM Lev Stipakov <lstipakov@gmail.com> wrote: > > Hi, > >> >> The way interactive service structures are coded should not require >> this at all, does it? The size and message type are already in the >> header, so why do we need to pass it? > > > But we need to know the response size in send_msg_iservice() since > we pass it to ReadFile(). So far we assumed that response is always ask_message_t > and we could do sizeof(*ack). With new response type this assumption doesn't hold so > as Gert suggested I added another version which accepts arbitrary response type and size. My point is that, this is not in the spirit of the rest of iservice code. See HandleMessage in interactive.c where the data is and then interpreted using the header type and size. For what max-size to pass to ReadFile, we know it from the size in the header element of the struct. Selva > > 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) > { > > Will send V2 tomorrow with this and CreateFileW changes. > > > -- > -Lev
Hi, What I have in mind would also require editing all calls to send_msg_iservice() which is essentially what Gert is objecting to. So ignore me -- a separate send_msg_iservice_ex may be the best option. Selva On Tue, Jun 25, 2019 at 5:00 PM Selva Nair <selva.nair@gmail.com> wrote: > > Hi, > > On Tue, Jun 25, 2019 at 4:38 PM Lev Stipakov <lstipakov@gmail.com> wrote: > > > > Hi, > > > >> > >> The way interactive service structures are coded should not require > >> this at all, does it? The size and message type are already in the > >> header, so why do we need to pass it? > > > > > > But we need to know the response size in send_msg_iservice() since > > we pass it to ReadFile(). So far we assumed that response is always ask_message_t > > and we could do sizeof(*ack). With new response type this assumption doesn't hold so > > as Gert suggested I added another version which accepts arbitrary response type and size. > > My point is that, this is not in the spirit of the rest of iservice code. See > HandleMessage in interactive.c where the data is and then interpreted > using the header type and size. > > For what max-size to pass to ReadFile, we know it from the size in the header > element of the struct. > > Selva > > > > > > 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) > > { > > > > Will send V2 tomorrow with this and CreateFileW changes. > > > > > > -- > > -Lev
diff --git a/include/openvpn-msg.h b/include/openvpn-msg.h index 66177a2..273d9a6 100644 --- a/include/openvpn-msg.h +++ b/include/openvpn-msg.h @@ -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_path[512]; +} open_tun_device_message_t; + +typedef struct { + message_header_t header; + HANDLE handle; + int error_number; +} open_tun_device_result_message_t; #endif /* ifndef OPENVPN_MSG_H_ */ diff --git a/src/openvpn/route.c b/src/openvpn/route.c index 4cdc4a9..27fa18c 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -2992,7 +2992,7 @@ do_route_service(const bool add, const route_message_t *rt, const size_t size, H ack_message_t ack; struct gc_arena gc = gc_new(); - if (!send_msg_iservice(pipe, rt, size, &ack, "ROUTE")) + if (!send_msg_iservice(pipe, rt, size, &ack, sizeof(ack), "ROUTE")) { goto out; } diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 8f8f7c6..2d7bd0d 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -115,7 +115,7 @@ do_address_service(const bool add, const short family, const struct tuntap *tt) addr.prefix_len = tt->netbits_ipv6; } - if (!send_msg_iservice(pipe, &addr, sizeof(addr), &ack, "TUN")) + if (!send_msg_iservice(pipe, &addr, sizeof(addr), &ack, sizeof(ack), "TUN")) { goto out; } @@ -181,7 +181,7 @@ do_dns6_service(bool add, const struct tuntap *tt) msg(D_LOW, "%s IPv6 dns servers on '%s' (if_index = %d) using service", (add ? "Setting" : "Deleting"), dns.iface.name, dns.iface.index); - if (!send_msg_iservice(pipe, &dns, sizeof(dns), &ack, "TUN")) + if (!send_msg_iservice(pipe, &dns, sizeof(dns), &ack, sizeof(ack), "TUN")) { goto out; } @@ -5227,7 +5227,7 @@ service_enable_dhcp(const struct tuntap *tt) .iface = { .index = tt->adapter_index, .name = "" } }; - if (!send_msg_iservice(pipe, &dhcp, sizeof(dhcp), &ack, "Enable_dhcp")) + if (!send_msg_iservice(pipe, &dhcp, sizeof(dhcp), &ack, sizeof(ack), "Enable_dhcp")) { goto out; } @@ -5248,6 +5248,43 @@ out: return ret; } +static HANDLE +service_open_tun_device(const HANDLE pipe, const char* device_path) +{ + 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_path, device_path, sizeof(open_tun_device.device_path)); + + if (!send_msg_iservice(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_path); + } + else + { + msg(M_INFO, "Opened tun device %s using service", device_path); + } + +out: + gc_free(&gc); + return result_msg.handle; +} + /* * Return a TAP name for netsh commands. */ @@ -5469,7 +5506,7 @@ register_dns_service(const struct tuntap *tt) message_header_t rdns = { msg_register_dns, sizeof(message_header_t), 0 }; - if (!send_msg_iservice(msg_channel, &rdns, sizeof(rdns), &ack, "Register_dns")) + if (!send_msg_iservice(msg_channel, &rdns, sizeof(rdns), &ack, sizeof(ack), "Register_dns")) { gc_free(&gc); return; @@ -5631,15 +5668,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_path); + } + 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) { @@ -5937,7 +5981,7 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun }; if (send_msg_iservice(tt->options.msg_channel, &msg, sizeof(msg), - &ack, "TUN")) + &ack, sizeof(ack), "TUN")) { status = ack.error_number; } diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c index eb4c030..80d2025 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -1280,7 +1280,7 @@ win_block_dns_service(bool add, int index, const HANDLE pipe) .iface = { .index = index, .name = "" } }; - if (!send_msg_iservice(pipe, &data, sizeof(data), &ack, "Block_DNS")) + if (!send_msg_iservice(pipe, &data, sizeof(data), &ack, sizeof(ack), "Block_DNS")) { goto out; } @@ -1474,14 +1474,14 @@ win32_version_string(struct gc_arena *gc, bool add_name) bool send_msg_iservice(HANDLE pipe, const void *data, size_t size, - ack_message_t *ack, const char *context) + 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", diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h index 4814bbc..6602552 100644 --- a/src/openvpn/win32.h +++ b/src/openvpn/win32.h @@ -311,11 +311,11 @@ 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. + * and read the result in |response| of size |response_size|. Returns false + * on communication error. The string in |context| is used to prefix error messages. */ bool send_msg_iservice(HANDLE pipe, const void *data, size_t size, - ack_message_t *ack, const char *context); + void *response, size_t response_size, const char *context); /* * Attempt to simulate fork/execve on Windows diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 623c3ff..f530c82 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -57,7 +57,7 @@ static HANDLE exit_event = NULL; static settings_t settings; static HANDLE rdns_semaphore = NULL; #define RDNS_TIMEOUT 600 /* seconds to wait for the semaphore */ - +static HANDLE ovpn_process = NULL; /* used by DuplicateHandle() when passing tun device handle to openvpn process */ openvpn_service_t interactive_service = { interactive, @@ -1198,6 +1198,44 @@ HandleEnableDHCPMessage(const enable_dhcp_message_t *dhcp) return err; } +static DWORD +HandleOpenTunDeviceMessage(const open_tun_device_message_t *open_tun, HANDLE *remote_handle) +{ + DWORD err = 0; + + *remote_handle = INVALID_HANDLE_VALUE; + + HANDLE local_handle = CreateFileA(open_tun->device_path, GENERIC_READ | GENERIC_WRITE, 0, 0, + OPEN_EXISTING, FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED, 0); + + if (local_handle == INVALID_HANDLE_VALUE) + { + WCHAR *device_path_wchar = NULL; + int size = sizeof(open_tun->device_path); + err = GetLastError(); + + device_path_wchar = malloc(size * sizeof(WCHAR)); + if (device_path_wchar) + { + MultiByteToWideChar(CP_UTF8, 0, open_tun->device_path, size, device_path_wchar, size); + device_path_wchar[size - 1] = 0; + } + MsgToEventLog(M_SYSERR, TEXT("Error opening tun device (%s)"), device_path_wchar); + free(device_path_wchar); + return err; + } + + if (!DuplicateHandle(GetCurrentProcess(), local_handle, ovpn_process, remote_handle, 0, FALSE, + DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) + { + err = GetLastError(); + MsgToEventLog(M_SYSERR, TEXT("Error duplicating tun device handle")); + return err; + } + + return err; +} + static VOID HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events, undo_lists_t *lists) { @@ -1210,6 +1248,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 +1316,22 @@ 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 + } + }; + res.error_number = HandleOpenTunDeviceMessage(&msg.open_tun, &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); @@ -1603,6 +1658,8 @@ RunOpenvpn(LPVOID p) free(input); } + ovpn_process = proc_info.hProcess; + while (TRUE) { DWORD bytes = PeekNamedPipeAsync(ovpn_pipe, 1, &exit_event);