Hi, Thanks for looking into this. See my comments below.
TLDR: > (i) stealing SYSTEM access from winlogon.exe is not a good thing to do > This doesn't happen for the majority of use cases - only when iservice is not used. We also elevate only for the single DeviceIOControl call. Below you mentioned psexec as a one of workarounds, but I think those will make user experience worse. Also consider the case when OpenVPN GUI is running as Administrator. (ii) with a small change we can support multiple tunnels and provide better > diagnostics when there are no free wintun adapters -- but this could also > be a follow up patch. > This is already implemented by Simon on top of my patches - see https://github.com/rozmansi/openvpn/commits/wintun > - /* for wintun kernel doesn't send DHCP requests, so use ipapi to set > IP address and netmask */ > + /* for wintun kernel doesn't send DHCP requests, so use netsh to set > IP address and netmask */ > > if (options->wintun) > > { > > - options->tuntap_options.ip_win32_type = IPW32_SET_IPAPI; > > + options->tuntap_options.ip_win32_type = IPW32_SET_NETSH; > > This shouldn't be required. I would prefer to not use netsh > for tasks where API calls are available. We know IPAPI works > as we use it in the service. > I remember having some issues with IPAPI which got resolved when I switched to netsh. However, I did some testing and I cannot reproduce IPAPI specific issues anymore. Too bad I didn't investigate it further back then. The problem I see now can also be reproduced also with netsh: - connect with tap adapter - kill openvpn process - connect with wintun adapter IPAPI / netsh fails to set IP address, since it is already assigned to another adapter. If this is OK, I would prefer to submit a separate patch which switches back to netsh. tun.c code has changed in follow-up patches (mostly here https://patchwork.openvpn.net/patch/918/), those patches are based on an assumption that wintun uses netsh. > > + msg(M_FATAL, "ERROR: Failed to register ring buffers: > %lu", GetLastError()); > > The correct error here is, very likely, the adapter is > already in use. To trigger that, why not do register_ring_buffers > soon after opening the device and on failure move on to the > next device (if any). > > That will also allow running multiple tunnels with no further changes > provided the user manually creates wintum adapters. > As mentioned before, this is already implemented by Simon: https://github.com/rozmansi/openvpn/commit/5bc09580e8771d8422feae4504fe2a74b7412fd1 > > +bool > +impersonate_as_system() > > +{ > > This is implemented by stealing the access token from > winlogon.exe. I don't think such tricks belong to OpenVPN. > It may also trip some anti-virus software. > > That said, probably there are no "legitimate" ways of getting > LOCAL SYSTEM rights on Windows without running a service. > > Why does wintun require SYSTEM for using it? If there is a > good reason for that, we should not let every admin > user bypass it. > I'll defer it to Simon. > > I would suggest to not elevate to system as the code will still > work in two important cases > (i) started by automatic service > (ii) started using interactive service (e.g., through the GUI) > > Those who really need to test OpenVPN with wintun from > command prompt can use diagnostic tools available to get > a cmd prompt as system (e.g., psexec). That also makes > it explicit that SYSTEM privilege is required. > > In the longer run, we could provide a script to launch > openvpn.exe using the interactive service. Modifying the > automatic service to use interactive service for launching > looks easy to do as well. Then, all privileged operations could > be removed from openvpn core. > I think it is good not to break user experience and allow run openvpn as an administrator without iservice using wintun at the expense on elevation to system for single API call. In case we keep this function, please log such > errors. Here and several such cases below. > Sure. -- -Lev
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
