[Openvpn-devel] [PATCH v2] Replace WIN32 by _WIN32

2016-11-13 Thread Gert Doering 
With c99, "WIN32" is no longer automatically defined when (cross-)building
for Windows, and proper compilation relies on including ,
before checking the macro.  "_WIN32" is the official define that is
guaranteed to be defined by the compiler itself, no includes are needed.

So, mechanically change all occurrances of "WIN32" to "_WIN32".

While at it, get rid of unused WIN32_0_1 #define in syshead.h

See also: 
http://nadeausoftware.com/articles/2012/01/c_c_tip_how_use_compiler_predefined_macros_detect_operating_system#WindowsCygwinnonPOSIXandMinGW

Trac #746

v2: rebased to master, merge the console[_builtin].c changes

Signed-off-by: Gert Doering 
---
 doc/doxygen/openvpn.doxyfile |  2 +-
 include/openvpn-plugin.h.in  |  2 +-
 m4/ax_socklen_t.m4   |  2 +-
 src/compat/compat-gettimeofday.c |  4 ++--
 src/compat/compat-inet_ntop.c|  2 +-
 src/compat/compat-inet_pton.c|  2 +-
 src/openvpn/block_dns.c  |  2 +-
 src/openvpn/block_dns.h  |  2 +-
 src/openvpn/common.h |  2 +-
 src/openvpn/console_builtin.c|  6 +++---
 src/openvpn/cryptoapi.c  |  2 +-
 src/openvpn/error.c  | 10 +-
 src/openvpn/error.h  |  6 +++---
 src/openvpn/event.c  |  8 
 src/openvpn/event.h  |  4 ++--
 src/openvpn/fdmisc.c |  4 ++--
 src/openvpn/fdmisc.h |  2 +-
 src/openvpn/forward.c|  4 ++--
 src/openvpn/init.c   | 36 ++--
 src/openvpn/manage.c | 18 +-
 src/openvpn/manage.h |  2 +-
 src/openvpn/misc.c   | 16 
 src/openvpn/misc.h   |  2 +-
 src/openvpn/openvpn.c|  8 
 src/openvpn/options.c| 40 
 src/openvpn/options.h|  6 +++---
 src/openvpn/otime.h  |  8 
 src/openvpn/platform.c   | 20 ++--
 src/openvpn/platform.h   |  2 +-
 src/openvpn/plugin.c |  8 
 src/openvpn/plugin.h |  2 +-
 src/openvpn/route.c  | 26 +-
 src/openvpn/route.h  | 10 +-
 src/openvpn/sig.c|  8 
 src/openvpn/sig.h|  2 +-
 src/openvpn/socket.c | 40 
 src/openvpn/socket.h | 18 +-
 src/openvpn/ssl.c|  4 ++--
 src/openvpn/ssl_backend.h|  2 +-
 src/openvpn/ssl_mbedtls.c|  2 +-
 src/openvpn/ssl_openssl.c|  2 +-
 src/openvpn/syshead.h| 26 --
 src/openvpn/tun.c| 24 
 src/openvpn/tun.h| 18 +-
 src/openvpn/win32.c  |  2 +-
 src/openvpn/win32.h  |  2 +-
 46 files changed, 205 insertions(+), 215 deletions(-)

diff --git a/doc/doxygen/openvpn.doxyfile b/doc/doxygen/openvpn.doxyfile
index 7a02028..a7d9728 100644
--- a/doc/doxygen/openvpn.doxyfile
+++ b/doc/doxygen/openvpn.doxyfile
@@ -235,7 +235,7 @@ EXPAND_ONLY_PREDEF = NO
 SEARCH_INCLUDES= YES
 INCLUDE_PATH   =
 INCLUDE_FILE_PATTERNS  =
-PREDEFINED = WIN32 NTLM USE_LZO ENABLE_FRAGMENT P2MP P2MP_SERVER 
ENABLE_CRYPTO ENABLE_CRYPTO_OPENSSL ENABLE_PLUGIN ENABLE_MANAGEMENT ENABLE_OCC 
HAVE_GETTIMEOFDAY
+PREDEFINED = _WIN32 NTLM USE_LZO ENABLE_FRAGMENT P2MP P2MP_SERVER 
ENABLE_CRYPTO ENABLE_CRYPTO_OPENSSL ENABLE_PLUGIN ENABLE_MANAGEMENT ENABLE_OCC 
HAVE_GETTIMEOFDAY
 EXPAND_AS_DEFINED  =
 SKIP_FUNCTION_MACROS   = YES
 #---
diff --git a/include/openvpn-plugin.h.in b/include/openvpn-plugin.h.in
index b03a9df..34ad18b 100644
--- a/include/openvpn-plugin.h.in
+++ b/include/openvpn-plugin.h.in
@@ -154,7 +154,7 @@ typedef void *openvpn_plugin_handle_t;
 /*
  * For Windows (needs to be modified for MSVC)
  */
-#if defined(WIN32) && !defined(OPENVPN_PLUGIN_H)
+#if defined(_WIN32) && !defined(OPENVPN_PLUGIN_H)
 # define OPENVPN_EXPORT __declspec(dllexport)
 #else
 # define OPENVPN_EXPORT
diff --git a/m4/ax_socklen_t.m4 b/m4/ax_socklen_t.m4
index cd7cad8..b420a17 100644
--- a/m4/ax_socklen_t.m4
+++ b/m4/ax_socklen_t.m4
@@ -55,7 +55,7 @@ getpeername(0,0,);
],
[[
 #include 
-#ifdef WIN32
+#ifdef _WIN32
 #include 
 #else
 #include 
diff --git a/src/compat/compat-gettimeofday.c b/src/compat/compat-gettimeofday.c
index 0f32d5d..19feaae 100644
--- a/src/compat/compat-gettimeofday.c
+++ b/src/compat/compat-gettimeofday.c
@@ -32,7 +32,7 @@
 
 #include "compat.h"
 
-#ifdef WIN32
+#ifdef _WIN32
 /*
  * NOTICE: mingw has much faster gettimeofday!
  * autoconf will set HAVE_GETTIMEOFDAY
@@ -126,6 +126,6 @@ gettimeofday (struct timeval *tv, void *tz)
return 0;
 }
 
-#endif /* WIN32 */

[Openvpn-devel] [PATCH applied] Re: Fix compilation on MinGW with -std=c99

2016-11-13 Thread Gert Doering 
Patch has been applied to the master branch.  Hope buildbot likes me again :)

commit 11eedcd0071e7185fc3011cda4703f5cc75fe979
Author: Gert Doering
Date:   Sun Nov 13 20:36:45 2016 +0100

 Fix compilation on MinGW with -std=c99

 Signed-off-by: Gert Doering 
 Acked-by: Steffan Karger 
 Message-Id: <20161113193645.73523-1-g...@greenie.muc.de>
 URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13032.html
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Fix compilation on MinGW with -std=c99

2016-11-13 Thread Steffan Karger 

On 13-11-16 20:36, Gert Doering wrote:
> commit 9223336a88bc moved the CFLAGS="-std=c99" bit in configure.ac
> before the "socklen_t" test, which relies on #ifdef WIN32 to decide
> whether to include  or  - which is no longer
> defined then, and things explode in interesting ways.
> 
> Change to _WIN32, which is the "always defined on all compilers" define
> for this.
> 
> Signed-off-by: Gert Doering 
> ---
>  m4/ax_socklen_t.m4 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/m4/ax_socklen_t.m4 b/m4/ax_socklen_t.m4
> index cd7cad8..b420a17 100644
> --- a/m4/ax_socklen_t.m4
> +++ b/m4/ax_socklen_t.m4
> @@ -55,7 +55,7 @@ getpeername(0,0,);
>   ],
>   [[
>  #include 
> -#ifdef WIN32
> +#ifdef _WIN32
>  #include 
>  #else
>  #include 
> 

ACK.  So many well-hidden bugs... Thanks for figuring this out!

-Steffan

--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Fix compilation on MinGW with -std=c99

2016-11-13 Thread Gert Doering 
commit 9223336a88bc moved the CFLAGS="-std=c99" bit in configure.ac
before the "socklen_t" test, which relies on #ifdef WIN32 to decide
whether to include  or  - which is no longer
defined then, and things explode in interesting ways.

Change to _WIN32, which is the "always defined on all compilers" define
for this.

Signed-off-by: Gert Doering 
---
 m4/ax_socklen_t.m4 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/m4/ax_socklen_t.m4 b/m4/ax_socklen_t.m4
index cd7cad8..b420a17 100644
--- a/m4/ax_socklen_t.m4
+++ b/m4/ax_socklen_t.m4
@@ -55,7 +55,7 @@ getpeername(0,0,);
],
[[
 #include 
-#ifdef WIN32
+#ifdef _WIN32
 #include 
 #else
 #include 
-- 
1.9.1


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 2/5] Add control channel encryption (--tls-crypt)

2016-11-13 Thread Steffan Karger 
Hi,

Thanks for reviewing!  Replies inline.

On 13-11-16 17:41, Arne Schwabe wrote:
> 
>> This boils down to the following on-the-wire packet format:
>>
>>-opcode- || -session_id- || -packet_id- || auth_tag || * payload *
> 
> I am pretty that opcode is *not* authenticated looking the code. Which
> is probably not a problem but should not be documented as authenticated.
> (There is a buf_advance(buf ,1) before calling the unwrap function.

No, the opcode *is* authenticated.  See e.g. tls_crypt_wrap(), which
receives a dst buffer which is prepared with the opcode and session_id,
and computes the HMAC over the contents.

(I only see a buf_advance (buf, SID_SIZE + 1) *after* the
tls_crypt_unwrap() call?)

>>Where
>>- XXX - means authenticated, and
>>* XXX * means authenticated and encrypted.
> 
> This misalignes the rest of the packet. I am not sure how big the
> penality actually is but we have DATA_V2 and COMP_V2 fix this
> misalignment. But then again ignore my comment for control channel traffic.

Yes, this is how it's always been.  I don't think that's an issue for
the (low-bandwidth) control channel traffic.

> For the packet format I think it would be very good to add a key id to
> help transisition or allow more paranoid people to have a a key per
> client. In the server you would specify all allowed valid keys together
> with their ids (or somehow emded that id/read that id from keyfile). Not
> that saying that this should be implement in the first version of the
> patch but something that we can implement later without having a
> tls-crypt and a tls-crypt v2 packet format.

Yeah, I have plans for supporting per-client keys, but those won't need
a different packet format.  Also, I'd like to support client-specific
keys for both tls-auth and tls-crypt.  *If* we need a new packet format,
I think the client-specific key patch set will be right moment to
introduce it.  (This stuff is not negotiated, which makes it less
painful to change anyway.)

>> Which is very similar to the current tls-auth packet format, and has the
>> same overhead as "--tls-auth" with "--auth SHA256".
>>
>> The use of a nonce misuse-resistant authenticated encryption scheme
>> allows us to worry less about the risks of nonce collisions.  This is
>> important, because in contrast with the data channel in TLS mode, we
>> will not be able to rotate tls-crypt keys often or fully guarantee nonce
>> uniqueness.  For non misuse-resistant modes such as GCM [1], [2], the
>> data channel in TLS mode only has to ensure that the packet counter
>> never rolls over, while tls-crypt would have to provide nonce uniqueness
>> over all control channel packets sent by all clients, for the lifetime
>> of the tls-crypt key.
>>
>> Unlike with tls-auth, no --key-direction has to be specified for
>> tls-crypt.  TLS servers always use key direction 1, and TLS clients
>> always use key direction 2, which means that client->server traffic and
>> server->client traffic always use different keys, without requiring
>> configuration.
>>
>> Using fixed, secure, encryption and authentication algorithms makes both
>> implementation and configuration easier.  If we ever want to, we can
>> extend this to support other crypto primitives.  Since tls-crypt should
>> provide privacy as well as DoS protection, these should not be made
>> negotiable.
>>
>> Security considerations:
>> 
>>
>> tls-crypt is a best-effort mechanism that aims to provide as much
>> privacy and security as possible, while staying as simple as possible.
>> The following are some security considerations for this scheme.
>>
>> 1. The same tls-crypt key is potentially shared by a lot of peers, so it
>>is quite likely to get compromised.  Once an attacker acquires the
>>tls-crypt key, this mechanism no longer provides any security against
>>the attacker.
>>
>> 2. Since many peers potentially use the tls-crypt key for a long time, a
>>lot of data might be encrypted under the tls-crypt key.  This leads
>>to two potential problems:
>>
>>* The "opcode || session id || packet id" combination might collide.
>>  This might happen in larger setups, because the session id contains
>>  just 64 bits or random.  Using the uniqueness requirement from the
> Typo: of random
> 
>>  GCM spec [3] (a collision probability of less than 2^(-32)),
>>  uniqueness is achieved when using the tls-crypt key for at most
>>  2^16 (65536) connections per process start.  (The packet id
>>  includes the daemon start time in the packet ID, which should be
>>  different after stopping and (re)starting OpenPVN.)
>>
>>  And if a collision happens, an attacker can *only* learn whether
>>  colliding packets contain the same plaintext.  Attackers will not
>>  be able to learn anything else about the plaintext (unless the
>>  attacker knows the plaintext of one of these packets, of course).
>>  Since the impact is limited, I

[Openvpn-devel] [PATCH applied] Re: Fix builds on compilers without anonymous union support

2016-11-13 Thread Gert Doering 
ACK, tested on FreeBSD 7.4 ("builds, works, happiness"), FreeBSD 10, and 
recent Linux ("were happy already", HAVE_ANONYMOUS... properly detected).

Let's see if buildbot thinks otherwise for the remaining platforms...

Your patch has been applied to the master branch.

commit 9223336a88bc065348d0ce37621bbf2b1087ba0e
Author: Steffan Karger
Date:   Sun Nov 13 19:03:23 2016 +0100

 Fix builds on compilers without anonymous union support

 Signed-off-by: Steffan Karger 
 Acked-by: Gert Doering 
 Message-Id: <1479060203-4472-1-git-send-email-stef...@karger.me>
 URL: 
http://www.mail-archive.com/search?l=mid=1479060203-4472-1-git-send-email-stef...@karger.me
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH applied] Re: Support --block-outside-dns on multiple tunnels

2016-11-13 Thread Gert Doering 
ACK.

The patch is basically the same as the previously-acked master patch
(plus code cleanups).

I have only reviewed this one, and not built a test installer and tested
it - I'll go test the snapshot as soon as buildbot makes one for me, but
given that the code changes are quite similar, I expect no surprises.

Your patch has been applied to the release/2.3 branch.

commit f65f85275aeee79ebfdee5e1e8b6f6508106
Author: Selva Nair
Date:   Sat Sep 17 00:10:39 2016 -0400

 Support --block-outside-dns on multiple tunnels

 Signed-off-by: Selva Nair 
 Acked-by: Gert Doering 
 Message-Id: <1474085439-28766-2-git-send-email-selva.n...@gmail.com>
 URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg12466.html
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Fix builds on compilers without anonymous union support

2016-11-13 Thread Steffan Karger 
The "Don't dereference type-punned pointers" patch introduced an anonymous
union, which older compilers do not support (or refuse to support when
-std=c99 is defined).  Add a configure check, and some wrapper defines to
repair builds on those compilers.

Signed-off-by: Steffan Karger 
---
 configure.ac | 34 --
 src/openvpn/mroute.h | 12 +++-
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/configure.ac b/configure.ac
index 357ba29..bba29b2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -390,6 +390,12 @@ AC_DEFINE_UNQUOTED([IPROUTE_PATH], ["$IPROUTE"], [Path to 
iproute tool])
 AC_DEFINE_UNQUOTED([ROUTE_PATH], ["$ROUTE"], [Path to route tool])
 AC_DEFINE_UNQUOTED([SYSTEMD_ASK_PASSWORD_PATH], ["$SYSTEMD_ASK_PASSWORD"], 
[Path to systemd-ask-password tool])
 
+# Set -std=c99 unless user already specified a -std=
+case "${CFLAGS}" in
+  *-std=*) ;;
+  *)   CFLAGS="${CFLAGS} -std=c99" ;;
+esac
+
 #
 # Libtool
 #
@@ -547,6 +553,28 @@ AC_CHECK_DECLS(
,
[[${SOCKET_INCLUDES}]]
 )
+AC_CHECKING([anonymous union support])
+AC_COMPILE_IFELSE(
+   [AC_LANG_PROGRAM(
+   [[
+   struct mystruct {
+ union {
+   int m1;
+   char m2;
+ };
+   };
+   ]],
+   [[
+   struct mystruct s;
+   s.m1 = 1; s.m2 = 2;
+   ]]
+   )],
+   [
+   AC_MSG_RESULT([yes])
+   AC_DEFINE([HAVE_ANONYMOUS_UNION_SUPPORT], [], [Compiler 
supports anonymous unions])
+   ],
+   [AC_MSG_RESULT([no])]
+)
 
 dnl We emulate signals in Windows
 AC_CHECK_DECLS(
@@ -1142,12 +1170,6 @@ if test "${enable_pkcs11}" = "yes"; then
)
 fi
 
-# Set -std=c99 unless user already specified a -std=
-case "${CFLAGS}" in
-  *-std=*) ;;
-  *)   CFLAGS="${CFLAGS} -std=c99" ;;
-esac
-
 if test "${enable_pedantic}" = "yes"; then
enable_strict="yes"
CFLAGS="${CFLAGS} -pedantic"
diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h
index 5fe17e7..8f7a064 100644
--- a/src/openvpn/mroute.h
+++ b/src/openvpn/mroute.h
@@ -96,7 +96,17 @@ struct mroute_addr {
   uint8_t prefix[12];
   in_addr_t addr;  /* _network order_ IPv4 address */
 } v4mappedv6;
-  };
+  }
+#ifndef HAVE_ANONYMOUS_UNION_SUPPORT
+/* Wrappers to support compilers that do not grok anonymous unions */
+  mroute_union
+#define raw_addr mroute_union.raw_addr
+#define eth_addr mroute_union.eth_addr
+#define v4 mroute_union.v4
+#define v6 mroute_union.v6
+#define v4mappedv6 mroute_union.v4mappedv6
+#endif
+  ;
 };
 
 /* Double-check that struct packing works as expected */
-- 
2.7.4


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH applied] Re: Support --block-outside-dns on multiple tunnels

2016-11-13 Thread Gert Doering 
ACK.

I won't claim I have deep insights in the WFP stuff (truly not), but 
the code looks reasonable otherwise - much of it is just re-shuffling
and changing to a well-known GUID -, the explanation makes sense, and
it works for me :-) - tested on Win7, with one or two active tunnels both
using block-outside-dns - in all cases, "DNS through either tunnel" 
worked, and "DNS via LAN" did not.

In addition to that, we have the positive test report from "supergregg"
in trac #718.

Your patch has been applied to the master branch.

commit fc30dc5f20d455242ed8489fb1a99446287ba9cb
Author: Selva Nair
Date:   Sat Sep 17 00:10:38 2016 -0400

 Support --block-outside-dns on multiple tunnels

 Signed-off-by: Selva Nair 
 Acked-by: Gert Doering 
 Message-Id: <1474085439-28766-1-git-send-email-selva.n...@gmail.com>
 URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg12465.html
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 2/5] Add control channel encryption (--tls-crypt)

2016-11-13 Thread Arne Schwabe 

> This boils down to the following on-the-wire packet format:
> 
>-opcode- || -session_id- || -packet_id- || auth_tag || * payload *

I am pretty that opcode is *not* authenticated looking the code. Which
is probably not a problem but should not be documented as authenticated.
(There is a buf_advance(buf ,1) before calling the unwrap function.

>Where
>- XXX - means authenticated, and
>* XXX * means authenticated and encrypted.

This misalignes the rest of the packet. I am not sure how big the
penality actually is but we have DATA_V2 and COMP_V2 fix this
misalignment. But then again ignore my comment for control channel traffic.

For the packet format I think it would be very good to add a key id to
help transisition or allow more paranoid people to have a a key per
client. In the server you would specify all allowed valid keys together
with their ids (or somehow emded that id/read that id from keyfile). Not
that saying that this should be implement in the first version of the
patch but something that we can implement later without having a
tls-crypt and a tls-crypt v2 packet format.
> 
> Which is very similar to the current tls-auth packet format, and has the
> same overhead as "--tls-auth" with "--auth SHA256".
> 
> The use of a nonce misuse-resistant authenticated encryption scheme
> allows us to worry less about the risks of nonce collisions.  This is
> important, because in contrast with the data channel in TLS mode, we
> will not be able to rotate tls-crypt keys often or fully guarantee nonce
> uniqueness.  For non misuse-resistant modes such as GCM [1], [2], the
> data channel in TLS mode only has to ensure that the packet counter
> never rolls over, while tls-crypt would have to provide nonce uniqueness
> over all control channel packets sent by all clients, for the lifetime
> of the tls-crypt key.
> 
> Unlike with tls-auth, no --key-direction has to be specified for
> tls-crypt.  TLS servers always use key direction 1, and TLS clients
> always use key direction 2, which means that client->server traffic and
> server->client traffic always use different keys, without requiring
> configuration.
> 
> Using fixed, secure, encryption and authentication algorithms makes both
> implementation and configuration easier.  If we ever want to, we can
> extend this to support other crypto primitives.  Since tls-crypt should
> provide privacy as well as DoS protection, these should not be made
> negotiable.
> 
> Security considerations:
> 
> 
> tls-crypt is a best-effort mechanism that aims to provide as much
> privacy and security as possible, while staying as simple as possible.
> The following are some security considerations for this scheme.
> 
> 1. The same tls-crypt key is potentially shared by a lot of peers, so it
>is quite likely to get compromised.  Once an attacker acquires the
>tls-crypt key, this mechanism no longer provides any security against
>the attacker.
> 
> 2. Since many peers potentially use the tls-crypt key for a long time, a
>lot of data might be encrypted under the tls-crypt key.  This leads
>to two potential problems:
> 
>* The "opcode || session id || packet id" combination might collide.
>  This might happen in larger setups, because the session id contains
>  just 64 bits or random.  Using the uniqueness requirement from the
Typo: of random

>  GCM spec [3] (a collision probability of less than 2^(-32)),
>  uniqueness is achieved when using the tls-crypt key for at most
>  2^16 (65536) connections per process start.  (The packet id
>  includes the daemon start time in the packet ID, which should be
>  different after stopping and (re)starting OpenPVN.)
> 
>  And if a collision happens, an attacker can *only* learn whether
>  colliding packets contain the same plaintext.  Attackers will not
>  be able to learn anything else about the plaintext (unless the
>  attacker knows the plaintext of one of these packets, of course).
>  Since the impact is limited, I consider this an acceptable
>  remaining risk.
> 
>* The IVs used in encryption might collide.  When two IVs collide, an
>  attacker can learn the xor of the two plaintexts by xorring the
>  ciphertexts.  This is a serious loss of confidentiality.  The IVs
>  are 128-bit, so when HMAC-SHA256 is a secure PRF (an assumption
>  that must also hold for TLS), and we use the same uniqueness
>  requirement from [3], this limits the total amount of control
>  channel messages for all peers in the setup to 2^48.  Assuming a
>  large setup of 2^16 (65536) clients, and a (conservative) number of
>  2^16 control channel packets per connection on average, this means
>  that clients may set up 2^16 connections on average.  I think these
>  numbers are reasonable.
> 
> (I have a follow-up proposal to use client-specific tls-auth/tls-crypt
> keys to partially mitigate these issues, but

Re: [Openvpn-devel] [PATCH 3/5] Add missing includes in error.h

2016-11-13 Thread Arne Schwabe 
Am 08.11.16 um 15:18 schrieb Steffan Karger:
> error.h depends on these, but is apparently never used by files that do
> not include them.  When implementing the --tls-crypt unit tests, I ran
> into this.
> 

ACK



--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 1/5] Refactor static/tls-auth key loading

2016-11-13 Thread Arne Schwabe 
Am 08.11.16 um 21:18 schrieb Steffan Karger:
> Remove duplicate code, in preparation for adding --tls-crypt, which
> otherwise would have to duplicate this code again.
> 
> This should be equivalent to the old code, except for two things:
> * The log lines for static key initialization change slightly, from
>   "Static Encrypt/Decrypt" to "Incoming/Outgoing Static Key Encryption"
> * We also 'check and fix highly unlikely key problems' for tls-auth
>   keys (boils down to a sanity-check for an all-zero key).

Looks good. ACK to this one.

Arne


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH applied] Re: Add in_port_t check to configure.ac

2016-11-13 Thread Gert Doering 
Patch has been applied to the master branch.

commit dd6714ae0a47a9401898ccc0dd7079f58ba29901
Author: Gert Doering
Date:   Sun Nov 13 16:55:35 2016 +0100

 Add in_port_t check to configure.ac

 Signed-off-by: Gert Doering 
 Acked-by: Steffan Karger 
 Message-Id: <20161113155535.68355-1-g...@greenie.muc.de>
 URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13021.html
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Add in_port_t check to configure.ac

2016-11-13 Thread Steffan Karger 


On 13-11-16 16:55, Gert Doering wrote:
> commit 8cac9b98d58b97 introduced using in_port_t which is not
> available on (all?) mingw build environments.
> 
> Add configure check, falling back to uint16_t.
> 
> Signed-off-by: Gert Doering 
> ---
>  configure.ac | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 357ba29..e44f619 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -493,6 +493,12 @@ AC_CHECK_TYPES(
>   [AC_DEFINE([in_addr_t], [uint32_t], [Workaround missing in_addr_t])],
>   [[${SOCKET_INCLUDES}]]
>  )
> +AC_CHECK_TYPES(
> + [in_port_t],
> + ,
> + [AC_DEFINE([in_port_t], [uint16_t], [Workaround missing in_port_t])],
> + [[${SOCKET_INCLUDES}]]
> +)
>  AC_CHECK_TYPE(
>   [struct iphdr],
>   [AC_DEFINE([HAVE_IPHDR], [1], [struct iphdr needed for IPv6 support])],
> 

ACK

-Steffan

--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Add in_port_t check to configure.ac

2016-11-13 Thread Gert Doering 
commit 8cac9b98d58b97 introduced using in_port_t which is not
available on (all?) mingw build environments.

Add configure check, falling back to uint16_t.

Signed-off-by: Gert Doering 
---
 configure.ac | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/configure.ac b/configure.ac
index 357ba29..e44f619 100644
--- a/configure.ac
+++ b/configure.ac
@@ -493,6 +493,12 @@ AC_CHECK_TYPES(
[AC_DEFINE([in_addr_t], [uint32_t], [Workaround missing in_addr_t])],
[[${SOCKET_INCLUDES}]]
 )
+AC_CHECK_TYPES(
+   [in_port_t],
+   ,
+   [AC_DEFINE([in_port_t], [uint16_t], [Workaround missing in_port_t])],
+   [[${SOCKET_INCLUDES}]]
+)
 AC_CHECK_TYPE(
[struct iphdr],
[AC_DEFINE([HAVE_IPHDR], [1], [struct iphdr needed for IPv6 support])],
-- 
1.9.1


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH applied] Re: Don't deference type-punned pointers

2016-11-13 Thread Gert Doering 
Hi,

On Sun, Nov 13, 2016 at 02:50:43PM +0100, Gert Doering wrote:
> ACK.  Brilliant approach to "make the compiler happy" that is *increasing*
> code readability, not making it worse.
> 
> Reviewed ("STARE AT CODE!"), client tests on linux and freebsd, server
> tests on linux.
> 
> Your patch has been applied to the master branch.

This had interesting fallout.  I expected some linux/BSD issues (due
to generally different socket things), but instead the buildbots had
massive issues on all "older" systems - namely, NetBSD 5.1, OpenBSD 4.9,
CentOS 6, FreeBSD 9.3, FreeBSD 7.4.

The core of the issue seems to be this one:

gcc -DHAVE_CONFIG_H -I. -I../../../openvpn/src/openvpn -I../.. -I../../include  
-I../../../openvpn/include  -I../../../openvpn/src/compat   -g -O2 
-std=c99 -MT error.o -MD -MP -MF .deps/error.Tpo -c -o error.o 
../../../openvpn/src/openvpn/error.c
In file included from ../../../openvpn/src/openvpn/manage.h:33,
 from ../../../openvpn/src/openvpn/options.h:40,
 from ../../../openvpn/src/openvpn/ssl.h:43,
 from ../../../openvpn/src/openvpn/ps.h:32,
 from ../../../openvpn/src/openvpn/error.c:43:
../../../openvpn/src/openvpn/mroute.h:99: warning: declaration does not declare 
anything
../../../openvpn/src/openvpn/mroute.h:103: error: 'struct mroute_addr' has no 
member named 'v4'
../../../openvpn/src/openvpn/mroute.h:103: error: 'struct mroute_addr' has no 
member named 'v4'
../../../openvpn/src/openvpn/mroute.h:103: error: bit-field 
'__error_if_negative' width not an integer constant
../../../openvpn/src/openvpn/mroute.h:106: error: 'struct mroute_addr' has no 
member named 'v6'
../../../openvpn/src/openvpn/mroute.h:106: error: 'struct mroute_addr' has no 
member named 'v6'


which translates to

 - the new union most not by anonymous ("does not declare anything")
 - if I name it, like "a", the warning in line 99 goes away
 - but then, all references need to be added that name
 "addr->a.v4.addr"
   etc.


While I could live with abandoning FreeBSD 7.4, we need to keep supporting
at least FreeBSD 9.3 - so, any ideas how to deal with that?


MinGW builds fail because there is no "in_port_t" declaration, which is
something configure should be able to fix for us.

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
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 4/5 v2] Move private file access checks to options_postprocess_filechecks()

2016-11-13 Thread Steffan Karger 
From: Steffan Karger 

This removes the dependency of crypto.c on misc.c, which makes testing
(stuff that needs) crypto.c functionality easier.  In particular, this
simplifies the --tls-crypt tests in one of the follow-up patches.

Apart from that, testing file access really belongs in
options_postprocess_filechecks(), and moving it there enables us to
perform the same check for other private files too.

v2: change indenting, remove remaining warn_if_group_others_accessible()
calls and move function to options.c.

Signed-off-by: Steffan Karger 
---
 src/openvpn/crypto.c  |  5 +---
 src/openvpn/misc.c| 27 -
 src/openvpn/misc.h|  3 ---
 src/openvpn/options.c | 61 ---
 src/openvpn/ssl_mbedtls.c |  2 --
 src/openvpn/ssl_openssl.c |  2 --
 6 files changed, 48 insertions(+), 52 deletions(-)

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index ab43005..05622ce 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -36,7 +36,7 @@
 #include "crypto.h"
 #include "error.h"
 #include "integer.h"
-#include "misc.h"
+#include "platform.h"
 
 #include "memdbg.h"
 
@@ -1307,9 +1307,6 @@ read_key_file (struct key2 *key2, const char *file, const 
unsigned int flags)
   if (!(flags & RKF_INLINE))
 buf_clear ();
 
-  if (key2->n)
-warn_if_group_others_accessible (error_filename);
-
 #if 0
   /* DEBUGGING */
   {
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 3d40f0b..ea0d75d 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -194,31 +194,6 @@ save_inetd_socket_descriptor (void)
 }
 
 /*
- * Warn if a given file is group/others accessible.
- */
-void
-warn_if_group_others_accessible (const char* filename)
-{
-#ifndef WIN32
-#ifdef HAVE_STAT
-  if (strcmp (filename, INLINE_FILE_TAG))
-{
-  struct stat st;
-  if (stat (filename, ))
-   {
- msg (M_WARN | M_ERRNO, "WARNING: cannot stat file '%s'", filename);
-   }
-  else
-   {
- if (st.st_mode & (S_IRWXG|S_IRWXO))
-   msg (M_WARN, "WARNING: file '%s' is group or others accessible", 
filename);
-   }
-}
-#endif
-#endif
-}
-
-/*
  * Print an error message based on the status code returned by system().
  */
 const char *
@@ -1115,8 +1090,6 @@ get_user_pass_cr (struct user_pass *up,
   FILE *fp;
   char password_buf[USER_PASS_LEN] = { '\0' };
 
-  warn_if_group_others_accessible (auth_file);
-
   fp = platform_fopen (auth_file, "r");
   if (!fp)
 msg (M_ERR, "Error opening '%s' auth file: %s", prefix, auth_file);
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index ceda323..cc0a474 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -71,9 +71,6 @@ void run_up_down (const char *command,
 
 void write_pid (const char *filename);
 
-/* check file protections */
-void warn_if_group_others_accessible(const char* filename);
-
 /* system flags */
 #define S_SCRIPT (1<<0)
 #define S_FATAL  (1<<1)
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index e9dc17e..17732ce 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2689,11 +2689,37 @@ options_postprocess_mutate (struct options *o)
  */
 #ifndef ENABLE_SMALL  /** Expect people using the stripped down version to 
know what they do */
 
+/*
+ * Warn if a given file is group/others accessible.
+ */
+static void
+warn_if_group_others_accessible (const char* filename)
+{
+#ifndef WIN32
+#ifdef HAVE_STAT
+  if (strcmp (filename, INLINE_FILE_TAG))
+{
+  struct stat st;
+  if (stat (filename, ))
+   {
+ msg (M_WARN | M_ERRNO, "WARNING: cannot stat file '%s'", filename);
+   }
+  else
+   {
+ if (st.st_mode & (S_IRWXG|S_IRWXO))
+   msg (M_WARN, "WARNING: file '%s' is group or others accessible", 
filename);
+   }
+}
+#endif
+#endif
+}
+
 #define CHKACC_FILE (1<<0)   /** Check for a file/directory precense */
 #define CHKACC_DIRPATH (1<<1)/** Check for directory precense where a file 
should reside */
 #define CHKACC_FILEXSTWR (1<<2)  /** If file exists, is it writable? */
 #define CHKACC_INLINE (1<<3) /** File is present if it's an inline file */
 #define CHKACC_ACPTSTDIN (1<<4)  /** If filename is stdin, it's allowed and 
"exists" */
+#define CHKACC_PRIVATE (1<<5)   /** Warn if this (private) file is 
group/others accessible */
 
 static bool
 check_file_access(const int type, const char *file, const int mode, const char 
*opt)
@@ -2734,6 +2760,11 @@ check_file_access(const int type, const char *file, 
const int mode, const char *
 if (platform_access (file, W_OK) != 0)
   errcode = errno;
 
+  if (type & CHKACC_PRIVATE)
+{
+  warn_if_group_others_accessible (file);
+}
+
   /* Scream if an error is found */
   if( errcode > 0 )
 msg (M_NOPREFIX|M_OPTERR, "%s fails with '%s': %s",
@@ -2850,10 +2881,12 @@

[Openvpn-devel] [PATCH applied] Re: Don't deference type-punned pointers

2016-11-13 Thread Gert Doering 
ACK.  Brilliant approach to "make the compiler happy" that is *increasing*
code readability, not making it worse.

Reviewed ("STARE AT CODE!"), client tests on linux and freebsd, server
tests on linux.

Your patch has been applied to the master branch.

commit 8cac9b98d58b97fbd5a23dd9f172a9843ecf5b50
Author: Steffan Karger
Date:   Sun Nov 13 14:17:27 2016 +0100

 Don't deference type-punned pointers

 Signed-off-by: Steffan Karger 
 Acked-by: Gert Doering 
 Message-Id: <1479043047-25883-1-git-send-email-stef...@karger.me>
 URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13017.html
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering


--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH v3] Don't deference type-punned pointers

2016-11-13 Thread Steffan Karger 
Dereferencing type-punned pointers is undefined behaviour according to the
C standard.  We should either obey the standard, or ensure that all
supported compilers deal with dereferencing type-punned pointers as we
want them to.  I think just obeying the standard is the easiest solution.

See e.g. http://blog.regehr.org/archives/959.

This commit refactors the offending code to use unions or memcpy() to
comply to strict aliasing rules.

Note that this also slightly changes mroute_addr_mask_host_bits(), to
behave as it was probably intended to:  only mask the address part, not
also the port part of IPv6 adresses if MR_WITH_PORT is used (ie ma->len
is sizeof(struct in6_addr)+2).

v2: fix all strict aliasing occurrences, not just those in mroute.h
v3: add missing ntohs() in mroute_addr_print_ex()

Signed-off-by: Steffan Karger 
---
 src/openvpn/error.h  | 10 +++
 src/openvpn/mroute.c | 85 +---
 src/openvpn/mroute.h | 41 +
 src/openvpn/multi.c  |  4 +--
 src/openvpn/ps.c | 10 ---
 src/openvpn/socket.c |  3 +-
 6 files changed, 96 insertions(+), 57 deletions(-)

diff --git a/src/openvpn/error.h b/src/openvpn/error.h
index ced2fdf..c90b36b 100644
--- a/src/openvpn/error.h
+++ b/src/openvpn/error.h
@@ -27,6 +27,8 @@
 
 #include "basic.h"
 
+#include 
+
 /* #define ABORT_ON_ERROR */
 
 #ifdef ENABLE_PKCS11
@@ -219,6 +221,14 @@ FILE *msg_fp(const unsigned int flags);
 void assert_failed (const char *filename, int line, const char *condition)
   __attribute__((__noreturn__));
 
+/* Poor-man's static_assert() for when not supplied by assert.h, taken from
+ * Linux's sys/cdefs.h under GPLv2 */
+#ifndef static_assert
+#define static_assert(expr, diagnostic) \
+extern int (*__OpenVPN_static_assert_function (void)) \
+  [!!sizeof (struct { int __error_if_negative: (expr) ? 2 : -1; })]
+#endif
+
 #ifdef ENABLE_DEBUG
 void crash (void); /* force a segfault (debugging only) */
 #endif
diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
index 972f1dd..c905af7 100644
--- a/src/openvpn/mroute.c
+++ b/src/openvpn/mroute.c
@@ -58,7 +58,8 @@ is_mac_mcast_addr (const uint8_t *mac)
 static inline bool
 is_mac_mcast_maddr (const struct mroute_addr *addr)
 {
-  return (addr->type & MR_ADDR_MASK) == MR_ADDR_ETHER && is_mac_mcast_addr 
(addr->addr); 
+  return (addr->type & MR_ADDR_MASK) == MR_ADDR_ETHER &&
+  is_mac_mcast_addr (addr->eth_addr);
 }
 
 /*
@@ -73,7 +74,7 @@ mroute_learnable_address (const struct mroute_addr *addr)
 
   for (i = 0; i < addr->len; ++i)
 {
-  int b = addr->addr[i];
+  int b = addr->raw_addr[i];
   if (b != 0x00)
not_all_zeros = true;
   if (b != 0xFF)
@@ -90,7 +91,7 @@ mroute_get_in_addr_t (struct mroute_addr *ma, const in_addr_t 
src, unsigned int
   ma->type = MR_ADDR_IPV4 | mask;
   ma->netbits = 0;
   ma->len = 4;
-  *(in_addr_t*)ma->addr = src;
+  ma->v4.addr = src;
 }
 }
 
@@ -102,7 +103,7 @@ mroute_get_in6_addr (struct mroute_addr *ma, const struct 
in6_addr src, unsigned
   ma->type = MR_ADDR_IPV6 | mask;
   ma->netbits = 0;
   ma->len = 16;
-  *(struct in6_addr *)ma->addr = src;
+  ma->v6.addr = src;
 }
 }
 
@@ -226,14 +227,14 @@ mroute_extract_addr_ether (struct mroute_addr *src,
  src->type = MR_ADDR_ETHER;
  src->netbits = 0;
  src->len = 6;
- memcpy (src->addr, eth->source, 6);
+ memcpy (src->eth_addr, eth->source, sizeof(dest->eth_addr));
}
   if (dest)
{
  dest->type = MR_ADDR_ETHER;
  dest->netbits = 0;
  dest->len = 6;
- memcpy (dest->addr, eth->dest, 6);
+ memcpy (dest->eth_addr, eth->dest, sizeof(dest->eth_addr));
 
  /* ethernet broadcast/multicast packet? */
  if (is_mac_mcast_addr (eth->dest))
@@ -281,15 +282,15 @@ bool mroute_extract_openvpn_sockaddr (struct mroute_addr 
*addr,
  addr->type = MR_ADDR_IPV4 | MR_WITH_PORT;
  addr->netbits = 0;
  addr->len = 6;
- memcpy (addr->addr, >addr.in4.sin_addr.s_addr, 4);
- memcpy (addr->addr + 4, >addr.in4.sin_port, 2);
+ addr->v4.addr = osaddr->addr.in4.sin_addr.s_addr;
+ addr->v4.port = osaddr->addr.in4.sin_port;
}
   else
{
  addr->type = MR_ADDR_IPV4;
  addr->netbits = 0;
  addr->len = 4;
- memcpy (addr->addr, >addr.in4.sin_addr.s_addr, 4);
+ addr->v4.addr = osaddr->addr.in4.sin_addr.s_addr;
}
   return true;
 }
@@ -299,15 +300,15 @@ bool mroute_extract_openvpn_sockaddr (struct mroute_addr 
*addr,
  addr->type = MR_ADDR_IPV6 | MR_WITH_PORT;
  addr->netbits = 0;
  addr->len = 18;
- memcpy (addr->addr, >addr.in6.sin6_addr, 16);
- memcpy (addr->addr + 16, >addr.in6.sin6_port, 2);
+ addr->v6.addr = osaddr->addr.in6.sin6_addr;
+ addr->v6.port =

[Openvpn-devel] [PATCH] Don't deference type-punned pointers

2016-11-13 Thread Steffan Karger 
Dereferencing type-punned pointers is undefined behaviour according to the
C standard.  We should either obey the standard, or ensure that all
supported compilers deal with dereferencing type-punned pointers as we
want them to.  I think just obeying the standard is the easiest solution.

See e.g. http://blog.regehr.org/archives/959.

This commit refactors the offending code to use unions or memcpy() to
comply to strict aliasing rules.

Note that this also slightly changes mroute_addr_mask_host_bits(), to
behave as it was probably intended to:  only mask the address part, not
also the port part of IPv6 adresses if MR_WITH_PORT is used (ie ma->len
is sizeof(struct in6_addr)+2).

v2: fix all strict aliasing occurrences, not just those in mroute.h.

Signed-off-by: Steffan Karger 
---
 src/openvpn/error.h  | 10 +++
 src/openvpn/mroute.c | 85 +---
 src/openvpn/mroute.h | 41 +
 src/openvpn/multi.c  |  4 +--
 src/openvpn/ps.c | 10 ---
 src/openvpn/socket.c |  3 +-
 6 files changed, 96 insertions(+), 57 deletions(-)

diff --git a/src/openvpn/error.h b/src/openvpn/error.h
index ced2fdf..c90b36b 100644
--- a/src/openvpn/error.h
+++ b/src/openvpn/error.h
@@ -27,6 +27,8 @@
 
 #include "basic.h"
 
+#include 
+
 /* #define ABORT_ON_ERROR */
 
 #ifdef ENABLE_PKCS11
@@ -219,6 +221,14 @@ FILE *msg_fp(const unsigned int flags);
 void assert_failed (const char *filename, int line, const char *condition)
   __attribute__((__noreturn__));
 
+/* Poor-man's static_assert() for when not supplied by assert.h, taken from
+ * Linux's sys/cdefs.h under GPLv2 */
+#ifndef static_assert
+#define static_assert(expr, diagnostic) \
+extern int (*__OpenVPN_static_assert_function (void)) \
+  [!!sizeof (struct { int __error_if_negative: (expr) ? 2 : -1; })]
+#endif
+
 #ifdef ENABLE_DEBUG
 void crash (void); /* force a segfault (debugging only) */
 #endif
diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
index 972f1dd..ad42f76 100644
--- a/src/openvpn/mroute.c
+++ b/src/openvpn/mroute.c
@@ -58,7 +58,8 @@ is_mac_mcast_addr (const uint8_t *mac)
 static inline bool
 is_mac_mcast_maddr (const struct mroute_addr *addr)
 {
-  return (addr->type & MR_ADDR_MASK) == MR_ADDR_ETHER && is_mac_mcast_addr 
(addr->addr); 
+  return (addr->type & MR_ADDR_MASK) == MR_ADDR_ETHER &&
+  is_mac_mcast_addr (addr->eth_addr);
 }
 
 /*
@@ -73,7 +74,7 @@ mroute_learnable_address (const struct mroute_addr *addr)
 
   for (i = 0; i < addr->len; ++i)
 {
-  int b = addr->addr[i];
+  int b = addr->raw_addr[i];
   if (b != 0x00)
not_all_zeros = true;
   if (b != 0xFF)
@@ -90,7 +91,7 @@ mroute_get_in_addr_t (struct mroute_addr *ma, const in_addr_t 
src, unsigned int
   ma->type = MR_ADDR_IPV4 | mask;
   ma->netbits = 0;
   ma->len = 4;
-  *(in_addr_t*)ma->addr = src;
+  ma->v4.addr = src;
 }
 }
 
@@ -102,7 +103,7 @@ mroute_get_in6_addr (struct mroute_addr *ma, const struct 
in6_addr src, unsigned
   ma->type = MR_ADDR_IPV6 | mask;
   ma->netbits = 0;
   ma->len = 16;
-  *(struct in6_addr *)ma->addr = src;
+  ma->v6.addr = src;
 }
 }
 
@@ -226,14 +227,14 @@ mroute_extract_addr_ether (struct mroute_addr *src,
  src->type = MR_ADDR_ETHER;
  src->netbits = 0;
  src->len = 6;
- memcpy (src->addr, eth->source, 6);
+ memcpy (src->eth_addr, eth->source, sizeof(dest->eth_addr));
}
   if (dest)
{
  dest->type = MR_ADDR_ETHER;
  dest->netbits = 0;
  dest->len = 6;
- memcpy (dest->addr, eth->dest, 6);
+ memcpy (dest->eth_addr, eth->dest, sizeof(dest->eth_addr));
 
  /* ethernet broadcast/multicast packet? */
  if (is_mac_mcast_addr (eth->dest))
@@ -281,15 +282,15 @@ bool mroute_extract_openvpn_sockaddr (struct mroute_addr 
*addr,
  addr->type = MR_ADDR_IPV4 | MR_WITH_PORT;
  addr->netbits = 0;
  addr->len = 6;
- memcpy (addr->addr, >addr.in4.sin_addr.s_addr, 4);
- memcpy (addr->addr + 4, >addr.in4.sin_port, 2);
+ addr->v4.addr = osaddr->addr.in4.sin_addr.s_addr;
+ addr->v4.port = osaddr->addr.in4.sin_port;
}
   else
{
  addr->type = MR_ADDR_IPV4;
  addr->netbits = 0;
  addr->len = 4;
- memcpy (addr->addr, >addr.in4.sin_addr.s_addr, 4);
+ addr->v4.addr = osaddr->addr.in4.sin_addr.s_addr;
}
   return true;
 }
@@ -299,15 +300,15 @@ bool mroute_extract_openvpn_sockaddr (struct mroute_addr 
*addr,
  addr->type = MR_ADDR_IPV6 | MR_WITH_PORT;
  addr->netbits = 0;
  addr->len = 18;
- memcpy (addr->addr, >addr.in6.sin6_addr, 16);
- memcpy (addr->addr + 16, >addr.in6.sin6_port, 2);
+ addr->v6.addr = osaddr->addr.in6.sin6_addr;
+ addr->v6.port = osaddr->addr.in6.sin6_port;
}
   else
{