From: Lev Stipakov Date: Fri, 24 Jul 2020 10:48:41 +0000 (+0300) Subject: wintun: remove SYSTEM elevation hack X-Git-Tag: v2.5_beta1~35 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6d19775a468f0d35cd1f199b489ee18a74e6c905;p=thirdparty%2Fopenvpn.git wintun: remove SYSTEM elevation hack As discussed a while ago on the mailing list and community meetings, having SYSTEM elevation hack inside openvpn code considered harmful. Since interactive service is the recommended way of using openvpn on Windows, limiting wintun usage to interactive service should not be an issue. Remove elevation hack code and provide an error message telling user to use interactive service or do SYSTEM elevation himself via psexec. Move implementation of register_ring_buffers() to header amd delete ring_buffer.c. Signed-off-by: Lev Stipakov Acked-by: Gert Doering Message-Id: <20200724104841.89-1-lstipakov@gmail.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20567.html Signed-off-by: Gert Doering --- diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index d2338e79b..37b002c61 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -142,6 +142,6 @@ openvpn_LDADD = \ $(OPTIONAL_DL_LIBS) \ $(OPTIONAL_INOTIFY_LIBS) if WIN32 -openvpn_SOURCES += openvpn_win32_resources.rc block_dns.c block_dns.h ring_buffer.c ring_buffer.h +openvpn_SOURCES += openvpn_win32_resources.rc block_dns.c block_dns.h ring_buffer.h openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm -lfwpuclnt -lrpcrt4 -lncrypt -lsetupapi endif diff --git a/src/openvpn/openvpn.vcxproj b/src/openvpn/openvpn.vcxproj index 2da2b8700..5367979de 100644 --- a/src/openvpn/openvpn.vcxproj +++ b/src/openvpn/openvpn.vcxproj @@ -201,7 +201,6 @@ - diff --git a/src/openvpn/openvpn.vcxproj.filters b/src/openvpn/openvpn.vcxproj.filters index af2f0bcf8..cf5748c7d 100644 --- a/src/openvpn/openvpn.vcxproj.filters +++ b/src/openvpn/openvpn.vcxproj.filters @@ -240,9 +240,6 @@ Source Files - - Source Files - Source Files diff --git a/src/openvpn/ring_buffer.c b/src/openvpn/ring_buffer.c deleted file mode 100644 index 8c81dc464..000000000 --- a/src/openvpn/ring_buffer.c +++ /dev/null @@ -1,56 +0,0 @@ -/* - * OpenVPN -- An application to securely tunnel IP networks - * over a single UDP port, with support for SSL/TLS-based - * session authentication and key exchange, - * packet encryption, packet authentication, and - * packet compression. - * - * Copyright (C) 2002-2019 OpenVPN Inc - * 2019 Lev Stipakov - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 - * as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License along - * with this program; if not, write to the Free Software Foundation, Inc., - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. - */ - -#include "ring_buffer.h" - -#ifdef _WIN32 - -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(struct tun_ring); - rr.send.tail_moved = send_tail_moved; - - rr.receive.ring = receive_ring; - rr.receive.ring_size = sizeof(struct tun_ring); - rr.receive.tail_moved = receive_tail_moved; - - res = DeviceIoControl(device, TUN_IOCTL_REGISTER_RINGS, &rr, sizeof(rr), - NULL, 0, &bytes_returned, NULL); - - return res != FALSE; -} - -#endif /* ifdef _WIN32 */ \ No newline at end of file diff --git a/src/openvpn/ring_buffer.h b/src/openvpn/ring_buffer.h index af46f1065..4293f6332 100644 --- a/src/openvpn/ring_buffer.h +++ b/src/openvpn/ring_buffer.h @@ -94,11 +94,32 @@ struct TUN_PACKET * that data has been written to receive ring * @return true if registration is successful, false otherwise - use GetLastError() */ -bool register_ring_buffers(HANDLE device, - struct tun_ring *send_ring, - struct tun_ring *receive_ring, - HANDLE send_tail_moved, - HANDLE receive_tail_moved); +static 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(struct tun_ring); + rr.send.tail_moved = send_tail_moved; + + rr.receive.ring = receive_ring; + rr.receive.ring_size = sizeof(struct tun_ring); + rr.receive.tail_moved = receive_tail_moved; + + res = DeviceIoControl(device, TUN_IOCTL_REGISTER_RINGS, &rr, sizeof(rr), + NULL, 0, &bytes_returned, NULL); + + return res != FALSE; +} #endif /* ifndef OPENVPN_RING_BUFFER_H */ #endif /* ifdef _WIN32 */ diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index e96368ca7..82d96927c 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -6148,35 +6148,10 @@ wintun_register_ring_buffer(struct tuntap *tt, const char *device_guid) } 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)) - { - switch (GetLastError()) - { - case ERROR_ACCESS_DENIED: - msg(M_FATAL, "Access denied registering ring buffers. Is this process run as SYSTEM?"); - break; - - case ERROR_ALREADY_INITIALIZED: - msg(M_NONFATAL, "Adapter %s is already in use", device_guid); - break; - - default: - msg(M_NONFATAL | M_ERRNO, "Failed to register ring buffers"); - } - ret = false; - } - if (!RevertToSelf()) - { - msg(M_FATAL, "ERROR: RevertToSelf error: %lu", GetLastError()); - } + msg(M_FATAL, "ERROR: Wintun requires SYSTEM privileges and therefore " + "should be used with interactive service. If you want to " + "use openvpn from command line, you need to do SYSTEM " + "elevation yourself (for example with psexec)."); } return ret; diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c index 303bd7af1..7e9131657 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -1509,99 +1509,4 @@ send_msg_iservice(HANDLE pipe, const void *data, size_t size, return ret; } -bool -impersonate_as_system() -{ - 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; - } - - 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; -} - #endif /* ifdef _WIN32 */ diff --git a/src/openvpnserv/Makefile.am b/src/openvpnserv/Makefile.am index f8d3319cc..5dc38c9bf 100644 --- a/src/openvpnserv/Makefile.am +++ b/src/openvpnserv/Makefile.am @@ -37,4 +37,4 @@ openvpnserv_SOURCES = \ validate.c validate.h \ $(top_srcdir)/src/openvpn/block_dns.c $(top_srcdir)/src/openvpn/block_dns.h \ openvpnserv_resources.rc \ - $(top_srcdir)/src/openvpn/ring_buffer.c $(top_srcdir)/src/openvpn/ring_buffer.h + $(top_srcdir)/src/openvpn/ring_buffer.h diff --git a/src/openvpnserv/openvpnserv.vcxproj b/src/openvpnserv/openvpnserv.vcxproj index c5a34b87c..5e973df41 100644 --- a/src/openvpnserv/openvpnserv.vcxproj +++ b/src/openvpnserv/openvpnserv.vcxproj @@ -115,7 +115,6 @@ - diff --git a/src/openvpnserv/openvpnserv.vcxproj.filters b/src/openvpnserv/openvpnserv.vcxproj.filters index 3cb14ef62..41ad3e806 100644 --- a/src/openvpnserv/openvpnserv.vcxproj.filters +++ b/src/openvpnserv/openvpnserv.vcxproj.filters @@ -33,9 +33,6 @@ Source Files - - Source Files -