| Message ID | 1561637226-18464-1-git-send-email-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v3] openvpnserv: enable interactive service to open tun | expand |
Hi, On Thu, Jun 27, 2019 at 8:08 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> > --- > 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 This works but there are two general concerns: (i) The new message is named message_open_tun, but it allows opening any file using the service. This is not secure. We need to restrict it to open tun/tap device nodes only. (ii) Should we allow all users to open tap6 adapters irrespective of any other access restrictions that may be present? I'm conflicted about this as, on closer look, access control in tap-windows6 appears broken. > @@ -117,4 +119,14 @@ typedef struct { > interface_t iface; > } enable_dhcp_message_t; > +typedef struct { > + message_header_t header; > + char device_path[256]; > +} open_tun_device_message_t; > + > +typedef struct { > + message_header_t header; > + HANDLE handle; > + int error_number; > +} open_tun_device_result_message_t; Defining this struct with error_number followed by handle would be better (makes its head match in memory with ack_message_t). That makes it possible to read a normal ack into it and resolve the error number. Can happen if openvpn.exe is upgraded but service stays at an old version -- such a service will respond with ack and error_number=ERROR_MESSAGE_TYPE. Selva
Hi, Sorry for delay - I was on vacation. (i) The new message is named message_open_tun, but it allows opening > any file using the service. This is not secure. I am thinking of possible vector of attack here. In our case it is service which launches openvpn process using path set in registry, opens pipe and passes pipe handle to launched process. To make service run malicious process one needs either to replace executable or modify registry. For both actions you need to be administrator, assuming default security policy. Alternatively, can random process hijack the pipe handle from another process? > We need to restrict it to open tun/tap device nodes only. > Without adding too much code, I can think of: - check that path starts with \\\\.\\Global\\ to make sure we open device, not file and - check that path starts with \\\\.\\Global\\WINTUN or ends with .tap Is this good enough or do you have better ideas? (ii) Should we allow all users to open tap6 adapters irrespective of > any other access restrictions that may be present? I'm conflicted > about this as, on closer look, access control in tap-windows6 appears > broken. > I'll pass on this one. Maybe "Strengthening access control for tap-windows6" could be another patch. > Defining this struct with error_number followed by handle would be > better (makes its head match in memory with ack_message_t). That makes > it possible to read a normal ack into it and resolve the error number. > Agreed, will do.
Hi On Wed, Jul 17, 2019 at 8:20 AM Lev Stipakov <lstipakov@gmail.com> wrote: > Hi, > > Sorry for delay - I was on vacation. > > (i) The new message is named message_open_tun, but it allows opening >> any file using the service. This is not secure. > > > I am thinking of possible vector of attack here. > > In our case it is service which launches openvpn process using > path set in registry, opens pipe and passes pipe handle to launched > process. > > To make service run malicious process one needs either to replace > executable or > modify registry. For both actions you need to be administrator, assuming > default security policy. > > Alternatively, can random process hijack the pipe handle from another > process? > Because the pipe handle is targeted for OpenVPN process id and not inheritable that may not be possible. But an attack is very simple: Just write a plugin and use the service to open files from it. Passing the pipe handle to the plugin is trivial. Anyone in OpenVPNAdministrators group can then use a custom config file and get access any file on the system. Membership in that group is not supposed to provide any privileges to limited users beyond setting up routes or manipulating some interface parameters. > >> We need to restrict it to open tun/tap device nodes only. >> > > Without adding too much code, I can think of: > > - check that path starts with \\\\.\\Global\\ to make sure we open > device, not file > > and > > - check that path starts with \\\\.\\Global\\WINTUN or ends with .tap > > Is this good enough or do you have better ideas? > That'll probably work with some extra sanity checks on the file name. Ideally we should just pass the dev-node (empty if unspecified) and type of device (TAP6 or WINTUN), but that will require a lot of duplication of code in the service, as you noted. One option is to pass the device guid in case of tap or the index in case of wintun and construct the path in the service. That requires very little extra code. Otherwise a thorough sanitization of the path is required as there could be obscure ways of breaking out using "..\" or otherwise, though I'm not sure. Things like \\.\C:\..\D:\ works on Windows so I won't take any chances. Selva PS. Just noticed you've already posted a v4 -- I haven't looked at it yet. <div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 17, 2019 at 8:20 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"><div dir="ltr"><div dir="ltr">Hi,<div><br></div><div>Sorry for delay - I was on vacation.</div><div><br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> (i) The new message is named message_open_tun, but it allows opening<br> any file using the service. This is not secure. </blockquote><div><br></div><div>I am thinking of possible vector of attack here.</div><div><br></div><div>In our case it is service which launches openvpn process using</div><div>path set in registry, opens pipe and passes pipe handle to launched process.</div><div><br></div><div>To make service run malicious process one needs either to replace executable or</div><div>modify registry. For both actions you need to be administrator, assuming default security policy.</div><div><br></div><div>Alternatively, can random process hijack the pipe handle from another process?</div></div></div></blockquote><div><br></div><div>Because the pipe handle is targeted for OpenVPN process id and not inheritable that may not be possible. But an attack is very simple: <br></div><div><br></div><div>Just write a plugin and use the service to open files from it. Passing the pipe handle to the plugin is trivial. Anyone in OpenVPNAdministrators group can then use a custom config file and get access any file on the system. Membership in that group is not supposed to provide any privileges to limited users beyond setting up routes or manipulating some interface parameters.<br></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"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">We need to restrict it to open tun/tap device nodes only.<br></blockquote><div><br></div><div>Without adding too much code, I can think of:</div><div><br></div><div> - check that path starts with \\\\.\\Global\\ to make sure we open device, not file</div><div><br></div><div>and</div><div> </div><div> - check that path starts with \\\\.\\Global\\WINTUN or ends with .tap</div><div><br></div><div>Is this good enough or do you have better ideas?</div></div></div></blockquote><div><br></div><div>That'll probably work with some extra sanity checks on the file name. Ideally we should just pass the dev-node (empty if unspecified) and type of device (TAP6 or WINTUN), but that will require a lot of duplication of code in the service, as you noted.<br></div><div><br></div><div>One option is to pass the device guid in case of tap or the index in case of wintun and construct the path in the service. That requires very little extra code. Otherwise a thorough sanitization of the path is required as there could be obscure ways of breaking out using "..\" or otherwise, though I'm not sure. Things like \\.\C:\..\D:\ works on Windows so I won't take any chances.<br></div><div> <br></div><div>Selva</div><div><br></div><div>PS. Just noticed you've already posted a v4 -- I haven't looked at it yet.<br></div></div></div>
Hi, > That'll probably work with some extra sanity checks on the file name. > Ideally we should just pass the dev-node (empty if unspecified) and type of > device (TAP6 or WINTUN), but that will require a lot of duplication of > code in the service, as you noted. > > One option is to pass the device guid in case of tap or the index in case > of wintun and construct the path in the service. That requires very little > extra code. Otherwise a thorough sanitization of the path is required as > there could be obscure ways of breaking out using "..\" or otherwise, > though I'm not sure. Things like \\.\C:\..\D:\ works on Windows so I won't > take any chances. > You are right, just tested and one can escape global like this: \\.\Global\..\C:\lol.tap I'll do as you've proposed - pass a string which is either guid or number, a boolean flag (wintun/tap6) and add some validation. > PS. Just noticed you've already posted a v4 -- I haven't looked at it yet. > v5 is coming!
diff --git a/include/openvpn-msg.h b/include/openvpn-msg.h index 66177a2..53d59a7 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[256]; +} 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/tun.c b/src/openvpn/tun.c index 8f8f7c6..8c7e705 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -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_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_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. */ @@ -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_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) { @@ -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_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) { diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c index eb4c030..039c1a4 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -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", 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 + * @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 */ diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 623c3ff..1729a08 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,48 @@ 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 device_path; + + *remote_handle = INVALID_HANDLE_VALUE; + + device_path = utf8to16(open_tun->device_path); + if (!device_path) + { + err = ERROR_OUTOFMEMORY; + goto out; + } + + /* 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(device_path); + + 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 +1249,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 +1317,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_path[sizeof(msg.open_tun.device_path) - 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 +1669,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);