Re: [Openvpn-devel] session-id implementation

[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

[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] session-id implementation

[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
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index c4c356d..2c827e2 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 session_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..fef7c1d 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1748,7 +1748,8 @@ pull_permission_mask (const struct context *c)
 | OPT_P_MESSAGES
 | OPT_P_EXPLICIT_NOTIFY
 | OPT_P_ECHO
-| OPT_P_PULL_MODE;
+| OPT_P_PULL_MODE
+| 

Re: [Openvpn-devel] session-id implementation

[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

Re: [Openvpn-devel] session-id implementation

[Lev Stipakov]

Hi Steffan,

Thanks for your comments. I have fixed (1) and (2) - well, reusing
existing code in (2) has fixed also (1).

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.

I'd be happy to join IRC meeting (nickname lev__) to discuss how we
should process with this feature.

-Lev

2014-10-09 22:49 GMT+03:00 Steffan Karger :
> Hi Lev,
>
> On 02-10-14 13:47, Lev Stipakov wrote:
>> Apologize for the delay. Patch with review suggestions attached.
>
> Thanks for providing the patch, and following up on comments on the
> list. I've been deferring a reply to your first version, because I
> wanted to take a thorough look at the code before replying, but somehow
> did not get to it. Sorry. So, instead of deferring further, I'll just
> mail what's in my head at this point.
>
> I think your approach is a good alternative to the original proposal. I
> like that it requires less bytes in the message. However, I think we
> should arrange maybe a community meeting to discuss your alternative and
> choose our way forward.
>
> For now, some things I noticed while taking a quick peek at your patch.
> Since I mostly focus on the crypto bits of openvpn, my remarks focus on
> the crypto part of your patch too. I haven't looked at the other parts.
>
> 1) Your patch introduced a memcmp() call to verify an HMAC. That would
> introduce a timing side channel we just got rid of in commit 11d2134.
> Please *always* use use constant time comparisons when checking HMACs.
> OpenVPN provides a memcmp_constant_time().
>
> 2) The crypto_test_hmac() function your patch introduces duplicates a
> lot of code from openvpn_decrypt(). I don't like to maintain more code
> than absolutely necessary ;) So, please add some functions to reuse the
> code. Also, I think the hard-coded offset in that function is a bit ugly.
>
> 3) The three-byte offset helps to align the data on 32-bit blocks, but I
> am trying to remember what the specific reason for this was. If it was
> to speed up the crypto, we should make sure its 128-bits aligned,
> because SSE/AES-NI instructions require 128-bits aligned data. (Though
> that would probably only be useful for CBC mode, since in CFB/OFB/GCM,
> the AES-operations are not actually performed the packet data buffer.)
>
> -Steffan
>
> --
> Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer
> Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports
> Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper
> Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer
> http://pubads.g.doubleclick.net/gampad/clk?id=154622311=/4140/ostg.clktrk
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel



-- 
-Lev

Re: [Openvpn-devel] session-id implementation

[Steffan Karger]

Hi Lev,

On 02-10-14 13:47, Lev Stipakov wrote:
> Apologize for the delay. Patch with review suggestions attached.

Thanks for providing the patch, and following up on comments on the
list. I've been deferring a reply to your first version, because I
wanted to take a thorough look at the code before replying, but somehow
did not get to it. Sorry. So, instead of deferring further, I'll just
mail what's in my head at this point.

I think your approach is a good alternative to the original proposal. I
like that it requires less bytes in the message. However, I think we
should arrange maybe a community meeting to discuss your alternative and
choose our way forward.

For now, some things I noticed while taking a quick peek at your patch.
Since I mostly focus on the crypto bits of openvpn, my remarks focus on
the crypto part of your patch too. I haven't looked at the other parts.

1) Your patch introduced a memcmp() call to verify an HMAC. That would
introduce a timing side channel we just got rid of in commit 11d2134.
Please *always* use use constant time comparisons when checking HMACs.
OpenVPN provides a memcmp_constant_time().

2) The crypto_test_hmac() function your patch introduces duplicates a
lot of code from openvpn_decrypt(). I don't like to maintain more code
than absolutely necessary ;) So, please add some functions to reuse the
code. Also, I think the hard-coded offset in that function is a bit ugly.

3) The three-byte offset helps to align the data on 32-bit blocks, but I
am trying to remember what the specific reason for this was. If it was
to speed up the crypto, we should make sure its 128-bits aligned,
because SSE/AES-NI instructions require 128-bits aligned data. (Though
that would probably only be useful for CBC mode, since in CFB/OFB/GCM,
the AES-operations are not actually performed the packet data buffer.)

-Steffan

Re: [Openvpn-devel] session-id implementation

[Lev Stipakov]

Hello Arne, Heikki and everyone,

Apologize for the delay. Patch with review suggestions attached.


-Lev.


2014-08-12 11:26 GMT+03:00 Heikki Hannikainen :
> On Wed, 9 Jul 2014, Arne Schwabe wrote:
>
>> Am 29.06.14 18:13, schrieb Arne Schwabe:
>>>
>>> Am 27.03.14 09:57, schrieb Lev Stipakov:

 Hi,

 Same patch with added NULL check in push.c:308. Turns out that
 peer_info might be NULL.

>>> I looked at the patched, a few minor nitpicks:
>
>
> One more little nitpick:
>
>   uint32_t sess;
>
>...
>
>   sess = ((P_DATA_V2 << P_OPCODE_SHIFT) | ks->key_id) |
> (multi->vpn_session_id << 8);
>   ASSERT (buf_write_prepend (buf, , 4));
>
> I think this will cause byte sex issues when talking between big and little
> endian machines.
>
> A strategically placed htonl() + ntohl() pair might help.  Have to make sure
> that the opcode really goes in as the first byte on the wire, and that the
> next 3 bytes are the 3 least significant bytes of the session ID.
>
>   uint32_t sess = htonl(
> (((P_DATA_V2 << P_OPCODE_SHIFT) | ks->key_id) << 24)
> | (multi->vpn_session_id & 0xFF)
>   );
>   ASSERT (buf_write_prepend (buf, , 4));
>
> Right? Maybe? :)
>
> I suppose this patch will improve things quite a bit for mobile clients
> which switch from network to another.
>
>   - Hessu
>



-- 
-Lev
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index c4c356d..8c0d33f 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -389,6 +389,60 @@ 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)
+{
+  struct gc_arena gc;
+  gc_init ();
+  int offset = 4; /* 1 byte opcode + 3 bytes session-id */
+
+  if (buf->len > 0 && opt->key_ctx_bi)
+{
+  struct key_ctx *ctx = >key_ctx_bi->decrypt;
+
+  /* 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 - offset) < hmac_len)
+	{
+	  gc_free ();
+	  return false;
+	}
+
+	  hmac_ctx_update (ctx->hmac, BPTR (buf) + offset + hmac_len,
+			   BLEN (buf) - offset - hmac_len);
+	  hmac_ctx_final (ctx->hmac, local_hmac);
+
+	  /* Compare locally computed HMAC with packet HMAC */
+	  if (memcmp (local_hmac, BPTR (buf) + offset, hmac_len))
+	{
+	  gc_free ();
+	  return false;
+	}
+
+	  gc_free ();
+	  return true;
+	}
+}
+
+  gc_free ();
+  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..fef7c1d 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1748,7 +1748,8 @@ pull_permission_mask (const struct context *c)
 | OPT_P_MESSAGES
 | OPT_P_EXPLICIT_NOTIFY
 | OPT_P_ECHO
-| OPT_P_PULL_MODE;
+| OPT_P_PULL_MODE
+| OPT_P_SESSION_ID;
 
   if (!c->options.route_nopull)
 flags |= (OPT_P_ROUTE | OPT_P_IPWIN32);
@@ -1825,6 +1826,13 @@ do_deferred_options (struct context *c, const unsigned int found)
 msg (D_PUSH, "OPTIONS IMPORT: --ip-win32 and/or --dhcp-option options modified");
   if (found & OPT_P_SETENV)
 msg (D_PUSH, "OPTIONS IMPORT: environment modified");
+
+  if (found & OPT_P_SESSION_ID)
+{
+  msg (D_PUSH, "OPTIONS IMPORT: session-id set");
+  c->c2.tls_multi->use_session_id = true;
+  c->c2.tls_multi->vpn_session_id = c->options.vpn_session_id;
+}
 }
 
 /*
diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 3468dab..7a6911c 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -38,6 +38,55 @@
 #include "memdbg.h"
 
 /*
+ * Update instance with new peer address
+ */
+void
+update_floated(struct multi_context *m, struct multi_instance *mi,
+	   struct mroute_addr real, uint32_t hv)
+{
+  struct mroute_addr real_old;
+
+  real_old = mi->real;
+  generate_prefix (mi);
+
+  /* remove before modifying mi->real, since it also modifies key in hash */
+  hash_remove(m->hash, _old);
+  hash_remove(m->iter, _old);
+
+  /* update address */
+  

Re: [Openvpn-devel] session-id implementation

[Heikki Hannikainen]

On Wed, 9 Jul 2014, Arne Schwabe wrote:


Am 29.06.14 18:13, schrieb Arne Schwabe:

Am 27.03.14 09:57, schrieb Lev Stipakov:

Hi,

Same patch with added NULL check in push.c:308. Turns out that
peer_info might be NULL.


I looked at the patched, a few minor nitpicks:


One more little nitpick:

  uint32_t sess;

   ...

  sess = ((P_DATA_V2 << P_OPCODE_SHIFT) | ks->key_id) | (multi->vpn_session_id 
<< 8);
  ASSERT (buf_write_prepend (buf, , 4));

I think this will cause byte sex issues when talking between big and 
little endian machines.


A strategically placed htonl() + ntohl() pair might help.  Have to make 
sure that the opcode really goes in as the first byte on the wire, and 
that the next 3 bytes are the 3 least significant bytes of the session ID.


  uint32_t sess = htonl(
(((P_DATA_V2 << P_OPCODE_SHIFT) | ks->key_id) << 24)
| (multi->vpn_session_id & 0xFF)
  );
  ASSERT (buf_write_prepend (buf, , 4));

Right? Maybe? :)

I suppose this patch will improve things quite a bit for mobile clients 
which switch from network to another.


  - Hessu

Re: [Openvpn-devel] session-id implementation

[Arne Schwabe]

Am 29.06.14 18:13, schrieb Arne Schwabe:
> Am 27.03.14 09:57, schrieb Lev Stipakov:
>> Hi,
>>
>> Same patch with added NULL check in push.c:308. Turns out that
>> peer_info might be NULL.
>>
> I looked at the patched, a few minor nitpicks:
>
> - The test should be if the IV_PROTO is at least 2 and not if exactly 2
> - use_session_id should be bool instead of int
> - If I understand the code in ssl.c tls_pre_decrypt corrrectly the
>
>   ASSERT (buf_advance (buf, op == P_DATA_V1 ? 1 : 4));
>
> will give an asserton if the other side just send a packet with only
> P_DATA_V2 as op code and no opcode.
>
> I have not checked if the addition three bytes cause any mtu related
> issues. Other than then that the patch looks good.
>
Other small nitpick. session_id should be session-id in the pushed options.

I don't really like the parsing of session_id outside parsing of other
options. It is probably better to just parse it like a normal option and
setup the session id in do_deferred_options.

Arne



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] session-id implementation

[Arne Schwabe]

Am 27.03.14 09:57, schrieb Lev Stipakov:
> Hi,
>
> Same patch with added NULL check in push.c:308. Turns out that
> peer_info might be NULL.
>
I looked at the patched, a few minor nitpicks:

- The test should be if the IV_PROTO is at least 2 and not if exactly 2
- use_session_id should be bool instead of int
- If I understand the code in ssl.c tls_pre_decrypt corrrectly the

  ASSERT (buf_advance (buf, op == P_DATA_V1 ? 1 : 4));

will give an asserton if the other side just send a packet with only
P_DATA_V2 as op code and no opcode.

I have not checked if the addition three bytes cause any mtu related
issues. Other than then that the patch looks good.

Arne



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] session-id implementation

[André Valentin]

Hi!

Great stuff, can't wait to use it!

André

On 27.03.2014 09:57, Lev Stipakov wrote:
> Hi,
>
> Same patch with added NULL check in push.c:308. Turns out that
> peer_info might be NULL.
>
> -Lev
>
> On Wed, Mar 26, 2014 at 10:52 AM, Lev Stipakov  wrote:
>> Hello,
>>
>> Despite that implementation of session-id has already been discussed,
>> I would like to propose an alternative approach.
>>
>> I suggest to use an array of multi_instance objects and session-id as
>> an item's index in that array. Why I think it is a good idea?
>>
>> 1) We could utilize less bytes for session-id, since array's max
>> length would be equal to max_clients. 3 bytes for a max of 2**24
>> clients, no need to swap one byte to the end of packet.
>>
>> [op][s1][s2][s3][data]
>>
>> 2) Lookup in array is faster than lookup by 8-byte session-id in
>> hashtable and happens with constant speed.
>>
>> 3) Proof of concept is attached!
>>
>> I have tested it with openvpn master branch (server) and ics-openvpn
>> (android) - merged without any conflicts and works nicely. Of course
>> it supports also peers without session-id support.
>>
>> This patch solves 2 issues for mobile clients:
>>
>> 1) UDP NAT timeout. After, say, 30, seconds router might close UDP
>> socket and new packet from client to VPN server originates from
>> different port. Lookup by peer address fails and packet got dropped.
>> Currently this is solved by small keep-alive interval, which drains
>> battery. With new implementation it is not an issue anymore since
>> lookup is done by session-id, therefore keep-alive interval can be
>> increased.
>>
>> 2) Transition between networks (for example WiFi->3G). Without
>> session-id client got disconnected. With this patch and
>> http://code.google.com/p/ics-openvpn/source/detail?r=bc9f3f448b9a4d95c999d3e582987840ba1e0fbf
>> transition happens seamlessly.
>>
>> I would love to hear any critics / comments!
>>
>> --
>> -Lev
>
>
>
>
> --
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] session-id implementation

[Lev Stipakov]

Hi,

Same patch with added NULL check in push.c:308. Turns out that
peer_info might be NULL.

-Lev

On Wed, Mar 26, 2014 at 10:52 AM, Lev Stipakov  wrote:
> Hello,
>
> Despite that implementation of session-id has already been discussed,
> I would like to propose an alternative approach.
>
> I suggest to use an array of multi_instance objects and session-id as
> an item's index in that array. Why I think it is a good idea?
>
> 1) We could utilize less bytes for session-id, since array's max
> length would be equal to max_clients. 3 bytes for a max of 2**24
> clients, no need to swap one byte to the end of packet.
>
> [op][s1][s2][s3][data]
>
> 2) Lookup in array is faster than lookup by 8-byte session-id in
> hashtable and happens with constant speed.
>
> 3) Proof of concept is attached!
>
> I have tested it with openvpn master branch (server) and ics-openvpn
> (android) - merged without any conflicts and works nicely. Of course
> it supports also peers without session-id support.
>
> This patch solves 2 issues for mobile clients:
>
> 1) UDP NAT timeout. After, say, 30, seconds router might close UDP
> socket and new packet from client to VPN server originates from
> different port. Lookup by peer address fails and packet got dropped.
> Currently this is solved by small keep-alive interval, which drains
> battery. With new implementation it is not an issue anymore since
> lookup is done by session-id, therefore keep-alive interval can be
> increased.
>
> 2) Transition between networks (for example WiFi->3G). Without
> session-id client got disconnected. With this patch and
> http://code.google.com/p/ics-openvpn/source/detail?r=bc9f3f448b9a4d95c999d3e582987840ba1e0fbf
> transition happens seamlessly.
>
> I would love to hear any critics / comments!
>
> --
> -Lev



-- 
-Lev
From 0f330408e4b013cd505ad9492888557daa47e905 Mon Sep 17 00:00:00 2001
From: Lev Stipakov 
Date: Tue, 11 Mar 2014 17:58:31 +0200
Subject: [PATCH] Floating implementation. Use array lookup for new opcode
 P_DATA_V2 and check HMAC for floated peers.

---
 src/openvpn/crypto.c |  54 
 src/openvpn/crypto.h |   3 ++
 src/openvpn/mudp.c   | 106 ---
 src/openvpn/multi.c  |  14 ++-
 src/openvpn/multi.h  |   2 +
 src/openvpn/options.c|  15 ++-
 src/openvpn/options.h|   4 +-
 src/openvpn/push.c   |   8 +++-
 src/openvpn/ssl.c|  59 +++---
 src/openvpn/ssl.h|   9 +++-
 src/openvpn/ssl_common.h |   4 ++
 11 files changed, 259 insertions(+), 19 deletions(-)

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index c4c356d..8c0d33f 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -389,6 +389,60 @@ 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)
+{
+  struct gc_arena gc;
+  gc_init ();
+  int offset = 4; /* 1 byte opcode + 3 bytes session-id */
+
+  if (buf->len > 0 && opt->key_ctx_bi)
+{
+  struct key_ctx *ctx = >key_ctx_bi->decrypt;
+
+  /* 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 - offset) < hmac_len)
+	{
+	  gc_free ();
+	  return false;
+	}
+
+	  hmac_ctx_update (ctx->hmac, BPTR (buf) + offset + hmac_len,
+			   BLEN (buf) - offset - hmac_len);
+	  hmac_ctx_final (ctx->hmac, local_hmac);
+
+	  /* Compare locally computed HMAC with packet HMAC */
+	  if (memcmp (local_hmac, BPTR (buf) + offset, hmac_len))
+	{
+	  gc_free ();
+	  return false;
+	}
+
+	  gc_free ();
+	  return true;
+	}
+}
+
+  gc_free ();
+  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/mudp.c b/src/openvpn/mudp.c
index 3468dab..f7ab625 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -38,6 +38,55 @@
 #include "memdbg.h"
 
 /*
+ * Update instance with new peer address
+ */
+void

[Openvpn-devel] session-id implementation

[Lev Stipakov]

Hello,

Despite that implementation of session-id has already been discussed,
I would like to propose an alternative approach.

I suggest to use an array of multi_instance objects and session-id as
an item's index in that array. Why I think it is a good idea?

1) We could utilize less bytes for session-id, since array's max
length would be equal to max_clients. 3 bytes for a max of 2**24
clients, no need to swap one byte to the end of packet.

[op][s1][s2][s3][data]

2) Lookup in array is faster than lookup by 8-byte session-id in
hashtable and happens with constant speed.

3) Proof of concept is attached!

I have tested it with openvpn master branch (server) and ics-openvpn
(android) - merged without any conflicts and works nicely. Of course
it supports also peers without session-id support.

This patch solves 2 issues for mobile clients:

1) UDP NAT timeout. After, say, 30, seconds router might close UDP
socket and new packet from client to VPN server originates from
different port. Lookup by peer address fails and packet got dropped.
Currently this is solved by small keep-alive interval, which drains
battery. With new implementation it is not an issue anymore since
lookup is done by session-id, therefore keep-alive interval can be
increased.

2) Transition between networks (for example WiFi->3G). Without
session-id client got disconnected. With this patch and
http://code.google.com/p/ics-openvpn/source/detail?r=bc9f3f448b9a4d95c999d3e582987840ba1e0fbf
transition happens seamlessly.

I would love to hear any critics / comments!

-- 
-Lev
From 284e473548a49012baf6c954a637161eec11c2e8 Mon Sep 17 00:00:00 2001
From: Lev Stipakov 
Date: Tue, 11 Mar 2014 17:58:31 +0200
Subject: [PATCH] Floating implementation. Use array lookup for new opcode
 P_DATA_V2 and check HMAC for floated peers.

---
 src/openvpn/crypto.c |  54 
 src/openvpn/crypto.h |   3 ++
 src/openvpn/mudp.c   | 106 ---
 src/openvpn/multi.c  |  14 ++-
 src/openvpn/multi.h  |   2 +
 src/openvpn/options.c|  15 ++-
 src/openvpn/options.h|   4 +-
 src/openvpn/push.c   |   8 +++-
 src/openvpn/ssl.c|  59 +++---
 src/openvpn/ssl.h|   9 +++-
 src/openvpn/ssl_common.h |   4 ++
 11 files changed, 259 insertions(+), 19 deletions(-)

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index c4c356d..8c0d33f 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -389,6 +389,60 @@ 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)
+{
+  struct gc_arena gc;
+  gc_init ();
+  int offset = 4; /* 1 byte opcode + 3 bytes session-id */
+
+  if (buf->len > 0 && opt->key_ctx_bi)
+{
+  struct key_ctx *ctx = >key_ctx_bi->decrypt;
+
+  /* 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 - offset) < hmac_len)
+	{
+	  gc_free ();
+	  return false;
+	}
+
+	  hmac_ctx_update (ctx->hmac, BPTR (buf) + offset + hmac_len,
+			   BLEN (buf) - offset - hmac_len);
+	  hmac_ctx_final (ctx->hmac, local_hmac);
+
+	  /* Compare locally computed HMAC with packet HMAC */
+	  if (memcmp (local_hmac, BPTR (buf) + offset, hmac_len))
+	{
+	  gc_free ();
+	  return false;
+	}
+
+	  gc_free ();
+	  return true;
+	}
+}
+
+  gc_free ();
+  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/mudp.c b/src/openvpn/mudp.c
index 3468dab..f7ab625 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -38,6 +38,55 @@
 #include "memdbg.h"
 
 /*
+ * Update instance with new peer address
+ */
+void
+update_floated(struct multi_context *m, struct multi_instance *mi,
+	   struct mroute_addr real, uint32_t hv)
+{
+  struct mroute_addr real_old;
+
+  real_old = mi->real;
+  generate_prefix (mi);
+
+  /* remove before modifying mi->real, since it also