Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi On Tue, Dec 17, 2019 at 6:09 AM Simon Rozman wrote: > > I have been playing with Lev's patches for the past few days. Tested them, > debugged them, did some fixes. There are things to be desired like > netsh=>ipcfg, remove or #ifdef the SYSTEM token hack... But those are design > choices we should pursue in the future. I believe patches are mature enough > to ack them. They should be merged into master to provide wider testing and > easier development progress. I agree. And, if we wont release official binaries with the system hack, the patch looks good to me too. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi Simon, A quick reply: > > IMO, the right approach on Windows is to run a bare minimal code as a > > service to get SYSTEM rights and the rest with limited privileges. > > Selva, those are two different use-cases. And none is "right" or "wrong". > OpenVPN can or should have both. :) > > 1. I need to run VPN tunnel as a persistent service - something that comes up > with computer (Group Policy Client service waits for about 30 seconds on boot > to get network access to AD server). And stays on all the time - any user > signed in or not. I connect computers with VPN. I too use OpenVPN like this, so I do understand the use case. And, the point was that the exe can be started through interactive service even in this case. That would allow running openvpn.exe at boot from a service with low privileges that delegates all privileged actions to iservice. Years ago when iservice was introduced we did briefly discuss this (with Heiko) but left it as a future enhancement which of course no one had time for. Unless I'm missing some scenario where this wont work. Anyway, this is beyond the scope of the current patch. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
I don't want to give up :) Let's at least wrap that code inside #define ENABLE_SYSTEM_ELEVATION (because we don't have enough defines) to not to complicate development / debugging. ti 17. jouluk. 2019 klo 13.18 Simon Rozman (si...@rozman.si) kirjoitti: > Hi, > > > > Lev, unfortunately, the openvpn.exe binary will still contain this hack's > machine code and might really rise some eyebrows with anti-virus software. > > > > Before the final release the entire SYSTEM token hack should be removed > from the OpenVPN source. > > > > I think it's okay to make a note in README that "--windows-driver wintun" > works with iservice only. (Or, if user is responsible to run openvpn.exe as > SYSTEM user himself somehow.) > > > > Regards, > > Simon > > > > *From:* Lev Stipakov > *Sent:* Tuesday, December 17, 2019 10:35 AM > *To:* Selva Nair > *Cc:* Simon Rozman ; Lev Stipakov ; > openvpn-devel > *Subject:* Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based > I/O > > > > How about compromise - let's add "--enable-system-elevation" windows > specific option. > > > > - When it is set, we print warning and elevate to SYSTEM for the single > DeviceIOControl call > > > > - When it is not set and wintun is used, we run openvpn from command line > via iservice > > > > - If service is missing, we print error "either use iservice or > --enable-system-elevation (experts only)" > > > > The ones who want to run openvpn from command line with wintun still could > do it via psexec and it is > > assumed that they know what they are doing. So why not make it simpler and > add a option > > "yes I know what I am doing and I am willing to take risks"? Also, it is > safer to use system privilege > > just for single call rather than run whole process with it. > > > > This also will make debugging / development easier. > -- -Lev ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi, Lev, unfortunately, the openvpn.exe binary will still contain this hack's machine code and might really rise some eyebrows with anti-virus software. Before the final release the entire SYSTEM token hack should be removed from the OpenVPN source. I think it's okay to make a note in README that "--windows-driver wintun" works with iservice only. (Or, if user is responsible to run openvpn.exe as SYSTEM user himself somehow.) Regards, Simon From: Lev Stipakov Sent: Tuesday, December 17, 2019 10:35 AM To: Selva Nair Cc: Simon Rozman ; Lev Stipakov ; openvpn-devel Subject: Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O How about compromise - let's add "--enable-system-elevation" windows specific option. - When it is set, we print warning and elevate to SYSTEM for the single DeviceIOControl call - When it is not set and wintun is used, we run openvpn from command line via iservice - If service is missing, we print error "either use iservice or --enable-system-elevation (experts only)" The ones who want to run openvpn from command line with wintun still could do it via psexec and it is assumed that they know what they are doing. So why not make it simpler and add a option "yes I know what I am doing and I am willing to take risks"? Also, it is safer to use system privilege just for single call rather than run whole process with it. This also will make debugging / development easier. smime.p7s Description: S/MIME cryptographic signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi, > > I am running OpenVPN on Windows using NSSM wrapper for years. I had a > brief discussion on the Hackathon with Samuli about integrating SCM > support directly into openvpn.exe (imagine --daemon for Windows): > > > > sc create OpenVPN$MyTunnel binpath= "C:\Program > > Files\OpenVPN\bin\openvpn" --daemon --config "C:\Program > > Files\OpenVPN\config\MyTunnel.ovpn" --log "C:\Program > > Files\OpenVPN\log\MyTunnel.log" start= auto depend= dhcp sc start > > OpenVPN$MyTunnel > > > > This would install openvpn.exe as a Windows service and run it as the > SYSTEM user — no need for iservice, no need for SYSTEM token hack. Other > than me, perhaps it could cover at least some of the users now running > openvpn.exe directly. > > This is not the direction I want to see us moving towards. Instead I > want to see us daemonizing OpenVPN.exe as LOCAL SERVICE or a custom > service user and delegate privileged operations to a separate service > running as SYSTEM. And we already have the latter: interactive service. > So, not even admin rights is needed in openvpn.exe, let alone SYSTEM. > > IMO, the right approach on Windows is to run a bare minimal code as a > service to get SYSTEM rights and the rest with limited privileges. Selva, those are two different use-cases. And none is "right" or "wrong". OpenVPN can or should have both. :) 1. I need to run VPN tunnel as a persistent service - something that comes up with computer (Group Policy Client service waits for about 30 seconds on boot to get network access to AD server). And stays on all the time - any user signed in or not. I connect computers with VPN. 2. You need an openvpn.exe (or introduce some openvpn-tui.exe) to be a command line version of openvpn-gui.exe. Something to be run by a regular user or a batch script in an unprivileged context. You connect users with VPN. If we implement both, we can obsolete SYSTEM token hack completely. I have been playing with Lev's patches for the past few days. Tested them, debugged them, did some fixes. There are things to be desired like netsh=>ipcfg, remove or #ifdef the SYSTEM token hack... But those are design choices we should pursue in the future. I believe patches are mature enough to ack them. They should be merged into master to provide wider testing and easier development progress. Regards, Simon smime.p7s Description: S/MIME cryptographic signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
How about compromise - let's add "--enable-system-elevation" windows
specific option.
- When it is set, we print warning and elevate to SYSTEM for the single
DeviceIOControl call
- When it is not set and wintun is used, we run openvpn from command line
via iservice
- If service is missing, we print error "either use iservice or
--enable-system-elevation (experts only)"
The ones who want to run openvpn from command line with wintun still could
do it via psexec and it is
assumed that they know what they are doing. So why not make it simpler and
add a option
"yes I know what I am doing and I am willing to take risks"? Also, it is
safer to use system privilege
just for single call rather than run whole process with it.
This also will make debugging / development easier.
ti 17. jouluk. 2019 klo 0.38 Selva Nair (selva.n...@gmail.com) kirjoitti:
> Hi,
>
> On Mon, Dec 16, 2019 at 5:18 PM Simon Rozman wrote:
> >
> > Hi,
> >
> > 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.
> >
> >
> > I understand. But stealing access token from winlogon.exe is a hack and
> not
> > something I would expect to see in a trustworthy executable. Diagnostic
> > and forensic tools may be justified in doing such things.
> >
> >
> > Wintun has a hardcoded check to allow ring registration from the SYSTEM
> user only.
> >
> > To be honest: I am using a Windows 10 VPC in test mode with a modified
> Wintun driver installed that also allows ring registration for the members
> of Administrators group. This allows me to quickly test ideas while
> reviewing/upgrading Lev's work. I can run Visual Studio to compile
> openvpn.exe on my computer as an unprivileged user. Then have an elevated
> Remote Debugger running on the VPC and Visual Studio to remotely run
> openvpn.exe with debugger attached with a single F5 click.
> >
> > Having to use OpenVPN GUI and iservice, or running openvpn.exe as
> service would require a lot more clicks to replace the binary, attach
> debugger etc.
> >
> > As far as I am concerned, this elevation hack may be removed from the
> OpenVPN source code in the final release. However, mind that this would
> prevent people from running openvpn.exe+Wintun from the command line.
> >
> > +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.
> >
> >
> > Unfortunately, I don't do security decisions in Wintun.
> >
> > Wintun was originally designed for WireGuard. WireGuard is architectured
> to run all its tunnels as Windows services running as the SYSTEM user.
> Wintun's security is as tight as possible so the WireGuard can barely use
> it. I know a guy who is tempted to introduce a userspace binary code
> signature check to the Wintun. :)
> >
> > Given the relative ease to get SYSTEM token just by being an elevated
> process — mind there's also a hack to get from non-elevated to elevated
> completely bypassing the UAC prompt as long as you are a member of
> Administrators — this SYSTEM restriction really doesn't provide
> considerable additional security compared to being a member of
> Administrators group.
>
> That's the point. It probably provides no extra security because we
> are forced to take the blame for using hacks to cross security
> boundaries. And, bypassing UAC is quite different from acquiring
> rights you were not supposed to have. Does openvpn wants to be doing
> that?
>
> >
> > 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.
> >
> >
> > I have already said what I think of it. As an admin I wouldn't like to
> see
> > users running processes that elevate to SYSTEM like this.
> >
> >
> > Selva, Windows is full of such hacks internally. :( This is no excuse
> for us doing the sa
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi,
On Mon, Dec 16, 2019 at 5:18 PM Simon Rozman wrote:
>
> Hi,
>
> 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.
>
>
> I understand. But stealing access token from winlogon.exe is a hack and not
> something I would expect to see in a trustworthy executable. Diagnostic
> and forensic tools may be justified in doing such things.
>
>
> Wintun has a hardcoded check to allow ring registration from the SYSTEM user
> only.
>
> To be honest: I am using a Windows 10 VPC in test mode with a modified Wintun
> driver installed that also allows ring registration for the members of
> Administrators group. This allows me to quickly test ideas while
> reviewing/upgrading Lev's work. I can run Visual Studio to compile
> openvpn.exe on my computer as an unprivileged user. Then have an elevated
> Remote Debugger running on the VPC and Visual Studio to remotely run
> openvpn.exe with debugger attached with a single F5 click.
>
> Having to use OpenVPN GUI and iservice, or running openvpn.exe as service
> would require a lot more clicks to replace the binary, attach debugger etc.
>
> As far as I am concerned, this elevation hack may be removed from the OpenVPN
> source code in the final release. However, mind that this would prevent
> people from running openvpn.exe+Wintun from the command line.
>
> +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.
>
>
> Unfortunately, I don't do security decisions in Wintun.
>
> Wintun was originally designed for WireGuard. WireGuard is architectured to
> run all its tunnels as Windows services running as the SYSTEM user. Wintun's
> security is as tight as possible so the WireGuard can barely use it. I know a
> guy who is tempted to introduce a userspace binary code signature check to
> the Wintun. :)
>
> Given the relative ease to get SYSTEM token just by being an elevated process
> — mind there's also a hack to get from non-elevated to elevated completely
> bypassing the UAC prompt as long as you are a member of Administrators — this
> SYSTEM restriction really doesn't provide considerable additional security
> compared to being a member of Administrators group.
That's the point. It probably provides no extra security because we
are forced to take the blame for using hacks to cross security
boundaries. And, bypassing UAC is quite different from acquiring
rights you were not supposed to have. Does openvpn wants to be doing
that?
>
> 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.
>
>
> I have already said what I think of it. As an admin I wouldn't like to see
> users running processes that elevate to SYSTEM like this.
>
>
> Selva, Windows is full of such hacks internally. :( This is no excuse for us
> doing the same of course. Just saying Windows is far from ideal world.
In this particular case this has nothing to do with Windows -- its
just a stupid decision by wintun that requires the use of undocumented
hacks to elevate to SYSTEM.
>
> I am running OpenVPN on Windows using NSSM wrapper for years. I had a brief
> discussion on the Hackathon with Samuli about integrating SCM support
> directly into openvpn.exe (imagine --daemon for Windows):
>
> sc create OpenVPN$MyTunnel binpath= "C:\Program Files\OpenVPN\bin\openvpn"
> --daemon --config "C:\Program Files\OpenVPN\config\MyTunnel.ovpn" --log
> "C:\Program Files\OpenVPN\log\MyTunnel.log" start= auto depend= dhcp
> sc start OpenVPN$MyTunnel
>
> This would install openvpn.exe as a Windows service and run it as the SYSTEM
> user — no need for iservice, no need for SYSTEM token hack. Other than me,
> perhaps it could cover at least some of the users now running openvpn.exe
> directly.
This is not the direction I want to see us moving towards. Instead I
want to
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi On Mon, Dec 16, 2019 at 4:31 PM Lev Stipakov wrote: >> >> I have already said what I think of it. As an admin I wouldn't like to see >> users running processes that elevate to SYSTEM like this. > > > Would it be too much if > > - openvpn.exe process detects that it is not started by interactive service > - interactive service process is running > - wintun is used as --windows-driver > I think this is possible and in fact I had something like this in mind as well I wrote we could provide an openvpn launcher script. And I would like to do this even if wintun is in use as ability to run openvpn.exe as a regular would be a plus for everyone. > then openvpn behaves as a command-line version of openvpn-gui - connects to > service, which > runs openvpn.exe, and then prints output, received via management interface, > to console? It may be better to redirect the output to the stdout or file as usual so that no user visible changes and easier to implement too. Just keep the process id returned from the service and when the user kills the front-end, terminate the background process started by the service too. This should also allow to run the automatic service as LocalService or a special service user as many services do. Selva > > -- > -Lev ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi,
>>> 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.
>
> I understand. But stealing access token from winlogon.exe is a hack and not
> something I would expect to see in a trustworthy executable. Diagnostic
> and forensic tools may be justified in doing such things.
Wintun has a hardcoded check to allow ring registration from the SYSTEM user
only.
To be honest: I am using a Windows 10 VPC in test mode with a modified Wintun
driver installed that also allows ring registration for the members of
Administrators group. This allows me to quickly test ideas while
reviewing/upgrading Lev's work. I can run Visual Studio to compile openvpn.exe
on my computer as an unprivileged user. Then have an elevated Remote Debugger
running on the VPC and Visual Studio to remotely run openvpn.exe with debugger
attached with a single F5 click.
Having to use OpenVPN GUI and iservice, or running openvpn.exe as service would
require a lot more clicks to replace the binary, attach debugger etc.
As far as I am concerned, this elevation hack may be removed from the OpenVPN
source code in the final release. However, mind that this would prevent people
from running openvpn.exe+Wintun from the command line.
>>> +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.
Unfortunately, I don't do security decisions in Wintun.
Wintun was originally designed for WireGuard. WireGuard is architectured to run
all its tunnels as Windows services running as the SYSTEM user. Wintun's
security is as tight as possible so the WireGuard can barely use it. I know a
guy who is tempted to introduce a userspace binary code signature check to the
Wintun. :)
Given the relative ease to get SYSTEM token just by being an elevated process —
mind there's also a hack to get from non-elevated to elevated completely
bypassing the UAC prompt as long as you are a member of Administrators — this
SYSTEM restriction really doesn't provide considerable additional security
compared to being a member of Administrators group.
>>> 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.
>
> I have already said what I think of it. As an admin I wouldn't like to see
> users running processes that elevate to SYSTEM like this.
Selva, Windows is full of such hacks internally. :( This is no excuse for us
doing the same of course. Just saying Windows is far from ideal world.
I am running OpenVPN on Windows using NSSM wrapper for years. I had a brief
discussion on the Hackathon with Samuli about integrating SCM support directly
into openvpn.exe (imagine --daemon for Windows):
sc create OpenVPN$MyTunnel binpath= "C:\Program Files\OpenVPN\bin\openvpn"
--daemon --config "C:\Program Files\OpenVPN\config\MyTunnel.ovpn" --log
"C:\Program Files\OpenVPN\log\MyTunnel.log" start= auto depend= dhcp
sc start OpenVPN$MyTunnel
This would install openvpn.exe as a Windows service and run it as the SYSTEM
user — no need for iservice, no need for SYSTEM token hack. Other than me,
perhaps it could cover at least some of the users now running openvpn.exe
directly.
Regards,
Simon
smime.p7s
Description: S/MIME cryptographic signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
> > I have already said what I think of it. As an admin I wouldn't like to see > users running processes that elevate to SYSTEM like this. > Would it be too much if - openvpn.exe process detects that it is not started by interactive service - interactive service process is running - wintun is used as --windows-driver then openvpn behaves as a command-line version of openvpn-gui - connects to service, which runs openvpn.exe, and then prints output, received via management interface, to console? -- -Lev ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi
On Mon, Dec 16, 2019 at 3:01 PM Lev Stipakov wrote:
>
> 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.
I understand. But stealing access token from winlogon.exe is a hack and not
something I would expect to see in a trustworthy executable. Diagnostic
and forensic tools may be justified in doing such things.
>
> Below you mentioned psexec as a one of workarounds, but I think
> those will make user experience worse.
I was not suggesting it as an approach for "users" -- only for testing
or for those
who insist running OpenVPN from command line.
> Also consider the case when OpenVPN GUI is running as Administrator.
That would be a wrong way of using the GUI in 2.4 + releases. We need not and
should not (IMO) support the GUI running as admin. That said, now that vista is
no longer supported we can enable the use of iservice with GUI running as admin.
>
>> (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.
I have already said what I think of it. As an admin I wouldn't like to see
users running processes that elevate to SYSTEM like this.
Selva
___
Openv
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
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
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi,
I was reluctant to review this as I do not understand
the event processing in OpenVPN well enough. Now that Stefann
has reviewed those bits and given an Ack, here are some
comments on the rest of the code.
TLDR:
(i) stealing SYSTEM access from winlogon.exe is not a good thing to do
(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.
On Fri, Dec 13, 2019 at 2:20 PM Lev Stipakov wrote:
>
> From: Lev Stipakov
>
> Implemented according to Wintun documentation
> and reference client code.
>
> Wintun uses ring buffers to communicate between
> kernel driver and user process. Client allocates
> send and receive ring buffers, creates events
> and passes it to kernel driver under LocalSystem
> privileges.
>
> When data is available for read, wintun modifies
> "tail" pointer of send ring and signals via event.
> User process reads data from "head" to "tail" and
> updates "head" pointer.
>
> When user process is ready to write, it writes
> to receive ring, updates "tail" pointer and signals
> to kernel via event.
>
> In openvpn code we add send ring's event to event loop.
> Before performing io wait, we compare "head" and "tail"
> pointers of send ring and if they're different, we skip
> io wait and perform read.
>
> This also adds ring buffers support to tcp and udp
> server code.
>
> Signed-off-by: Lev Stipakov
> ---
> v6:
> - added a sanity check to write_wintun() to avoid
> writing malformed IPv4/6 packet, which causes
> "ring buffer is out of capacity" error.
>
> v5:
> - fix crash at ring buffer registration on Win7
> (passing NULL to DeviceIOControl, reported by kitsune1)
>
> v4:
> - added helper function tuntap_ring_empty()
> - refactored event handling, got rid of separate
> event_ctl() call for wintun and send/receive_tail_moved
> members
> - added wintun_ prefix for ring buffer variables
> - added a comment explaining the size of wintun-specific buffers
>
> v3:
> - simplified convoluted #ifdefs
> - replaced "greater than" with "greater or equal than"
>
> v2;
> - rebased on top of master
>
> src/openvpn/forward.c | 25 ++-
> src/openvpn/forward.h | 38 +-
> src/openvpn/mtcp.c| 19 -
> src/openvpn/mudp.c| 7 +-
> src/openvpn/options.c | 4 +-
> src/openvpn/syshead.h | 1 +
> src/openvpn/tun.c | 62 +++-
> src/openvpn/tun.h | 169 +-
> src/openvpn/win32.c | 122 ++
> src/openvpn/win32.h | 51 +
> 10 files changed, 487 insertions(+), 11 deletions(-)
>
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 8451706b..3dc04a40 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1256,8 +1256,24 @@ read_incoming_tun(struct context *c)
> perf_push(PERF_READ_IN_TUN);
>
> c->c2.buf = c->c2.buffers->read_tun_buf;
> +
> #ifdef _WIN32
> -read_tun_buffered(c->c1.tuntap, &c->c2.buf);
> +if (c->c1.tuntap->wintun)
> +{
> +read_wintun(c->c1.tuntap, &c->c2.buf);
> +if (c->c2.buf.len == -1)
> +{
> +register_signal(c, SIGHUP, "tun-abort");
> +c->persist.restart_sleep_seconds = 1;
> +msg(M_INFO, "Wintun read error, restarting");
> +perf_pop();
> +return;
> +}
> +}
> +else
> +{
> +read_tun_buffered(c->c1.tuntap, &c->c2.buf);
> +}
> #else
> ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame)));
> ASSERT(buf_safe(&c->c2.buf, MAX_RW_SIZE_TUN(&c->c2.frame)));
> @@ -2099,6 +2115,13 @@ io_wait_dowork(struct context *c, const unsigned int
> flags)
> tuntap |= EVENT_READ;
> }
>
> +#ifdef _WIN32
> +if (tuntap_is_wintun(c->c1.tuntap))
> +{
> +tuntap = EVENT_READ;
> +}
> +#endif
> +
> /*
> * Configure event wait based on socket, tuntap flags.
> */
> diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
> index 48202c07..b711ff00 100644
> --- a/src/openvpn/forward.h
> +++ b/src/openvpn/forward.h
> @@ -375,6 +375,12 @@ p2p_iow_flags(const struct context *c)
> {
> flags |= IOW_TO_TUN;
> }
> +#ifdef _WIN32
> +if (tuntap_ring_empty(c->c1.tuntap))
> +{
> +flags &= ~IOW_READ_TUN;
> +}
> +#endif
> return flags;
> }
>
> @@ -403,8 +409,36 @@ io_wait(struct context *c, const unsigned int flags)
> }
> else
> {
> -/* slow path */
> -io_wait_dowork(c, flags);
> +#ifdef _WIN32
> +bool skip_iowait = flags & IOW_TO_TUN;
> +if (flags & IOW_READ_TUN)
> +{
> +/*
> + * don't read from tun if we have pending write to link,
> + * since every tun read overwrites to_link buffer filled
> + * by previous tun read
> + */
> +skip_iowait = !(flags & IOW_
Re: [Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
Hi Lev,
Thanks for the update. Even with the latest added fixes and sanity
checks, the resulting code is now 30 lines shorter *and* more readable.
Some last questions / comments inline:
On 13-12-2019 20:19, Lev Stipakov wrote:
> @@ -2099,6 +2115,13 @@ io_wait_dowork(struct context *c, const unsigned int
> flags)
> tuntap |= EVENT_READ;
> }
>
> +#ifdef _WIN32
> +if (tuntap_is_wintun(c->c1.tuntap))
> +{
> +tuntap = EVENT_READ;
> +}
> +#endif
Now that the code is refactored, it is clear that for the tuntap case
the code will ignore any of the earlier set flags. Can you maybe add a
comment explaining why the other flags are being ignored?
> +/* there is a data in wintun ring buffer, read it immediately */
Grammar nit: "there is data" (remove 'a').
Otherwise, as far as stare-at-code goes, I'm fine with this patch now. I
haven't tested it, nor have I reviewed Windows-specific interaction. So
this needs at least one other review from someone able to review those
parts.
Regarding the integration into OpenVPN:
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v6 4/7] wintun: ring buffers based I/O
From: Lev Stipakov
Implemented according to Wintun documentation
and reference client code.
Wintun uses ring buffers to communicate between
kernel driver and user process. Client allocates
send and receive ring buffers, creates events
and passes it to kernel driver under LocalSystem
privileges.
When data is available for read, wintun modifies
"tail" pointer of send ring and signals via event.
User process reads data from "head" to "tail" and
updates "head" pointer.
When user process is ready to write, it writes
to receive ring, updates "tail" pointer and signals
to kernel via event.
In openvpn code we add send ring's event to event loop.
Before performing io wait, we compare "head" and "tail"
pointers of send ring and if they're different, we skip
io wait and perform read.
This also adds ring buffers support to tcp and udp
server code.
Signed-off-by: Lev Stipakov
---
v6:
- added a sanity check to write_wintun() to avoid
writing malformed IPv4/6 packet, which causes
"ring buffer is out of capacity" error.
v5:
- fix crash at ring buffer registration on Win7
(passing NULL to DeviceIOControl, reported by kitsune1)
v4:
- added helper function tuntap_ring_empty()
- refactored event handling, got rid of separate
event_ctl() call for wintun and send/receive_tail_moved
members
- added wintun_ prefix for ring buffer variables
- added a comment explaining the size of wintun-specific buffers
v3:
- simplified convoluted #ifdefs
- replaced "greater than" with "greater or equal than"
v2;
- rebased on top of master
src/openvpn/forward.c | 25 ++-
src/openvpn/forward.h | 38 +-
src/openvpn/mtcp.c| 19 -
src/openvpn/mudp.c| 7 +-
src/openvpn/options.c | 4 +-
src/openvpn/syshead.h | 1 +
src/openvpn/tun.c | 62 +++-
src/openvpn/tun.h | 169 +-
src/openvpn/win32.c | 122 ++
src/openvpn/win32.h | 51 +
10 files changed, 487 insertions(+), 11 deletions(-)
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 8451706b..3dc04a40 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1256,8 +1256,24 @@ read_incoming_tun(struct context *c)
perf_push(PERF_READ_IN_TUN);
c->c2.buf = c->c2.buffers->read_tun_buf;
+
#ifdef _WIN32
-read_tun_buffered(c->c1.tuntap, &c->c2.buf);
+if (c->c1.tuntap->wintun)
+{
+read_wintun(c->c1.tuntap, &c->c2.buf);
+if (c->c2.buf.len == -1)
+{
+register_signal(c, SIGHUP, "tun-abort");
+c->persist.restart_sleep_seconds = 1;
+msg(M_INFO, "Wintun read error, restarting");
+perf_pop();
+return;
+}
+}
+else
+{
+read_tun_buffered(c->c1.tuntap, &c->c2.buf);
+}
#else
ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame)));
ASSERT(buf_safe(&c->c2.buf, MAX_RW_SIZE_TUN(&c->c2.frame)));
@@ -2099,6 +2115,13 @@ io_wait_dowork(struct context *c, const unsigned int
flags)
tuntap |= EVENT_READ;
}
+#ifdef _WIN32
+if (tuntap_is_wintun(c->c1.tuntap))
+{
+tuntap = EVENT_READ;
+}
+#endif
+
/*
* Configure event wait based on socket, tuntap flags.
*/
diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
index 48202c07..b711ff00 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -375,6 +375,12 @@ p2p_iow_flags(const struct context *c)
{
flags |= IOW_TO_TUN;
}
+#ifdef _WIN32
+if (tuntap_ring_empty(c->c1.tuntap))
+{
+flags &= ~IOW_READ_TUN;
+}
+#endif
return flags;
}
@@ -403,8 +409,36 @@ io_wait(struct context *c, const unsigned int flags)
}
else
{
-/* slow path */
-io_wait_dowork(c, flags);
+#ifdef _WIN32
+bool skip_iowait = flags & IOW_TO_TUN;
+if (flags & IOW_READ_TUN)
+{
+/*
+ * don't read from tun if we have pending write to link,
+ * since every tun read overwrites to_link buffer filled
+ * by previous tun read
+ */
+skip_iowait = !(flags & IOW_TO_LINK);
+}
+if (tuntap_is_wintun(c->c1.tuntap) && skip_iowait)
+{
+unsigned int ret = 0;
+if (flags & IOW_TO_TUN)
+{
+ret |= TUN_WRITE;
+}
+if (flags & IOW_READ_TUN)
+{
+ret |= TUN_READ;
+}
+c->c2.event_set_status = ret;
+}
+else
+#endif
+{
+/* slow path */
+io_wait_dowork(c, flags);
+}
}
}
diff --git a/src/openvpn/mtcp.c b/src/openvpn/mtcp.c
index abe20593..30a13f73 100644
--- a/src/openvpn/mtcp.c
+++ b/src/openvpn/mtcp.c
@@ -269,8 +269,25 @@ multi_tcp_wait(const struct context *c,
struct multi_tcp *mtcp)
{
int status;
+unsigned int *p
