| Message ID | 1573148729-27339-4-git-send-email-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Wintun support | expand |
Hi, > -----Original Message----- > From: Lev Stipakov [mailto:lstipakov@gmail.com] > Sent: Thursday, November 7, 2019 6:45 PM > To: openvpn-devel@lists.sourceforge.net > Cc: Lev Stipakov <lev@openvpn.net> > Subject: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun > device > > +const static GUID GUID_DEVCLASS_NET = { 0x4d36e972L, 0xe325, 0x11ce, { > +0xbf, 0xc1, 0x08, 0x00, 0x2b, 0xe1, 0x03, 0x18 } }; const static GUID > +GUID_DEVINTERFACE_NET = { 0xcac88484, 0x7 515, 0x4c03, { 0x82, 0xe6, > +0x71, 0xa8, 0x7a, 0xba, 0xc3, 0x61 } }; > + GUID_DEVCLASS_NET is declared in devguid.h, GUID_DEVINTERFACE_NET in ndisguid.h... No need to redefine them. However, while one could include those SDK files, one needs to add the appropriate .lib files too. It's not worth complicating for just a couple of GUIDs that will never ever change. So, ACK. The rest LGTM. Acked-by: Simon Rozman <simon@rozman.si> Best regards, Simon
Your patch has been applied to the master branch.
Since this introduces a new library requirement, I gave it a test run
on my Ubuntu 16 / MinGW linux build system. "Builds fine". Have not run
the result.
I have not done anything resembling proper code review. Just basic
"this is only changing _WIN32 code, and doesn't seem to do bad stuff
with pointers".
What I do not really like is the inflation of the code with
if (!tt->wintun)
statements now. I think this should be refactored out into an
"open_tun_wintun()" and an "open_tun_tap_windows()" function, where
all the bits that are now inside an "if (!tt->wintun)" get their
own function with less if() and less nesting.
commit e64b4a9e68bde3cb7d878d277878fb2805040e3e
Author: Lev Stipakov
Date: Thu Nov 7 19:45:25 2019 +0200
wintun: implement opening wintun device
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Simon Rozman <simon@rozman.si>
Message-Id: <1573148729-27339-4-git-send-email-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19029.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Hi, > What I do not really like is the inflation of the code with > > if (!tt->wintun) > > statements now. I think this should be refactored out into an > "open_tun_wintun()" and an "open_tun_tap_windows()" function, where > all the bits that are now inside an "if (!tt->wintun)" get their > own function with less if() and less nesting. > I agree. I have sent a (yet another) patch (https://patchwork.openvpn.net/patch/918/ ) which refactors open_tun() method, which now looks like this: tun_open_device(tt, dev_node, &device_guid); if (tt->wintun) { wintun_register_ring_buffer(tt); } else { tuntap_post_open(tt, device_guid); } Note that it requires 6 of 7 wintun patches to be merged. Alternatively I can rebase wintun 4/5/6 patches on top of this one.
Hi, On Thu, Nov 7, 2019 at 12:49 PM Lev Stipakov <lstipakov@gmail.com> wrote: > From: Lev Stipakov <lev@openvpn.net> > > To open wintun device, we cannot use "\\.\Global\Wintun<luid>" > path as before. To get device path which we supply to CreateFile, > we have to use SetupAPI to: > > - enumerate network adapters with "wintun" as component id > - for each adapter save its guid > - open device information set > - for each item in set > - open corresponding registry key to get net_cfg_instance_id > - get symbolic link name of device interface by instance id > - path will be symbolic link name of device instance matched with > adapter's guid > > See > https://github.com/OpenVPN/openvpn3/blob/master/openvpn/tun/win/tunutil.hpp > and > > https://github.com/WireGuard/wireguard-go/blob/master/tun/wintun/wintun_windows.go > for > implementation examples. > > Signed-off-by: Lev Stipakov <lev@openvpn.net> > This also has been ACKed and merged, but two questions that may need some attention: @@ -5541,7 +5668,8 @@ void > open_tun(const char *dev, const char *dev_type, const char *dev_node, > struct tuntap *tt) > { > struct gc_arena gc = gc_new(); > - char device_path[256]; > + char tuntap_device_path[256]; > + char *path = NULL; > const char *device_guid = NULL; > DWORD len; > bool dhcp_masq = false; > @@ -5571,6 +5699,8 @@ open_tun(const char *dev, const char *dev_type, > const char *dev_node, struct tun > { > const struct tap_reg *tap_reg = get_tap_reg(&gc); > const struct panel_reg *panel_reg = get_panel_reg(&gc); > + const struct device_instance_id_interface > *device_instance_id_interface = get_device_instance_id_interface(&gc); > + > char actual_buffer[256]; > > at_least_one_tap_win(tap_reg); > @@ -5586,24 +5716,22 @@ open_tun(const char *dev, const char *dev_type, > const char *dev_node, struct tun > } > > /* Open Windows TAP-Windows adapter */ > - openvpn_snprintf(device_path, sizeof(device_path), "%s%s%s", > + openvpn_snprintf(tuntap_device_path, > sizeof(tuntap_device_path), "%s%s%s", > USERMODEDEVICEDIR, > 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 > - ); > + tt->hand = CreateFile(tuntap_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) > { > - msg(M_ERR, "CreateFile failed on TAP device: %s", > device_path); > + msg(M_ERR, "CreateFile failed on TAP device: %s", > tuntap_device_path); > Doesn't this mean that if --dev-node is specified, we'll open tapwindows adapter with that name even if "--window-driver wintun" is specified? The open may succeed but subsequent wintun-specific processing will fail. } > } > else > @@ -5613,43 +5741,78 @@ open_tun(const char *dev, const char *dev_type, > const char *dev_node, struct tun > /* Try opening all TAP devices until we find one available */ > while (true) > { > + bool is_picked_device_wintun = false; > device_guid = get_unspecified_device_guid(device_number, > actual_buffer, > > sizeof(actual_buffer), > tap_reg, > panel_reg, > + > &is_picked_device_wintun, > &gc); > > if (!device_guid) > { > - msg(M_FATAL, "All TAP-Windows adapters on this system > are currently in use."); > + msg(M_FATAL, "All %s adapters on this system are > currently in use.", tt->wintun ? "wintun" : "TAP - Windows"); > If I'm not mistaken wintun device can be opened multiple times, so we'll never get the "All wintun adapters on this system...." error. Instead, open will succeed here and something else may fail later. FILE_SHARE_READ = 0 will not save us when the driver does not enforce it. Selva <div dir="ltr"><div>Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 7, 2019 at 12:49 PM 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">From: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> <br> To open wintun device, we cannot use "\\.\Global\Wintun<luid>"<br> path as before. To get device path which we supply to CreateFile,<br> we have to use SetupAPI to:<br> <br> - enumerate network adapters with "wintun" as component id<br> - for each adapter save its guid<br> - open device information set<br> - for each item in set<br> - open corresponding registry key to get net_cfg_instance_id<br> - get symbolic link name of device interface by instance id<br> - path will be symbolic link name of device instance matched with adapter's guid<br> <br> See <a href="https://github.com/OpenVPN/openvpn3/blob/master/openvpn/tun/win/tunutil.hpp" rel="noreferrer" target="_blank">https://github.com/OpenVPN/openvpn3/blob/master/openvpn/tun/win/tunutil.hpp</a> and<br> <a href="https://github.com/WireGuard/wireguard-go/blob/master/tun/wintun/wintun_windows.go" rel="noreferrer" target="_blank">https://github.com/WireGuard/wireguard-go/blob/master/tun/wintun/wintun_windows.go</a> for<br> implementation examples.<br> <br> Signed-off-by: Lev Stipakov <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br></blockquote><div><br></div><div>This also has been ACKed and merged, but two questions that may need some attention:<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"> @@ -5541,7 +5668,8 @@ void<br> open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tuntap *tt)<br> {<br> struct gc_arena gc = gc_new();<br> - char device_path[256];<br> + char tuntap_device_path[256];<br> + char *path = NULL;<br> const char *device_guid = NULL;<br> DWORD len;<br> bool dhcp_masq = false;<br> @@ -5571,6 +5699,8 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun<br> {<br> const struct tap_reg *tap_reg = get_tap_reg(&gc);<br> const struct panel_reg *panel_reg = get_panel_reg(&gc);<br> + const struct device_instance_id_interface *device_instance_id_interface = get_device_instance_id_interface(&gc);<br> +<br> char actual_buffer[256];<br> <br> at_least_one_tap_win(tap_reg);<br> @@ -5586,24 +5716,22 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun<br> }<br> <br> /* Open Windows TAP-Windows adapter */<br> - openvpn_snprintf(device_path, sizeof(device_path), "%s%s%s",<br> + openvpn_snprintf(tuntap_device_path, sizeof(tuntap_device_path), "%s%s%s",<br> USERMODEDEVICEDIR,<br> device_guid,<br> TAP_WIN_SUFFIX);<br> <br> - tt->hand = CreateFile(<br> - device_path,<br> - GENERIC_READ | GENERIC_WRITE,<br> - 0, /* was: FILE_SHARE_READ */<br> - 0,<br> - OPEN_EXISTING,<br> - FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED,<br> - 0<br> - );<br> + tt->hand = CreateFile(tuntap_device_path,<br> + GENERIC_READ | GENERIC_WRITE,<br> + 0, /* was: FILE_SHARE_READ */<br> + 0,<br> + OPEN_EXISTING,<br> + FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED,<br> + 0);<br> <br> if (tt->hand == INVALID_HANDLE_VALUE)<br> {<br> - msg(M_ERR, "CreateFile failed on TAP device: %s", device_path);<br> + msg(M_ERR, "CreateFile failed on TAP device: %s", tuntap_device_path);<br></blockquote><div><br></div><div>Doesn't this mean that if --dev-node is specified, we'll open tapwindows adapter</div><div>with that name even if "--window-driver wintun" is specified? The open may succeed</div><div>but subsequent wintun-specific processing will fail.</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"> }<br> }<br> else<br> @@ -5613,43 +5741,78 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun<br> /* Try opening all TAP devices until we find one available */<br> while (true)<br> {<br> + bool is_picked_device_wintun = false;<br> device_guid = get_unspecified_device_guid(device_number,<br> actual_buffer,<br> sizeof(actual_buffer),<br> tap_reg,<br> panel_reg,<br> + &is_picked_device_wintun,<br> &gc);<br> <br> if (!device_guid)<br> {<br> - msg(M_FATAL, "All TAP-Windows adapters on this system are currently in use.");<br> + msg(M_FATAL, "All %s adapters on this system are currently in use.", tt->wintun ? "wintun" : "TAP - Windows");<br></blockquote><div><br></div><div>If I'm not mistaken wintun device can be opened multiple times, so we'll never get the<br>"All wintun adapters on this system...." error. Instead, open will succeed here and </div><div>something else may fail later. FILE_SHARE_READ = 0 will not save us when the driver</div><div>does not enforce it.</div><div><br></div><div>Selva</div></div></div>
Hi, Doesn't this mean that if --dev-node is specified, we'll open tapwindows > adapter > with that name even if "--window-driver wintun" is specified? The open may > succeed > but subsequent wintun-specific processing will fail. > --dev-node is not (yet) supported and open will fail (just re-tested). > If I'm not mistaken wintun device can be opened multiple times, so we'll > never get the > "All wintun adapters on this system...." error. > Well, we get it if wintun driver is not installed :) but yep, open will succeed. Instead, open will succeed here and something else may fail later. > Right after opening we register ring buffers, and that will fail (with a friendly message): ERROR: Failed to register ring buffers: 1247 We should probably make it even more user friendly and write something like "make sure wintun adapter is not in use". Wireguard creates wintun adapter on demand (on connect) and openvpn during installation and then picks the first one found on connect. Maybe that logic could be more clever to not to pick adapter which is currently used by wireguard. I haven't looked yet into running wireguard and openvpn side by side (or multiple openvpn instances with wintun), just tested that wg and openvpn could co-exist without problems on the same machine.
Hi, On Tue, Nov 19, 2019 at 3:50 AM Lev Stipakov <lstipakov@gmail.com> wrote: > Hi, > > Doesn't this mean that if --dev-node is specified, we'll open tapwindows >> adapter >> with that name even if "--window-driver wintun" is specified? The open >> may succeed >> but subsequent wintun-specific processing will fail. >> > > --dev-node is not (yet) supported and open will fail (just re-tested). > I did a quick test and found the open to succeed with a misleading message that Wintun device opened (though it opened the tap device) and then fails on register ring buffers. I haven't applied all patches as yet, so not sure whether this is changed in later patches. If I'm not mistaken wintun device can be opened multiple times, so we'll >> never get the >> "All wintun adapters on this system...." error. >> > Well, we get it if wintun driver is not installed :) but yep, open will > succeed. > Instead, open will succeed here and something else may fail later. >> > > Right after opening we register ring buffers, and that will fail (with a > friendly message): > > ERROR: Failed to register ring buffers: 1247 > > We should probably make it even more user friendly and write something > like "make sure wintun adapter is not in use". > Apart from the error message, there is a larger issue especially when we use iservice. In that case, we have to preserve privilege separation and allowing a user to open a device handle in use by another has to be avoided. The service could keep a lock on devices it manages, but I have no idea how we are going to lock out devices opened by other means (say instances started by automatic service, or wireguard or some other third party product). Can we get the wintun folks to support exclusive open (say FILE_SHARE_READ = 0) or expose the internal count of number of open handles? That would also make it easier to fix this and provide better error messages. > Wireguard creates wintun adapter on demand (on connect) and openvpn during > installation and then > picks the first one found on connect. Maybe that logic could be more > clever to not to pick adapter which > is currently used by wireguard. > > I haven't looked yet into running wireguard and openvpn side by side (or > multiple openvpn instances > with wintun), just tested that wg and openvpn could co-exist without > problems on the same machine. > Hmm.. if multiple openvpn instances are not tested this is not ready for review yet, is it? Again, a quick test shows that, with multiple openvpn instances, it does open an already opened device and then fails while registering ring buffers. Selva <div dir="ltr"><div>Hi,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 19, 2019 at 3:50 AM Lev Stipakov <<a href="mailto:lstipakov@gmail.com">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><div dir="ltr"><br></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"><div dir="ltr"><div class="gmail_quote"><div>Doesn't this mean that if --dev-node is specified, we'll open tapwindows adapter</div><div>with that name even if "--window-driver wintun" is specified? The open may succeed</div><div>but subsequent wintun-specific processing will fail.</div></div></div></blockquote><div><br></div><div>--dev-node is not (yet) supported and open will fail (just re-tested).</div></div></div></blockquote><div><br></div><div class="gmail_quote">I did a quick test and found the open to succeed with a misleading message that Wintun device opened (though it opened the tap device) and then fails on register ring buffers. I haven't applied all patches as yet, so not sure whether this is changed in later patches.<br></div><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><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>If I'm not mistaken wintun device can be opened multiple times, so we'll never get the<br>"All wintun adapters on this system...." error.</div></div></div></blockquote></blockquote><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>Well, we get it if wintun driver is not installed :) but yep, open will succeed.</div></blockquote><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"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>Instead, open will succeed here and something else may fail later.</div></blockquote></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><br></div><div>Right after opening we register ring buffers, and that will fail (with a friendly message):</div><div><br></div><div>ERROR: Failed to register ring buffers: 1247</div><div><br></div><div>We should probably make it even more user friendly and write something like "make sure wintun adapter is not in use".</div></blockquote></div><div><br></div><div>Apart from the error message, there is a larger issue especially when we use iservice. In that case, we have to preserve privilege separation and allowing a user to open a device handle in use by another has to be avoided. The service could keep a lock on devices it manages, but I have no idea how we are going to lock out devices opened by other means (say instances started by automatic service, or wireguard or some other third party product).<br></div><div><br></div><div>Can we get the wintun folks to support exclusive open (say FILE_SHARE_READ = 0) or expose the internal count of number of open handles? That would also make it easier to fix this and provide better error messages.<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><br></div><div>Wireguard creates wintun adapter on demand (on connect) and openvpn during installation and then</div><div>picks the first one found on connect. Maybe that logic could be more clever to not to pick adapter which</div><div>is currently used by wireguard.</div><div><br></div><div>I haven't looked yet into running wireguard and openvpn side by side (or multiple openvpn instances</div><div>with wintun), just tested that wg and openvpn could co-exist without problems on the same machine.</div></div></div></blockquote><div><br></div><div>Hmm.. if multiple openvpn instances are not tested this is not ready for review yet, is it?<br></div><div><br></div><div>Again, a quick test shows that, with multiple openvpn instances, it does open an already opened device and then fails while registering ring buffers.</div><div><br></div><div>Selva<br></div></div></div>
Hi, Apart from the error message, there is a larger issue especially when we > use iservice. In that case, we have to preserve privilege separation and > allowing a user to open a device handle in use by another has to be avoided. > Do you see it as a security issue when handle can be opened by another process? To read / write to tunnel one needs to register ring buffers, and this call will fail for any other process. Am I missing something here? > Can we get the wintun folks to support exclusive open (say FILE_SHARE_READ > = 0) or expose the internal count of number of open handles? That would > also make it easier to fix this and provide better error messages. > We could maybe submit a patch for that. I agree it would be nice to have. > Hmm.. if multiple openvpn instances are not tested this is not ready for > review yet, is it? > I don't think so. I would not expect new tun driver (or code around it) to support all features of old driver right from the beginning. Wintun doesn't replace tap-windows6, so if someone requires functionality which wintun doesn't yet support, one could always switch to previous driver. Again, a quick test shows that, with multiple openvpn instances, it does > open an already opened device and then fails while registering ring buffers. > Yeah, it would be nice to eventual support multiple tunnels, but I don't think that this is "must have" in the first version. <div dir="auto"><div dir="ltr"><div>Hi,</div><div><br></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"><div dir="ltr"><div class="gmail_quote"><div>Apart from the error message, there is a larger issue especially when we use iservice. In that case, we have to preserve privilege separation and allowing a user to open a device handle in use by another has to be avoided.</div></div></div></blockquote><div><br></div><div>Do you see it as a security issue when handle can be opened by another process?</div><div><br></div><div>To read / write to tunnel one needs to register ring buffers, and this call will fail for any other process. Am I missing something here?</div><div> </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>Can we get the wintun folks to support exclusive open (say FILE_SHARE_READ = 0) or expose the internal count of number of open handles? That would also make it easier to fix this and provide better error messages.</div></div></div></blockquote><div><br></div><div>We could maybe submit a patch for that.</div><div dir="auto"><br></div><div dir="auto">I agree it would be nice to have.</div><div> </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>Hmm.. if multiple openvpn instances are not tested this is not ready for review yet, is it?<br></div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I don't think so. I would not expect new tun driver (or code around it) to support all features of old driver right from the beginning. </div><div dir="auto"><br></div><div dir="auto">Wintun doesn't replace tap-windows6, so if someone requires functionality which wintun doesn't yet support, one could always switch to previous driver. </div><div dir="auto"><br></div><div dir="ltr"><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"><div dir="ltr"><div class="gmail_quote"><div>Again, a quick test shows that, with multiple openvpn instances, it does open an already opened device and then fails while registering ring buffers.</div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Yeah, it would be nice to eventual support multiple tunnels, but I don't think that this is "must have" in the first version. </div></div>
Hi Lev, On Tue, Nov 19, 2019 at 12:23 PM Lev Stipakov <lstipakov@gmail.com> wrote: > Hi, > > Apart from the error message, there is a larger issue especially when we >> use iservice. In that case, we have to preserve privilege separation and >> allowing a user to open a device handle in use by another has to be avoided. >> > > Do you see it as a security issue when handle can be opened by another > process? > I don't know the internals of wintun to know that for sure. > > To read / write to tunnel one needs to register ring buffers, and this > call will fail for any other process. Am I missing something here? > Hopefully, Simon can confirm whether that provides a sufficient safety net. Selva <div dir="ltr"><div>Hi Lev,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 19, 2019 at 12:23 PM 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="auto"><div dir="ltr"><div>Hi,</div><div><br></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"><div dir="ltr"><div class="gmail_quote"><div>Apart from the error message, there is a larger issue especially when we use iservice. In that case, we have to preserve privilege separation and allowing a user to open a device handle in use by another has to be avoided.</div></div></div></blockquote><div><br></div><div>Do you see it as a security issue when handle can be opened by another process?</div></div></div></div></blockquote><div><br></div><div>I don't know the internals of wintun to know that for sure. <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="auto"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>To read / write to tunnel one needs to register ring buffers, and this call will fail for any other process. Am I missing something here?</div></div></div></div></blockquote><div><br></div><div>Hopefully, Simon can confirm whether that provides a sufficient safety net.<br></div></div><div class="gmail_quote"><br></div><div class="gmail_quote">Selva<br></div></div>
Hi,
The Wintun doesn't create its own communication I/O device. Running a separate NdisRegisterDeviceEx() device came with a big can of worms, so we decided not to run our own, but rather piggy-back on the existing NDIS one from the NdisMRegisterMiniportDriver() that we tap on. Technically, we could limit it to a single client handle, but this device is primarily used by Windows to manage the adapter, therefore we must not tweak it in this direction.
As Lev said: Wintun adds an IOCTL call to register ring buffers and it is this IOCTL that has a refcounting implemented to deny secondary connection attempts. Unfortunately for OpenVPN, this IOCTL is restricted to privileged processes only. You cannot use it to detect which Wintun adapters are in use by an unprivileged openvpn.exe process. WireGuard is very strict about which TUN adapter it will use – it will never ever use a lottery to pick one.
We have more options here:
A. Wintun adapter can be explicitly chosen by configuration only (by name or GUID). With tapctl.exe utility users can make an adapter with predefined name and put this name into .ovpn. If the Wintun adapter is not specified explicitly => configuration error. Less lottery; predictable tunnel-adapter mapping when running multiple tunnels; disturbed users because there is an inconsistency between TAP-Windows6 and Wintun adapter selection logic.
B. The tapctl.exe code is in OpenVPN repo already and could be integrated in openvpn.exe and interactive service to create Wintun adapter on demand. But, you have to take responsibility to clean what you have created. Pretty much the same as A), but nicer for user. In the long term, I'd suggest this "create adapter if doesn't exist" approach even for TAP-Windows6 adapters.
C. Use adapter media state to see if particular Wintun adapter is already in use or not. Mind that registering buffer also sets MediaConnectStateConnected, and stays connected until its client closes handle or dies. This allows OpenVPN to extend its "use first available adapter" approach to Wintun adapters.
Best regards,
Simon
From: Selva Nair <selva.nair@gmail.com>
Sent: Tuesday, November 19, 2019 7:03 PM
To: Lev Stipakov <lstipakov@gmail.com>
Cc: Lev Stipakov <lev@openvpn.net>; openvpn-devel <openvpn-devel@lists.sourceforge.net>
Subject: Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi Lev,
On Tue, Nov 19, 2019 at 12:23 PM Lev Stipakov <lstipakov@gmail.com <mailto:lstipakov@gmail.com> > wrote:
Hi,
Apart from the error message, there is a larger issue especially when we use iservice. In that case, we have to preserve privilege separation and allowing a user to open a device handle in use by another has to be avoided.
Do you see it as a security issue when handle can be opened by another process?
I don't know the internals of wintun to know that for sure.
To read / write to tunnel one needs to register ring buffers, and this call will fail for any other process. Am I missing something here?
Hopefully, Simon can confirm whether that provides a sufficient safety net.
Selva
<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 15 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
code
{mso-style-priority:99;
font-family:Consolas;}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0cm;
margin-right:0cm;
margin-bottom:0cm;
margin-left:36.0pt;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.EmailStyle19
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;
mso-fareast-language:EN-US;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:70.85pt 70.85pt 70.85pt 70.85pt;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:182279862;
mso-list-type:hybrid;
mso-list-template-ids:514901922 -1427472230 69468185 69468187 69468175 69468185 69468187 69468175 69468185 69468187;}
@list l0:level1
{mso-level-number-format:alpha-upper;
mso-level-text:"%1\)";
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level3
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l0:level4
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level5
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level6
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l0:level7
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level8
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level9
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
ol
{margin-bottom:0cm;}
ul
{margin-bottom:0cm;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body lang=SL link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'>Hi,<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'>The Wintun doesn't create its own communication I/O device. Running a separate NdisRegisterDeviceEx() device came with a big can of worms, so we decided not to run our own, but rather piggy-back on the existing NDIS one from the NdisMRegisterMiniportDriver() that we tap on. Technically, we could limit it to a single client handle, but this device is primarily used by Windows to manage the adapter, therefore we must not tweak it in this direction.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'>As Lev said: Wintun adds an IOCTL call to register ring buffers and it is this IOCTL that has a refcounting implemented to deny secondary connection attempts. Unfortunately for OpenVPN, this IOCTL is restricted to privileged processes only. You cannot use it to detect which Wintun adapters are in use by an unprivileged openvpn.exe process. WireGuard is very strict about which TUN adapter it will use – it will never ever use a lottery to pick one.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'>We have more options here:<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'><o:p> </o:p></span></p><ol style='margin-top:0cm' start=1 type=A><li class=MsoListParagraph style='margin-left:0cm;mso-list:l0 level1 lfo1'><span lang=EN-GB style='mso-fareast-language:EN-US'>Wintun adapter can be explicitly chosen by configuration only (by name or GUID). With tapctl.exe utility users can make an adapter with predefined name and put this name into .ovpn. If the Wintun adapter is not specified explicitly => configuration error. Less lottery; predictable tunnel-adapter mapping when running multiple tunnels; disturbed users because there is an inconsistency between TAP-Windows6 and Wintun adapter selection logic.<o:p></o:p></span></li><li class=MsoListParagraph style='margin-left:0cm;mso-list:l0 level1 lfo1'><span lang=EN-GB style='mso-fareast-language:EN-US'>The tapctl.exe code is in OpenVPN repo already and could be integrated in openvpn.exe and interactive service to create Wintun adapter on demand. But, you have to take responsibility to clean what you have created. Pretty much the same as A), but nicer for user. In the long term, I'd suggest this "create adapter if doesn't exist" approach even for TAP-Windows6 adapters.<o:p></o:p></span></li><li class=MsoListParagraph style='margin-left:0cm;mso-list:l0 level1 lfo1'><span lang=EN-GB style='mso-fareast-language:EN-US'>Use adapter media state to see if particular Wintun adapter is already in use or not. Mind that registering buffer also sets MediaConnectStateConnected, and stays connected until its client closes handle or dies. This allows OpenVPN to extend its "use first available adapter" approach to Wintun adapters.<o:p></o:p></span></li></ol><p class=MsoNormal><span lang=EN-GB style='mso-fareast-language:EN-US'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-GB>Best regards,<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-GB>Simon<o:p></o:p></span></p><p class=MsoNormal><span style='mso-fareast-language:EN-US'><o:p> </o:p></span></p><div style='border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt'><div><div style='border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm'><p class=MsoNormal><b><span lang=EN-US>From:</span></b><span lang=EN-US> Selva Nair <selva.nair@gmail.com> <br><b>Sent:</b> Tuesday, November 19, 2019 7:03 PM<br><b>To:</b> Lev Stipakov <lstipakov@gmail.com><br><b>Cc:</b> Lev Stipakov <lev@openvpn.net>; openvpn-devel <openvpn-devel@lists.sourceforge.net><br><b>Subject:</b> Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device<o:p></o:p></span></p></div></div><p class=MsoNormal><o:p> </o:p></p><div><div><p class=MsoNormal>Hi Lev,<o:p></o:p></p></div><p class=MsoNormal><o:p> </o:p></p><div><div><p class=MsoNormal>On Tue, Nov 19, 2019 at 12:23 PM Lev Stipakov <<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>> wrote:<o:p></o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm'><div><div><div><p class=MsoNormal>Hi,<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm'><div><div><div><p class=MsoNormal>Apart from the error message, there is a larger issue especially when we use iservice. In that case, we have to preserve privilege separation and allowing a user to open a device handle in use by another has to be avoided.<o:p></o:p></p></div></div></div></blockquote><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Do you see it as a security issue when handle can be opened by another process?<o:p></o:p></p></div></div></div></div></blockquote><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>I don't know the internals of wintun to know that for sure. <o:p></o:p></p></div><div><p class=MsoNormal> <o:p></o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm'><div><div><div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>To read / write to tunnel one needs to register ring buffers, and this call will fail for any other process. Am I missing something here?<o:p></o:p></p></div></div></div></div></blockquote><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Hopefully, Simon can confirm whether that provides a sufficient safety net.<o:p></o:p></p></div></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Selva<o:p></o:p></p></div></div></div></div></body></html>
Hi, > We have more options here: > Can we combine B and C? 1) During installation, we create one adapter, as we do now. 2) On connection we enumerate adapters and pick the first one which is not connected. 3) If we could not find free adapter (another VPN connection is already running), we create one on demand.
Hi, (cc:ed to -devel) > I would vote for B and not the combination. > > With wintun there is no backwards compatibility requirements, so we could > use a cleaner, consistent and simpler approach (i.e B). Do not create any > adapter during installation and dynamically create a temporary adapter at > connection time. > My main concern with creating tun adapter on demand is that it is far from instant: $ time ./tapctl.exe create --hwid wintun {D9F56B7A-3054-4ADC-9457-61030F0B469D} real 0m2,090s I don't think we want to add it to connection time. Creating one persistent adapter per profile (as viscosity does for > tapwindows and wireguard seems to do for wintun) > If I remember right, wireguard doesn't create persistent adapter, instead it adds/removes it on demand. If --dev-node is specified, we open the named adapter which the user is > supposed to have created as we do for tapwindows. > Yes, I plan to add support for --dev-node for wintun.
I know. The tap.c code needs an upgrade, not to evaluate all drivers, but just compatible drivers when creating a new adapter. This speeds things a lot. There's a flag that needs to be changed. Somewhere deep on my TODO lists.
I would suggest against temporary adapters on Windows. This is OK on Linux, but Windows is not Linux. Apart from the time penalty you already discovered, there are more annoying issues with temporary adapters: adapters always get a new GUID on creation, making NLA think it's a new network each time. This will keep resetting the firewall profile assigned to the adapter and piling up network IDs in user registry at each connection attempt. Also, changing bindings on adapters don't persist (vmWare, VirtualBox, Hyper-V bindings comes to my mind)…
I am testing an intermediate approach to adapter management on Windows, inspired by Microsoft's own way how Windows handle dial-up adapters:
1. if adapter "My VPN Connection" doesn't exist, create it.
2. else enable it
3. use it
4. disable it
In my observation, this is the most streamlined approach on Windows. It avoids adapter creation burden, while still (re)creates it on demand. Disabling adapter when it is not in use releases the resources, removes the adapter from the network stack, halts the adapter, unloads the driver when the last of the adapters (of the same driver) is disabled… Like adapter was deleted, only its settings persist in registry.
An annoyance here is, the adapters pile up over time. On multi-user computers, OpenVPN GUI don't have a complete overview which one are stale (no corresponding .ovpn file exist anymore) to clean them. Cleaning requires elevation… An admin user may use Device manager to clean them, uninstaller should clean them. Both are not ideal. On test computers with lots of configuration fuss that might be a problem. On production computers this shouldn't be a big issue. Mind that even Hyper-V doesn't clean up adapters if you install and uninstall it on Windows.
Maybe we could save the absolute path to .ovpn file in the adapter registry to assign it to a particular profile. Interactive service could periodically delete orphaned adapters. I'm not worried about .ovpn-less tunnels: users running openvpn.exe specifying all settings in the command line know what they're doing.
Best regards,
Simon
From: Lev Stipakov <lstipakov@gmail.com>
Sent: Monday, November 25, 2019 10:04 AM
To: Selva Nair <selva.nair@gmail.com>
Cc: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Subject: Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi,
(cc:ed to -devel)
I would vote for B and not the combination.
With wintun there is no backwards compatibility requirements, so we could use a cleaner, consistent and simpler approach (i.e B). Do not create any adapter during installation and dynamically create a temporary adapter at connection time.
My main concern with creating tun adapter on demand is that it is far from instant:
$ time ./tapctl.exe create --hwid wintun
{D9F56B7A-3054-4ADC-9457-61030F0B469D}
real 0m2,090s
I don't think we want to add it to connection time.
Creating one persistent adapter per profile (as viscosity does for tapwindows and wireguard seems to do for wintun)
If I remember right, wireguard doesn't create persistent adapter, instead it adds/removes it on demand.
If --dev-node is specified, we open the named adapter which the user is supposed to have created as we do for tapwindows.
Yes, I plan to add support for --dev-node for wintun.
Hi On Mon, Nov 25, 2019 at 4:03 AM Lev Stipakov <lstipakov@gmail.com> wrote: > Hi, > > (cc:ed to -devel) > > >> I would vote for B and not the combination. >> >> With wintun there is no backwards compatibility requirements, so we could >> use a cleaner, consistent and simpler approach (i.e B). Do not create any >> adapter during installation and dynamically create a temporary adapter at >> connection time. >> > > My main concern with creating tun adapter on demand is that it is far from > instant: > > $ time ./tapctl.exe create --hwid wintun > {D9F56B7A-3054-4ADC-9457-61030F0B469D} > > real 0m2,090s > > I don't think we want to add it to connection time. > No, we don't want that. I do realize what I wrote earlier was far from ideal. I had forgotten my travails two years back attempting to add dynamic tap adapter creation to OpnVPN.... As an easy way for supporting multiple tunnels, can we treat success of open-device + ring-buffer-registration calls together as a successful open of the adapter, and move on to the next one when that fails? That would be very similar to what we do for tapwindows right now. To assist users, when we fail with no free adapters, we could add a feature to the GUI to add a new one using the RunAsAdmin shell-execute we already have support for. Will need an improved devcon/tapinstall that does not reset all adapters when a new one is added, except when unavoidable due to driver version change. Simon's suggestion of persistent adapters as required and kept disabled when not in use sounds interesting. As some providers do supply 100's of configs, I think we should refrain from creating one adapter per ovpn, though. They should be using multiple --remote lines in a single config, but, for that to work well, we need to improve --management-remote option to provide a friendly UI for remote selection. Selva <div dir="ltr"><div>Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Nov 25, 2019 at 4:03 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 class="gmail_quote"><div>Hi,</div><div><br></div><div>(cc:ed to -devel)</div><div> </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>I would vote for B and not the combination.<br></div><div><br></div><div>With wintun there is no backwards compatibility requirements, so we could use a cleaner, consistent and simpler approach (i.e B). Do not create any adapter during installation and dynamically create a temporary adapter at connection time.</div></div></div></blockquote><div><br></div><div>My main concern with creating tun adapter on demand is that it is far from instant:</div><div><br></div><div>$ time ./tapctl.exe create --hwid wintun<br>{D9F56B7A-3054-4ADC-9457-61030F0B469D}<br><br>real 0m2,090s<br></div><div><br></div><div>I don't think we want to add it to connection time.</div></div></div></blockquote><div><br></div><div>No, we don't want that.<br></div><div><br></div><div>I do realize what I wrote earlier was far from ideal. I had forgotten my travails two years back attempting to add dynamic tap adapter creation to OpnVPN....<br></div><div><br></div><div>As an easy way for supporting multiple tunnels, can we treat success of open-device + ring-buffer-registration calls together as a successful open of the adapter, and move on to the next one when that fails? That would be very similar to what we do for tapwindows right now.</div><div><br></div><div>To assist users, when we fail with no free adapters, we could add a feature to the GUI to add a new one using the RunAsAdmin shell-execute we already have support for. Will need an improved devcon/tapinstall that does not reset all adapters when a new one is added, except when unavoidable due to driver version change. <br></div><div><br></div><div>Simon's suggestion of persistent adapters as required and kept disabled when not in use sounds interesting. As some providers do supply 100's of configs, I think we should refrain from creating one adapter per ovpn, though. They should be using multiple --remote lines in a single config, but, for that to work well, we need to improve --management-remote option to provide a friendly UI for remote selection.<br></div><div><br></div><div>Selva</div></div></div>
Hi, On Mon, Nov 25, 2019 at 09:44:19AM +0000, Simon Rozman wrote: > 1. if adapter "My VPN Connection" doesn't exist, create it. > 2. else enable it > 3. use it > 4. disable it > An annoyance here is, the adapters pile up over time. On multi-user > computers, OpenVPN GUI don't have a complete overview which one are > stale (no corresponding .ovpn file exist anymore) to clean them. I like the initial approach, but the "pile-up" is annoying. What about "do this for .ovpn configs that contain a --dev-node setting, and otherwise just try to enumerate 'OpenVPN Wintun 1', 'OpenVPN Wintun 2', ... and create those as-needed? This should keep the numbers down... gert
diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index fbb86ad..a091ffc 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -139,5 +139,5 @@ openvpn_LDADD = \ $(OPTIONAL_DL_LIBS) if WIN32 openvpn_SOURCES += openvpn_win32_resources.rc block_dns.c block_dns.h -openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm -lfwpuclnt -lrpcrt4 -lncrypt +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 e77f026..9ffef9f 100644 --- a/src/openvpn/openvpn.vcxproj +++ b/src/openvpn/openvpn.vcxproj @@ -91,7 +91,7 @@ </ClCompile> <ResourceCompile /> <Link> - <AdditionalDependencies>legacy_stdio_definitions.lib;Ncrypt.lib;libssl.lib;libcrypto.lib;lzo2.lib;pkcs11-helper.dll.lib;gdi32.lib;ws2_32.lib;wininet.lib;crypt32.lib;iphlpapi.lib;winmm.lib;Fwpuclnt.lib;Rpcrt4.lib;%(AdditionalDependencies)</AdditionalDependencies> + <AdditionalDependencies>legacy_stdio_definitions.lib;Ncrypt.lib;libssl.lib;libcrypto.lib;lzo2.lib;pkcs11-helper.dll.lib;gdi32.lib;ws2_32.lib;wininet.lib;crypt32.lib;iphlpapi.lib;winmm.lib;Fwpuclnt.lib;Rpcrt4.lib;setupapi.lib;%(AdditionalDependencies)</AdditionalDependencies> <AdditionalLibraryDirectories>$(OPENSSL_HOME)/lib;$(LZO_HOME)/lib;$(PKCS11H_HOME)/lib;%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories> <SubSystem>Console</SubSystem> </Link> @@ -117,7 +117,7 @@ </ClCompile> <ResourceCompile /> <Link> - <AdditionalDependencies>legacy_stdio_definitions.lib;Ncrypt.lib;libssl.lib;libcrypto.lib;lzo2.lib;pkcs11-helper.dll.lib;gdi32.lib;ws2_32.lib;wininet.lib;crypt32.lib;iphlpapi.lib;winmm.lib;Fwpuclnt.lib;Rpcrt4.lib;%(AdditionalDependencies)</AdditionalDependencies> + <AdditionalDependencies>legacy_stdio_definitions.lib;Ncrypt.lib;libssl.lib;libcrypto.lib;lzo2.lib;pkcs11-helper.dll.lib;gdi32.lib;ws2_32.lib;wininet.lib;crypt32.lib;iphlpapi.lib;winmm.lib;Fwpuclnt.lib;Rpcrt4.lib;setupapi.lib;%(AdditionalDependencies)</AdditionalDependencies> <AdditionalLibraryDirectories>$(OPENSSL_HOME)/lib;$(LZO_HOME)/lib;$(PKCS11H_HOME)/lib;%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories> <SubSystem>Console</SubSystem> </Link> @@ -301,4 +301,4 @@ <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" /> <ImportGroup Label="ExtensionTargets"> </ImportGroup> -</Project> \ No newline at end of file +</Project> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index ce23eb6..37bf065 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -58,6 +58,9 @@ #ifdef _WIN32 +const static GUID GUID_DEVCLASS_NET = { 0x4d36e972L, 0xe325, 0x11ce, { 0xbf, 0xc1, 0x08, 0x00, 0x2b, 0xe1, 0x03, 0x18 } }; +const static GUID GUID_DEVINTERFACE_NET = { 0xcac88484, 0x7515, 0x4c03, { 0x82, 0xe6, 0x71, 0xa8, 0x7a, 0xba, 0xc3, 0x61 } }; + /* #define SIMULATE_DHCP_FAILED */ /* simulate bad DHCP negotiation */ #define NI_TEST_FIRST (1<<0) @@ -3444,7 +3447,123 @@ tun_finalize( return ret; } -const struct tap_reg * +static const struct device_instance_id_interface * +get_device_instance_id_interface(struct gc_arena* gc) +{ + HDEVINFO dev_info_set; + DWORD err; + struct device_instance_id_interface *first = NULL; + struct device_instance_id_interface *last = NULL; + + dev_info_set = SetupDiGetClassDevsEx(&GUID_DEVCLASS_NET, NULL, NULL, DIGCF_PRESENT, NULL, NULL, NULL); + if (dev_info_set == INVALID_HANDLE_VALUE) + { + err = GetLastError(); + msg(M_FATAL, "Error [%u] opening device information set key: %s", (unsigned int)err, strerror_win32(err, gc)); + } + + for (DWORD i = 0;; ++i) + { + SP_DEVINFO_DATA device_info_data; + BOOL res; + HKEY dev_key; + char net_cfg_instance_id_string[] = "NetCfgInstanceId"; + char net_cfg_instance_id[256]; + char device_instance_id[256]; + DWORD len; + DWORD data_type; + LONG status; + ULONG dev_interface_list_size; + CONFIGRET cr; + struct buffer dev_interface_list; + + ZeroMemory(&device_info_data, sizeof(SP_DEVINFO_DATA)); + device_info_data.cbSize = sizeof(SP_DEVINFO_DATA); + res = SetupDiEnumDeviceInfo(dev_info_set, i, &device_info_data); + if (!res) + { + if (GetLastError() == ERROR_NO_MORE_ITEMS) + { + break; + } + else + { + continue; + } + } + + dev_key = SetupDiOpenDevRegKey(dev_info_set, &device_info_data, DICS_FLAG_GLOBAL, 0, DIREG_DRV, KEY_QUERY_VALUE); + if (dev_key == INVALID_HANDLE_VALUE) + { + continue; + } + + len = sizeof(net_cfg_instance_id); + data_type = REG_SZ; + status = RegQueryValueEx(dev_key, + net_cfg_instance_id_string, + NULL, + &data_type, + net_cfg_instance_id, + &len); + if (status != ERROR_SUCCESS) + { + goto next; + } + + len = sizeof(device_instance_id); + res = SetupDiGetDeviceInstanceId(dev_info_set, &device_info_data, device_instance_id, len, &len); + if (!res) + { + goto next; + } + + cr = CM_Get_Device_Interface_List_Size(&dev_interface_list_size, + (LPGUID)& GUID_DEVINTERFACE_NET, + device_instance_id, + CM_GET_DEVICE_INTERFACE_LIST_PRESENT); + + if (cr != CR_SUCCESS) + { + goto next; + } + + dev_interface_list = alloc_buf_gc(dev_interface_list_size, gc); + cr = CM_Get_Device_Interface_List((LPGUID)& GUID_DEVINTERFACE_NET, device_instance_id, + BPTR(&dev_interface_list), + dev_interface_list_size, + CM_GET_DEVICE_INTERFACE_LIST_PRESENT); + if (cr != CR_SUCCESS) + { + goto next; + } + + struct device_instance_id_interface* dev_if; + ALLOC_OBJ_CLEAR_GC(dev_if, struct device_instance_id_interface, gc); + dev_if->net_cfg_instance_id = string_alloc(net_cfg_instance_id, gc); + dev_if->device_interface_list = string_alloc(BSTR(&dev_interface_list), gc); + + /* link into return list */ + if (!first) + { + first = dev_if; + } + if (last) + { + last->next = dev_if; + } + last = dev_if; + + next: + RegCloseKey(dev_key); + } + + SetupDiDestroyDeviceInfoList(dev_info_set); + + return first; +} + +static const struct tap_reg * get_tap_reg(struct gc_arena *gc) { HKEY adapter_key; @@ -3541,11 +3660,13 @@ get_tap_reg(struct gc_arena *gc) if (status == ERROR_SUCCESS && data_type == REG_SZ) { if (!strcmp(component_id, TAP_WIN_COMPONENT_ID) || - !strcmp(component_id, "root\\" TAP_WIN_COMPONENT_ID)) + !strcmp(component_id, "root\\" TAP_WIN_COMPONENT_ID) || + !strcmp(component_id, WINTUN_COMPONENT_ID)) { struct tap_reg *reg; ALLOC_OBJ_CLEAR_GC(reg, struct tap_reg, gc); reg->guid = string_alloc(net_cfg_instance_id, gc); + reg->wintun = !strcmp(component_id, WINTUN_COMPONENT_ID); /* link into return list */ if (!first) @@ -3569,7 +3690,7 @@ get_tap_reg(struct gc_arena *gc) return first; } -const struct panel_reg * +static const struct panel_reg * get_panel_reg(struct gc_arena *gc) { LONG status; @@ -3776,7 +3897,7 @@ show_tap_win_adapters(int msglev, int warnlev) const struct tap_reg *tap_reg = get_tap_reg(&gc); const struct panel_reg *panel_reg = get_panel_reg(&gc); - msg(msglev, "Available TAP-WIN32 adapters [name, GUID]:"); + msg(msglev, "Available TAP-WIN32 / Wintun adapters [name, GUID, driver]:"); /* loop through each TAP-Windows adapter registry entry */ for (tr = tap_reg; tr != NULL; tr = tr->next) @@ -3788,7 +3909,7 @@ show_tap_win_adapters(int msglev, int warnlev) { if (!strcmp(tr->guid, pr->guid)) { - msg(msglev, "'%s' %s", pr->name, tr->guid); + msg(msglev, "'%s' %s %s", pr->name, tr->guid, tr->wintun ? "wintun" : "tap-windows6"); ++links; } } @@ -3907,6 +4028,7 @@ get_unspecified_device_guid(const int device_number, int actual_name_size, const struct tap_reg *tap_reg_src, const struct panel_reg *panel_reg_src, + bool *wintun, struct gc_arena *gc) { const struct tap_reg *tap_reg = tap_reg_src; @@ -3956,6 +4078,10 @@ get_unspecified_device_guid(const int device_number, /* Save GUID for return value */ ret = alloc_buf_gc(256, gc); buf_printf(&ret, "%s", tap_reg->guid); + if (wintun != NULL) + { + *wintun = tap_reg->wintun; + } return BSTR(&ret); } @@ -4733,6 +4859,7 @@ tap_allow_nonadmin_access(const char *dev_node) sizeof(actual_buffer), tap_reg, panel_reg, + NULL, &gc); if (!device_guid) @@ -5267,9 +5394,9 @@ netsh_get_id(const char *dev_node, struct gc_arena *gc) } else { - guid = get_unspecified_device_guid(0, BPTR(&actual), BCAP(&actual), tap_reg, panel_reg, gc); + guid = get_unspecified_device_guid(0, BPTR(&actual), BCAP(&actual), tap_reg, panel_reg, NULL, gc); - if (get_unspecified_device_guid(1, NULL, 0, tap_reg, panel_reg, gc)) /* ambiguous if more than one TAP-Windows adapter */ + if (get_unspecified_device_guid(1, NULL, 0, tap_reg, panel_reg, NULL, gc)) /* ambiguous if more than one TAP-Windows adapter */ { guid = NULL; } @@ -5541,7 +5668,8 @@ void open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tuntap *tt) { struct gc_arena gc = gc_new(); - char device_path[256]; + char tuntap_device_path[256]; + char *path = NULL; const char *device_guid = NULL; DWORD len; bool dhcp_masq = false; @@ -5571,6 +5699,8 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun { const struct tap_reg *tap_reg = get_tap_reg(&gc); const struct panel_reg *panel_reg = get_panel_reg(&gc); + const struct device_instance_id_interface *device_instance_id_interface = get_device_instance_id_interface(&gc); + char actual_buffer[256]; at_least_one_tap_win(tap_reg); @@ -5586,24 +5716,22 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun } /* Open Windows TAP-Windows adapter */ - openvpn_snprintf(device_path, sizeof(device_path), "%s%s%s", + openvpn_snprintf(tuntap_device_path, sizeof(tuntap_device_path), "%s%s%s", USERMODEDEVICEDIR, 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 - ); + tt->hand = CreateFile(tuntap_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) { - msg(M_ERR, "CreateFile failed on TAP device: %s", device_path); + msg(M_ERR, "CreateFile failed on TAP device: %s", tuntap_device_path); } } else @@ -5613,43 +5741,78 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun /* Try opening all TAP devices until we find one available */ while (true) { + bool is_picked_device_wintun = false; device_guid = get_unspecified_device_guid(device_number, actual_buffer, sizeof(actual_buffer), tap_reg, panel_reg, + &is_picked_device_wintun, &gc); if (!device_guid) { - msg(M_FATAL, "All TAP-Windows adapters on this system are currently in use."); + msg(M_FATAL, "All %s adapters on this system are currently in use.", tt->wintun ? "wintun" : "TAP - Windows"); } - /* Open Windows TAP-Windows adapter */ - openvpn_snprintf(device_path, sizeof(device_path), "%s%s%s", - USERMODEDEVICEDIR, - 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->wintun) + { + const struct device_instance_id_interface* dev_if; + + if (!is_picked_device_wintun) + { + /* wintun driver specified but picked adapter is not wintun, proceed to next one */ + goto next; + } + + path = NULL; + for (dev_if = device_instance_id_interface; dev_if != NULL; dev_if = dev_if->next) + { + if (strcmp(dev_if->net_cfg_instance_id, device_guid) == 0) + { + path = (char *)dev_if->device_interface_list; + break; + } + } + if (path == NULL) + { + goto next; + } + } + else + { + if (is_picked_device_wintun) + { + /* tap-windows6 driver specified but picked adapter is wintun, proceed to next one */ + goto next; + } + + /* Open Windows TAP-Windows adapter */ + openvpn_snprintf(tuntap_device_path, sizeof(tuntap_device_path), "%s%s%s", + USERMODEDEVICEDIR, + device_guid, + TAP_WIN_SUFFIX); + path = tuntap_device_path; + } + + tt->hand = CreateFile(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) { - msg(D_TUNTAP_INFO, "CreateFile failed on TAP device: %s", device_path); + msg(D_TUNTAP_INFO, "CreateFile failed on %s device: %s", tt->wintun ? "wintun" : "TAP", tuntap_device_path); } else { break; } + next: device_number++; } } @@ -5659,10 +5822,11 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun tt->actual_name = string_alloc(actual_buffer, NULL); } - msg(M_INFO, "TAP-WIN32 device [%s] opened: %s", tt->actual_name, device_path); + msg(M_INFO, "%s device [%s] opened: %s", tt->wintun ? "Wintun" : "TAP-WIN32", tt->actual_name, path); tt->adapter_index = get_adapter_index(device_guid); /* get driver version info */ + if (!tt->wintun) { ULONG info[3]; CLEAR(info); @@ -5702,6 +5866,7 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun } /* get driver MTU */ + if (!tt->wintun) { ULONG mtu; if (DeviceIoControl(tt->hand, TAP_WIN_IOCTL_GET_MTU, @@ -5761,7 +5926,7 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun /* set point-to-point mode if TUN device */ - if (tt->type == DEV_TYPE_TUN) + if ((tt->type == DEV_TYPE_TUN) && !tt->wintun) { if (!tt->did_ifconfig_setup && !tt->did_ifconfig_ipv6_setup) { @@ -5816,7 +5981,7 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun /* should we tell the TAP-Windows driver to masquerade as a DHCP server as a means * of setting the adapter address? */ - if (dhcp_masq) + if (dhcp_masq && !tt->wintun) { uint32_t ep[4]; @@ -5894,6 +6059,7 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun } /* set driver media status to 'connected' */ + if (!tt->wintun) { ULONG status = TRUE; if (!DeviceIoControl(tt->hand, TAP_WIN_IOCTL_SET_MEDIA_STATUS, diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index df935f6..19cab7e 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -27,6 +27,8 @@ #ifdef _WIN32 #include <winioctl.h> #include <tap-windows.h> +#include <setupapi.h> +#include <cfgmgr32.h> #endif #include "buffer.h" @@ -38,6 +40,10 @@ #include "misc.h" #include "networking.h" +#ifdef _WIN32 +#define WINTUN_COMPONENT_ID "wintun" +#endif + #if defined(_WIN32) || defined(TARGET_ANDROID) #define TUN_ADAPTER_INDEX_INVALID ((DWORD)-1) @@ -340,6 +346,7 @@ route_order(void) struct tap_reg { const char *guid; + bool wintun; struct tap_reg *next; }; @@ -350,6 +357,13 @@ struct panel_reg struct panel_reg *next; }; +struct device_instance_id_interface +{ + const char *net_cfg_instance_id; + const char *device_interface_list; + struct device_instance_id_interface *next; +}; + int ascii2ipset(const char *name); const char *ipset2ascii(int index);