Re: [Openvpn-devel] [PATCH v8-master] Add Windows DNS Leak fix using WFP ('block-outside-dns')

2015-12-11 Thread Lev Stipakov 

On 10.12.2015 18:49, ValdikSS wrote:


I'd better go with just
closing the engine without deleting everything. I don't see any
drawbacks, that should be perfectly OK for a dynamic session.

Is this correct, Lev? If yes, I'll push v9 today or tomorrow morning.


Removing non-working FwpmSubLayerDeleteByKey0 call is indeed step into 
right direction, rest is up to you :) I already mentioned above what I 
would do.


Just for the record - this patch does not work on my only win10 box with 
DNS leak - it blocks leaks but each DNS request takes 10 seconds, so I 
cannot give an ACK. On the other hand, it works for others in this 
discussion and is optional in any case (should not break things). So I 
won't say NACK either.


-Lev

Re: [Openvpn-devel] [PATCH v8-master] Add Windows DNS Leak fix using WFP ('block-outside-dns')

2015-12-10 Thread Selva Nair 
Hi,

On Thu, Dec 10, 2015 at 3:18 PM, ValdikSS  wrote:

> Use dynamic sessions.
>
> Many applications add filtering policy objects at start, and then delete
> these objects at stop. By using a dynamic session, you guarantee that these
> objects are deleted even if the application crashes. Furthermore, simply
> closing the engine handle at stop is more efficient than making individual
> calls to delete each object.
>
> Going by that just closing the engine appears good enough. I never tested
really changing the tapluid between a SIGHUP restart (how to trigger that)
to know whether any old filters hang around after a  _uninit. Anyway, those
are very unlikely cases and we cant guarad against everything up front.

I would say go for v9 with a simple closing of the engine.

Thanks,

Selva

Re: [Openvpn-devel] [PATCH v8-master] Add Windows DNS Leak fix using WFP ('block-outside-dns')

2015-12-10 Thread ValdikSS 
I'm not totally sure about that, but I suppose it shouldn't leak.
Here's what Microsoft's Best Practice says:
> Use dynamic sessions.
>
> Many applications add filtering policy objects at start, and then delete 
> these objects at stop. By using a dynamic session, you guarantee that these 
> objects
> are deleted even if the application crashes. Furthermore, simply closing the 
> engine handle at stop is more efficient than making individual calls to delete
> each object.
>
https://msdn.microsoft.com/en-us/library/windows/desktop/bb442411%28v=vs.85%29.aspx

> If /session/.*flags* is set to *FWPM_SESSION_FLAG_DYNAMIC*, any WFP objects 
> added during the session are automatically deleted when the session ends. If 
> the
> session is not dynamic, the caller needs to explicitly delete all WFP objects 
> added during the session.
https://technet.microsoft.com/ru-ru/aa364040


On 12/10/2015 08:17 PM, Selva Nair wrote:
>
> On Thu, Dec 10, 2015 at 11:49 AM, ValdikSS  > wrote:
>
> Provided it doesn't leak memory. As the current implementation of 
> wfp_add_filter does return the id of the added filter (in ) , its 
> easy to save them
> into some globals and delete all the filters in wfp_uninit.
>
> However, If you are sure there is no leak, just closing the engine is fine.
>
> Other than this v8 is good to go.
>
> Selva
>
>
> --
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH v8-master] Add Windows DNS Leak fix using WFP ('block-outside-dns')

2015-12-10 Thread Selva Nair 
On Thu, Dec 10, 2015 at 11:49 AM, ValdikSS  wrote:

> It's a very minor actually, FwpmSubLayerDeleteByKey0 is indeed fails every
> time but it doesn't break anything since we close the engine right after.
> Lev suggested to use a correct way with making a filter GUID and deleting
> it before deleting sublayer, but I'd better go with just closing the engine
> without deleting everything. I don't see any drawbacks, that should be
> perfectly OK for a dynamic session.


Provided it doesn't leak memory. As the current implementation of
wfp_add_filter does return the id of the added filter (in ) , its
easy to save them into some globals and delete all the filters in
wfp_uninit.

However, If you are sure there is no leak, just closing the engine is fine.

Other than this v8 is good to go.

Selva

Re: [Openvpn-devel] [PATCH v8-master] Add Windows DNS Leak fix using WFP ('block-outside-dns')

2015-12-10 Thread ValdikSS 
No, I'm afraid it would fail by default  since DNS queries are (usually) made 
from svchost.exe. Whitelisting openvpn.exe is done to not to break OpenVPN on 
UDP
port 53. I could be incorrect tho.

On 12/10/2015 07:31 PM, Selva Nair wrote:
>
> On Thu, Dec 10, 2015 at 11:24 AM, ValdikSS  > wrote:
>
> Yes, I meant to leave the block all and permit openvpn.exe filters always on 
> and delete/re-add only the filters for the tun/tap interface. Well, its 
> probably
> just a cosmetic change, so no strong opinion.
>
> Selva
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH v8-master] Add Windows DNS Leak fix using WFP ('block-outside-dns')

2015-12-10 Thread Gert Doering 
Hi,

On Thu, Dec 10, 2015 at 11:55:16AM +0200, Lev Stipakov wrote:
> Sorry for the late response.
[..]
> FwpmSubLayerDeleteByKey0 will likely fail if there is a filter 
> associated with sublayer. On the other side, FwpmEngineClose0 seems to 
> be enough to remove blocking.

So, is this something that needs to be fixed before we release 2.3.9, or
can it wait for improvement later on?

I just agreed with Samuli that we are going to release 2.3.9 on Monday,
and this should go in - so either I get a v9 "with nice and shiny" until
Sunday, or someone tells me "v8 is good enough, go for it"...

Thanks :-)

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH v8-master] Add Windows DNS Leak fix using WFP ('block-outside-dns')

2015-12-10 Thread ValdikSS 
That would break name resolver on reconnection if remote is a hostname.

On 12/10/2015 06:53 PM, Selva Nair wrote:
>
> On Thu, Dec 10, 2015 at 4:55 AM, Lev Stipakov  > wrote:
>
> That sounds useful for yet another reason as well. Its only necessary to 
> delete and recreate the permit filters when the tun restarts. The block 
> filters need
> be set up only once.
>
> Selva
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH v8-master] Add Windows DNS Leak fix using WFP ('block-outside-dns')

2015-12-10 Thread Lev Stipakov 

Hi,

Sorry for the late response.

+bool
+win_wfp_uninit()
+{
+dmsg (D_LOW, "Uninitializing WFP");
+if (m_hEngineHandle) {
+FwpmSubLayerDeleteByKey0(m_hEngineHandle, _subLayerGUID);
+CLEAR(m_subLayerGUID);
+FwpmEngineClose0(m_hEngineHandle);
+m_hEngineHandle = NULL;


FwpmSubLayerDeleteByKey0 will likely fail if there is a filter 
associated with sublayer. On the other side, FwpmEngineClose0 seems to 
be enough to remove blocking.


I think the correct way would be to create GUID for filter same way as 
you did for sublayer and copy it to filter.filterKey. In uninit method, 
before deleting sublayer you call FwpmFilterDeleteByKey0 with that guid.


-Lev

[Openvpn-devel] [PATCH v8-master] Add Windows DNS Leak fix using WFP ('block-outside-dns')

2015-12-09 Thread ValdikSS 
This option blocks all out-of-tunnel communication on TCP/UDP port 53 (except
for OpenVPN itself), preventing DNS Leaks on Windows 8.1 and 10.
---
 doc/openvpn.8   |  12 ++-
 src/openvpn/Makefile.am |   2 +-
 src/openvpn/init.c  |  17 
 src/openvpn/openvpn.vcxproj |   4 +-
 src/openvpn/options.c   |  10 ++
 src/openvpn/options.h   |   1 +
 src/openvpn/win32.c | 216 
 src/openvpn/win32.h |   3 +
 8 files changed, 260 insertions(+), 5 deletions(-)
 mode change 100755 => 100644 src/openvpn/openvpn.vcxproj

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index 9889540..7e73073 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -1129,8 +1129,8 @@ When used with
 .B \-\-client
 or
 .B \-\-pull,
-accept options pushed by server EXCEPT for routes and dhcp options
-like DNS servers.
+accept options pushed by server EXCEPT for routes, block-outside-dns and dhcp
+options like DNS servers.

 When used on the client, this option effectively bars the
 server from adding routes to the client's routing table,
@@ -5574,6 +5574,14 @@ adapter list to the syslog or log file after the TUN/TAP 
adapter
 has been brought up and any routes have been added.
 .\"*
 .TP
+.B \-\-block\-outside\-dns
+Block DNS servers on other network adapters to prevent
+DNS leaks. This option prevents any application from accessing
+TCP or UDP port 53 except one inside the tunnel. It uses
+Windows Filtering Platform (WFP) and works on Windows Vista or
+later.
+.\"*
+.TP
 .B \-\-dhcp\-renew
 Ask Windows to renew the TAP adapter lease on startup.
 This option is normally unnecessary, as Windows automatically
diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
index c840f16..c55a520 100644
--- a/src/openvpn/Makefile.am
+++ b/src/openvpn/Makefile.am
@@ -127,5 +127,5 @@ openvpn_LDADD = \
$(OPTIONAL_DL_LIBS)
 if WIN32
 openvpn_SOURCES += openvpn_win32_resources.rc
-openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm
+openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm 
-lfwpuclnt -lrpcrt4
 endif
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index c5c0ab6..9f3da60 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1468,6 +1468,15 @@ do_open_tun (struct context *c)
   "up",
   c->c2.es);

+#if defined(WIN32)
+  if (c->options.block_outside_dns)
+  {
+dmsg (D_LOW, "Blocking outside DNS");
+if (!win_wfp_block_dns(c->c1.tuntap->adapter_index))
+msg (M_FATAL, "Blocking DNS failed!");
+  }
+#endif
+
   /* possibly add routes */
   if ((route_order() == ROUTE_AFTER_TUN) && 
(!c->options.route_delay_defined))
do_route (>options, c->c1.route_list, c->c1.route_ipv6_list,
@@ -1596,6 +1605,14 @@ do_close_tun (struct context *c, bool force)
   "down",
   c->c2.es);

+#if defined(WIN32)
+if (c->options.block_outside_dns)
+{
+if (!win_wfp_uninit())
+msg (M_FATAL, "Uninitialising WFP failed!");
+}
+#endif
+
  /* actually close tun/tap device based on --down-pre flag */
  if (c->options.down_pre)
do_close_tun_simple (c);
diff --git a/src/openvpn/openvpn.vcxproj b/src/openvpn/openvpn.vcxproj
old mode 100755
new mode 100644
index b117b0b..821c46c
--- a/src/openvpn/openvpn.vcxproj
+++ b/src/openvpn/openvpn.vcxproj
@@ -64,7 +64,7 @@
   
$(SOURCEBASE);%(AdditionalIncludeDirectories)
 
 
-  
libeay32.lib;ssleay32.lib;lzo2.lib;pkcs11-helper.dll.lib;gdi32.lib;ws2_32.lib;wininet.lib;crypt32.lib;iphlpapi.lib;winmm.lib;%(AdditionalDependencies)
+  
libeay32.lib;ssleay32.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)
   
$(OPENSSL_HOME)/lib;$(LZO_HOME)/lib;$(PKCS11H_HOME)/lib;%(AdditionalLibraryDirectories)
   true
   Console
@@ -89,7 +89,7 @@
   
$(SOURCEBASE);%(AdditionalIncludeDirectories)
 
 
-  
libeay32.lib;ssleay32.lib;lzo2.lib;pkcs11-helper.dll.lib;gdi32.lib;ws2_32.lib;wininet.lib;crypt32.lib;iphlpapi.lib;winmm.lib;%(AdditionalDependencies)
+  
libeay32.lib;ssleay32.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)
   
$(OPENSSL_HOME)/lib;$(LZO_HOME)/lib;$(PKCS11H_HOME)/lib;%(AdditionalLibraryDirectories)
   true
   Console
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 36290a0..4b98275 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -704,6 +704,9 @@ static const char usage_message[] =
   "   optional parameter controls the initial state of 
ex.\n"