[Openvpn-devel,5/5] Detect TAP interfaces with root-enumerated hardware ID

Message ID 20181016102627.18676-5-simon@rozman.si
State Accepted
Headers show
Series [Openvpn-devel,1/5] Set output name to libopenvpnmsica.dll in MSVC builds too | expand

Commit Message

Simon Rozman Oct. 15, 2018, 11:26 p.m. UTC
This patch extends the TAP interface enumerating to detect the TAP
interfaces registered using "root\tap0901" hardware ID. Before, only TAP
interfaces with legacy "tap0901" HWID were detected by openvpn.exe.

The openvpnmsica.dll and tapctl.exe install TAP interfaces using root-
enumerated HWIDs, and were not detected by openvpn.exe.
---
 src/openvpn/tun.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Kristof Provost via Openvpn-devel Nov. 8, 2018, 11:50 a.m. UTC | #1
Samuli, LGTM.

-----Original Message-----
From: Simon Rozman <simon@rozman.si> 
Sent: Tuesday, October 16, 2018 3:26 AM
To: openvpn-devel@lists.sourceforge.net
Subject: [Openvpn-devel] [PATCH 5/5] Detect TAP interfaces with root-enumerated hardware ID

This patch extends the TAP interface enumerating to detect the TAP
interfaces registered using "root\tap0901" hardware ID. Before, only TAP
interfaces with legacy "tap0901" HWID were detected by openvpn.exe.

The openvpnmsica.dll and tapctl.exe install TAP interfaces using root-
enumerated HWIDs, and were not detected by openvpn.exe.
---
 src/openvpn/tun.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 948fd17d..5fde2ab8 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -3524,7 +3524,8 @@ get_tap_reg(struct gc_arena *gc)
 
                 if (status == ERROR_SUCCESS && data_type == REG_SZ)
                 {
-                    if (!strcmp(component_id, TAP_WIN_COMPONENT_ID))
+                    if (!strcmp(component_id, TAP_WIN_COMPONENT_ID) ||
+                        !strcmp(component_id, "root\\" TAP_WIN_COMPONENT_ID))
                     {
                         struct tap_reg *reg;
                         ALLOC_OBJ_CLEAR_GC(reg, struct tap_reg, gc);
Gert Doering Jan. 17, 2019, 5:18 a.m. UTC | #2
Hi,

On Tue, Oct 16, 2018 at 12:26:27PM +0200, Simon Rozman wrote:
> This patch extends the TAP interface enumerating to detect the TAP
> interfaces registered using "root\tap0901" hardware ID. Before, only TAP
> interfaces with legacy "tap0901" HWID were detected by openvpn.exe.
> 
> The openvpnmsica.dll and tapctl.exe install TAP interfaces using root-
> enumerated HWIDs, and were not detected by openvpn.exe.

I do not see a specific reason to *not* apply this patch, but I'm wondering
if you could shed some background light on the changed behaviour in
openvpnmsica.dll and tapctl.exe?

I'm not sure if we're going to care for someone who uses a 2.5 msi installer
to set up a TAP interface and then runs a 2.3 openvpn.exe on top of it and
wonders why it isn't finding the TAP driver - but the question might come
up, and I like having good answers... :-)

gert
Gert Doering Jan. 17, 2019, 5:29 a.m. UTC | #3
Acked-by: Gert Doering <gert@greenie.muc.de>

We have a LGTM from Jon as well, but I'm taking this on me - because I
decided I want this in 2.4 as well.  Unlikely as it may be, we *will*
have people that happen to have a tap adapter on their system and try
to run 2.4.x binaries on it...  so this is long-term compatibility.

Your patch has been applied to the master and release/2.4 branch.

commit 6e03336d8a4aada12c4950a9683a483470fe4f15 (master)
commit eef040cc909bb0e9bae793b61c2f4f0da7e109d5 (release/2.4)
Author: Simon Rozman
Date:   Tue Oct 16 12:26:27 2018 +0200

     Detect TAP interfaces with root-enumerated hardware ID

     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20181016102627.18676-5-simon@rozman.si>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17762.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Simon Rozman Jan. 19, 2019, 6:38 a.m. UTC | #4
Hi,

> On Tue, Oct 16, 2018 at 12:26:27PM +0200, Simon Rozman wrote:
> > This patch extends the TAP interface enumerating to detect the TAP
> > interfaces registered using "root\tap0901" hardware ID. Before, only
> > TAP interfaces with legacy "tap0901" HWID were detected by
> openvpn.exe.
> >
> > The openvpnmsica.dll and tapctl.exe install TAP interfaces using root-
> > enumerated HWIDs, and were not detected by openvpn.exe.
> 
> I do not see a specific reason to *not* apply this patch, but I'm
> wondering if you could shed some background light on the changed
> behaviour in openvpnmsica.dll and tapctl.exe?

The tapctl.exe and openvpnmsica.dll use a simplified interface installation
compared to devcon.exe/tapinstall.exe. They assume the driver is already
installed and they don't require INF file to create a TAP interface.
However, when the interface is installed this way, Windows reports its
hardware ID as "root\\tap0901". Whereas, tapinstall.exe installed TAP
interfaces report their HWID as "tap0901". That's about the only difference
I noticed.

(Tested with driver versions 9.21.x and later.)

Rather than exploring, what causes the difference and fuelled by Microsoft's
recommendation to use root-enumerated HWIDs anyway, I proposed to extend the
openvpn.exe's TAP interface detection to accept both: "tap0901" _and_
"root\\tap0901" interfaces.

> I'm not sure if we're going to care for someone who uses a 2.5 msi
> installer to set up a TAP interface and then runs a 2.3 openvpn.exe on
> top of it and wonders why it isn't finding the TAP driver - but the
> question might come up, and I like having good answers... :-)

Suggest them to remove the TAP interface and reinstall it using
tapinstall.exe. The tapinstall.exe should still be able to install 9.22 and
later TUN interfaces.

Or even better: apply this patch to 2.4 and 2.3 branches (it's a really
minor change) and ask them to compile and use the patched version of 2.3. :)

Best regards,
Simon
Gert Doering Jan. 20, 2019, 2:04 a.m. UTC | #5
Hi,

On Sat, Jan 19, 2019 at 05:38:27PM +0000, Simon Rozman wrote:
> > > The openvpnmsica.dll and tapctl.exe install TAP interfaces using root-
> > > enumerated HWIDs, and were not detected by openvpn.exe.
> > 
> > I do not see a specific reason to *not* apply this patch, but I'm
> > wondering if you could shed some background light on the changed
> > behaviour in openvpnmsica.dll and tapctl.exe?
> 
> The tapctl.exe and openvpnmsica.dll use a simplified interface installation
> compared to devcon.exe/tapinstall.exe. They assume the driver is already
> installed and they don't require INF file to create a TAP interface.
> However, when the interface is installed this way, Windows reports its
> hardware ID as "root\\tap0901". Whereas, tapinstall.exe installed TAP
> interfaces report their HWID as "tap0901". That's about the only difference
> I noticed.

O-kay... :-) - I do not know enough about driver *installation* on Windows
to know whether this is all good or there might be dragons lurking.  We'll
have to test quite a bit, it seems :-)

> (Tested with driver versions 9.21.x and later.)
> 
> Rather than exploring, what causes the difference and fuelled by Microsoft's
> recommendation to use root-enumerated HWIDs anyway, I proposed to extend the
> openvpn.exe's TAP interface detection to accept both: "tap0901" _and_
> "root\\tap0901" interfaces.

Works for me.  Which is why I ...

> Or even better: apply this patch to 2.4 and 2.3 branches (it's a really
> minor change) and ask them to compile and use the patched version of 2.3. :)

... applied it to 2.4 as well.  

If someone struggles with 2.3 on windows (beyond XP), my empathy for them
will be close to zero.  2.4 is so much better, especially on windows, that
there is no reason to stick to 2.3... (on Linux distributions, sticking
to the "distro provided package" might give people 2.3.x for years to 
come :-( - but this is no argument on Windows).

gert

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 948fd17d..5fde2ab8 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -3524,7 +3524,8 @@  get_tap_reg(struct gc_arena *gc)
 
                 if (status == ERROR_SUCCESS && data_type == REG_SZ)
                 {
-                    if (!strcmp(component_id, TAP_WIN_COMPONENT_ID))
+                    if (!strcmp(component_id, TAP_WIN_COMPONENT_ID) ||
+                        !strcmp(component_id, "root\\" TAP_WIN_COMPONENT_ID))
                     {
                         struct tap_reg *reg;
                         ALLOC_OBJ_CLEAR_GC(reg, struct tap_reg, gc);