[Openvpn-devel] [PATCH applied] Re: Fix regression with password protected private keys (polarssl)

2014-10-24 Thread Gert Doering 
ACK, verifying against the polarssl commit.

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

commit f056c8eadc4d5fcda5d1e861425802f503587f16
Author: Steffan Karger
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Fri Sep 19 06:43:48 2014 +0200

 Fix regression with password protected private keys (polarssl)

 Signed-off-by: Steffan Karger 
 Acked-by: Gert Doering 
 Message-Id: <5432e951.6020...@fox-it.com>
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Fix regression with password protected private keys (polarssl)

2014-10-24 Thread Gert Doering 
ACK, verifying against the polarssl commit.

Your patch has been applied to the master branch.

commit 4b9eaa1ee40648f101deb4ebf07a04cd5b5400e9
Author: Steffan Karger
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Fri Sep 19 06:19:13 2014 +0200

 Fix regression with password protected private keys (polarssl)

 Signed-off-by: Steffan Karger 
 Acked-by: Gert Doering 
 Message-Id: <5432e951.6020...@fox-it.com>
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Modification to address bug where OpenVPN enters state where it is unresponsive and cannot be terminated. Log output is continuous spew of code=995 errors.

2014-10-24 Thread Gert Doering 
Your patch has been applied to the master and release/2.3 branches.

Since the second patch basically undoes everything the first one does
and uses a different approach, I've merged them into one patch to make
the net change more explicit - and fixed a bit of spurious whitespace.

Also, I've taken the liberty of adding a non-Win32 version of 
tuntap_abort() to tun.h, to avoid breaking compilation elsewhere.

But anyway, thanks for telling us where the problem with our talking to 
the driver was :-)


commit 7aa178381241ae015273914065471e0d271ee1c3 (master)
commit 4fe957aabb9289b13ce9c28854d1e912efee9b77 (release/2.3)

Author: TDivine
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Wed Oct 22 10:07:39 2014 +0300

Fix "code=995" bug with windows NDIS6 tap driver.

Modification to address bug where OpenVPN enters state where it is
unresponsive and cannot be terminated. Log output is continuous spew of 
"code=995" errors.

Revised fix for code=995 sped bug.

Adding new tap adapters while connected:
  https://community.openvpn.net/openvpn/ticket/430

Acked-by: Gert Doering 
Message-Id: <1413961660-19251-2-git-send-email-sam...@openvpn.net>
URL: http://article.gmane.org/gmane.network.openvpn.devel/9165
Signed-off-by: Gert Doering 

Acked-by: Gert Doering 
Message-Id: <1413961660-19251-3-git-send-email-sam...@openvpn.net>
URL: http://article.gmane.org/gmane.network.openvpn.devel/9167
Signed-off-by: Gert Doering 



--
kind regards,

Gert Doering

Re: [Openvpn-devel] session-id implementation

2014-10-24 Thread David Sommerseth 
On 24/10/14 15:20, Gert Doering wrote:
[...snip...]
> ... please don't do whitespace changes in places where no code changes
> (as it makes it harder to see where changes happened)
[...snip...]
> Here's an escaped tab-to-space conversion or so, but "just whitespace 
> change" nonetheless.
[...snip...]
> Whitespace...

As there were a lot of "whitespace" issues here, I'd recommend you to
enable colours in git.  This will highlight many of the whitespace
issues here.  Just add this section to either your global git config
(~/.gitconfig) or directly your git tree config (.git/config):

[color]
branch = auto
diff = auto
log = auto
interactive = auto
status = auto
pager = true

Some might find it useful to also have this:

[core]
pager = less -X -R -F


When the patch is "complete", it would also help us maintainers if you
do 'git add' on the files you've changed (see git status) and then do a
commit (git commit -s) with a describing commit message (describe the
patch in a non-technical manner [1]).  Then you can use 'git
format-patch HEAD~1' to get the last commit as patch.  See our quick git
crash course [2] for more things you can do with git, like sending mails
directly from gi.

[1] 
[2] 


-- 
kind regards,

David Sommerseth



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] session-id implementation

2014-10-24 Thread Lev Stipakov 
Hello,

As discussed on IRC meeting, we replace session-id with peer-id.

So, waiting for review and code-ACK :)

-Lev

2014-10-23 17:07 GMT+03:00 Lev Stipakov :
> Hi Steffan,
>
> Patch attached.
>
> -Lev
>
> 2014-10-23 10:52 GMT+03:00 Steffan Karger :
>> Hi Lev,
>>
>> On 10/21/2014 09:33 AM, Lev Stipakov wrote:
>>>
>>> Thanks for your comments. I have fixed (1) and (2) - well, reusing
>>> existing code in (2) has fixed also (1).
>>
>> Thanks! Do you have the patch somewhere for us to look at?
>>
>>> Regarding (3) - I don't have much experience in crypto thing, so it
>>> would be nice if someone suggests if we should use another alignment.
>>
>> >From later discussions I get the feeling the alignment was not crypto
>> related.
>>
>> Also, alignment for crypto is very platform-specific. e.g. AES-NI
>> instructions require 128-bit alignment for maximum speed. Though it
>> depends on the crypto mode and implementation whether the data we supply
>> it is directly used or not.
>>
>> The main accelerated crypto mode currently is AES-GCM, and that does not
>> run the data through the crypto (but rather encrypts counters, and then
>> XORs the result with the data). So I wouldn't worry too much about that
>> for now.
>>
>> -Steffan
>>
>> --
>> ___
>> Openvpn-devel mailing list
>> Openvpn-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
>
>
> --
> -Lev



-- 
-Lev
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index c4c356d..c26e02d 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -220,6 +220,30 @@ err:
   return;
 }
 
+int verify_hmac(struct buffer *buf, struct key_ctx *ctx, int offset)
+{
+  uint8_t local_hmac[MAX_HMAC_KEY_LENGTH]; /* HMAC of ciphertext computed locally */
+  int hmac_len = 0;
+
+  hmac_ctx_reset(ctx->hmac);
+  /* Assume the length of the input HMAC */
+  hmac_len = hmac_ctx_size (ctx->hmac);
+
+  /* Authentication fails if insufficient data in packet for HMAC */
+  if (buf->len - offset < hmac_len)
+return 0;
+
+  hmac_ctx_update (ctx->hmac, BPTR (buf) + hmac_len + offset,
+  	BLEN (buf) - hmac_len - offset);
+  hmac_ctx_final (ctx->hmac, local_hmac);
+
+  /* Compare locally computed HMAC with packet HMAC */
+  if (memcmp_constant_time (local_hmac, BPTR (buf) + offset, hmac_len) == 0)
+return hmac_len;
+
+  return 0;
+}
+
 /*
  * If (opt->flags & CO_USE_IV) is not NULL, we will read an IV from the packet.
  *
@@ -246,30 +270,13 @@ openvpn_decrypt (struct buffer *buf, struct buffer work,
   /* Verify the HMAC */
   if (ctx->hmac)
 	{
-	  int hmac_len;
-	  uint8_t local_hmac[MAX_HMAC_KEY_LENGTH]; /* HMAC of ciphertext computed locally */
-
-	  hmac_ctx_reset(ctx->hmac);
-
-	  /* Assume the length of the input HMAC */
-	  hmac_len = hmac_ctx_size (ctx->hmac);
-
-	  /* Authentication fails if insufficient data in packet for HMAC */
-	  if (buf->len < hmac_len)
-	CRYPT_ERROR ("missing authentication info");
-
-	  hmac_ctx_update (ctx->hmac, BPTR (buf) + hmac_len, BLEN (buf) - hmac_len);
-	  hmac_ctx_final (ctx->hmac, local_hmac);
-
-	  /* Compare locally computed HMAC with packet HMAC */
-	  if (memcmp_constant_time (local_hmac, BPTR (buf), hmac_len))
+	  int hmac_len = verify_hmac(buf, ctx, 0);
+	  if (hmac_len == 0)
 	CRYPT_ERROR ("packet HMAC authentication failed");
-
 	  ASSERT (buf_advance (buf, hmac_len));
 	}
 
   /* Decrypt packet ID + payload */
-
   if (ctx->cipher)
 	{
 	  const unsigned int mode = cipher_ctx_mode (ctx->cipher);
@@ -389,6 +396,28 @@ openvpn_decrypt (struct buffer *buf, struct buffer work,
 }
 
 /*
+ * This verifies if a packet and its HMAC fit to a crypto context.
+ *
+ * On success true is returned.
+ */
+bool
+crypto_test_hmac (struct buffer *buf, const struct crypto_options *opt)
+{
+  if (buf->len > 0 && opt->key_ctx_bi)
+{
+  struct key_ctx *ctx = >key_ctx_bi->decrypt;
+
+  /* Verify the HMAC */
+  if (ctx->hmac)
+	{
+	  /* sizeof(uint32_t) comes from peer_id (3 bytes) and opcode (1 byte) */
+	  return verify_hmac(buf, ctx, sizeof(uint32_t)) != 0;
+	}
+}
+  return false;
+}
+
+/*
  * How many bytes will we add to frame buffer for a given
  * set of crypto options?
  */
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index 3b4b88e..68cdf16 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -279,6 +279,9 @@ bool openvpn_decrypt (struct buffer *buf, struct buffer work,
 		  const struct crypto_options *opt,
 		  const struct frame* frame);
 
+
+bool crypto_test_hmac (struct buffer *buf, const struct crypto_options *opt);
+
 /** @} name Functions for performing security operations on data channel packets */
 
 void crypto_adjust_frame_parameters(struct frame *frame,
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 3b72b96..a92d717 100644
---

Re: [Openvpn-devel] man page and options.c contradict

2014-10-24 Thread Steffan Karger 
On 10/24/2014 11:41 AM, Arne Schwabe wrote:
> Am 24.10.14 11:15, schrieb Gert Doering:
> And an important distinction is that the p2p mode does not use Diffie
> Hellman, meaning that it provides no Perferct Forward Security.

No, I think there's another distinction. There's 'static key mode'
(--secret), which does not do key negotiations. But it is also possible
to use --tls-server with --remote, resulting in a p2p network setup, but
using TLS (and thus dynamic key negotiation and PFS).

So much possibilities!

-Steffan

Re: [Openvpn-devel] man page and options.c contradict

2014-10-24 Thread Arne Schwabe 
Am 24.10.14 11:15, schrieb Gert Doering:
> Hi,
>
> On Fri, Oct 24, 2014 at 01:04:17AM -0600, Reinoud Koornstra wrote:
>> I understood the term mode wrong.
>> So the main mode can be p2p or server to denote the openvpn protocol inside
>> tcp or udp(stateless) session. Initially I thought the term mode is used to
>> denote server, client or udp.
> OpenVPN has the modes "peer2peer" or "(multi-)client to server".
>
> The "client" mode differs from "peer2peer" only insofar as it expects
> to receive confings from the server, while the "server" mode is intended
> to handle multiple clients, send them configs, and so on.
And an important distinction is that the p2p mode does not use Diffie
Hellman, meaning that it provides no Perferct Forward Security.


Arne

Re: [Openvpn-devel] man page and options.c contradict

2014-10-24 Thread Gert Doering 
Hi,

On Fri, Oct 24, 2014 at 01:04:17AM -0600, Reinoud Koornstra wrote:
> I understood the term mode wrong.
> So the main mode can be p2p or server to denote the openvpn protocol inside
> tcp or udp(stateless) session. Initially I thought the term mode is used to
> denote server, client or udp.

OpenVPN has the modes "peer2peer" or "(multi-)client to server".

The "client" mode differs from "peer2peer" only insofar as it expects
to receive confings from the server, while the "server" mode is intended
to handle multiple clients, send them configs, and so on.


If you run OpenVPN as "--server", it will also default to "--tcp-server"
if you only tell it "use TCP", because it knows that tcp-client is not
useful in OpenVPN server mode - but that's just a shortcut to ease the
config, and it can be a bit confusing, yes.

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


pgpketo4dD94H.pgp
Description: PGP signature

[Openvpn-devel] FW: [PATCH] Fix regression with password protected private keys (polarssl)

2014-10-24 Thread Steffan Karger 
Attempt 2, see below. It seems that somehow my previous mail has disappeared 
from the interwebs, I can't find it in the archives.

-Original Message-
From: Steffan Karger [mailto:steffan.kar...@fox-it.com] 
Sent: maandag 6 oktober 2014 21:11
To: openvpn-devel@lists.sourceforge.net
Subject: [PATCH] Fix regression with password protected private keys (polarssl)

Hi,

Between versions 1.2.7 and 1.2.8, polarssl changed the errors returned by the 
X509 parsing functions, which broke the OpenVPN implementation for password 
protected private keys in polarssl builds. Later, for polarssl 1.3, the return 
codes changed again.

The attached patches fix the regression by checking for the new errors in 
OpenVPN. Since the 2.3 and master code is slightly different here, I made a 
patch for each branch.

The polarssl change for 1.2.8:
https://github.com/polarssl/polarssl/commit/b495d3a

An later for polarssl 1.3 (search for pk_parse_key()):
https://github.com/polarssl/polarssl/commit/1a7550a

-Steffan
From 3c6c25b8c2270ad0af71a8837b60ea40ecfe66be Mon Sep 17 00:00:00 2001
From: Steffan Karger 
Date: Fri, 19 Sep 2014 06:43:48 +0200
Subject: [PATCH (2.3)] Fix regression with password protected private keys
 (polarssl)

Between versions 1.2.7 and 1.2.8, polarssl changed the errors
returned by the X509 parsing functions, which broke the OpenVPN
implementation for password protected private keys in polarssl
builds. This patch fixes that by checking for the new errors in
OpenVPN.

Signed-off-by: Steffan Karger 
---
 src/openvpn/ssl_polarssl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/openvpn/ssl_polarssl.c b/src/openvpn/ssl_polarssl.c
index e3e3017..aba405b 100644
--- a/src/openvpn/ssl_polarssl.c
+++ b/src/openvpn/ssl_polarssl.c
@@ -273,7 +273,7 @@ tls_ctx_load_priv_file (struct tls_root_ctx *ctx, const char *priv_key_file,
   status = x509parse_key(ctx->priv_key,
 	  priv_key_file_inline, strlen(priv_key_file_inline),
 	  NULL, 0);
-  if (POLARSSL_ERR_PEM_PASSWORD_REQUIRED == status)
+  if (POLARSSL_ERR_X509_PASSWORD_REQUIRED == status)
 	{
 	  char passbuf[512] = {0};
 	  pem_password_callback(passbuf, 512, 0, NULL);
@@ -285,7 +285,7 @@ tls_ctx_load_priv_file (struct tls_root_ctx *ctx, const char *priv_key_file,
   else
 {
   status = x509parse_keyfile(ctx->priv_key, priv_key_file, NULL);
-  if (POLARSSL_ERR_PEM_PASSWORD_REQUIRED == status)
+  if (POLARSSL_ERR_X509_PASSWORD_REQUIRED == status)
 	{
 	  char passbuf[512] = {0};
 	  pem_password_callback(passbuf, 512, 0, NULL);
@@ -295,7 +295,7 @@ tls_ctx_load_priv_file (struct tls_root_ctx *ctx, const char *priv_key_file,
   if (0 != status)
 {
 #ifdef ENABLE_MANAGEMENT
-  if (management && (POLARSSL_ERR_PEM_PASSWORD_MISMATCH == status))
+  if (management && (POLARSSL_ERR_X509_PASSWORD_MISMATCH == status))
 	  management_auth_failure (management, UP_TYPE_PRIVATE_KEY, NULL);
 #endif
   msg (M_WARN, "Cannot load private key file %s", priv_key_file);
-- 
1.9.1

From 5671a73039e94df29fbe07b8250284366ebcda7d Mon Sep 17 00:00:00 2001
From: Steffan Karger 
Date: Fri, 19 Sep 2014 06:19:13 +0200
Subject: [PATCH (master)] Fix regression with password protected private keys
 (polarssl)

Between versions 1.2 and 1.3, polarssl changed the errors
returned by the X509 parsing functions, which broke the OpenVPN
implementation for password protected private keys in polarssl
builds. This patch fixes that by checking for the new errors in
OpenVPN.

Signed-off-by: Steffan Karger 
---
 src/openvpn/ssl_polarssl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/openvpn/ssl_polarssl.c b/src/openvpn/ssl_polarssl.c
index 102a5a4..94ae4c3 100644
--- a/src/openvpn/ssl_polarssl.c
+++ b/src/openvpn/ssl_polarssl.c
@@ -298,7 +298,7 @@ tls_ctx_load_priv_file (struct tls_root_ctx *ctx, const char *priv_key_file,
 	  (const unsigned char *) priv_key_inline, strlen(priv_key_inline),
 	  NULL, 0);
 
-  if (POLARSSL_ERR_PEM_PASSWORD_REQUIRED == status)
+  if (POLARSSL_ERR_PK_PASSWORD_REQUIRED == status)
 	{
 	  char passbuf[512] = {0};
 	  pem_password_callback(passbuf, 512, 0, NULL);
@@ -310,7 +310,7 @@ tls_ctx_load_priv_file (struct tls_root_ctx *ctx, const char *priv_key_file,
   else
 {
   status = pk_parse_keyfile(ctx->priv_key, priv_key_file, NULL);
-  if (POLARSSL_ERR_PEM_PASSWORD_REQUIRED == status)
+  if (POLARSSL_ERR_PK_PASSWORD_REQUIRED == status)
 	{
 	  char passbuf[512] = {0};
 	  pem_password_callback(passbuf, 512, 0, NULL);
@@ -320,7 +320,7 @@ tls_ctx_load_priv_file (struct tls_root_ctx *ctx, const char *priv_key_file,
   if (0 != status)
 {
 #ifdef ENABLE_MANAGEMENT
-  if (management && (POLARSSL_ERR_PEM_PASSWORD_MISMATCH == status))
+  if (management && (POLARSSL_ERR_PK_PASSWORD_MISMATCH == status))
 	  management_auth_failure