Re: [Openvpn-devel] [PATCH] Peer-id patch v7

[Gert Doering]

Hi,

On Thu, Nov 27, 2014 at 05:08:09PM +0200, Lev Stipakov wrote:
> As for the second question - hard to say. If we make it opt-in, we
> probably will need to announce this feature loudly to make users aware
> of that. From the other side, it is not inconceivable to assume that
> someone might not want this. Maybe we could make it opt-out and have
> "-no-peer-id" config option?

Definitely not "a new option", as we already have --float :-)

But I can see the argument about opt-in/opt-out - I found it a useful
thing to explicitely enable the feature (so, no surprises here) but if
there is not enough advertising, people won't be able to benefit 
from it...

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


pgpu84QrDvLKn.pgp
Description: PGP signature

Re: [Openvpn-devel] [PATCH] Peer-id patch v7

[Lev Stipakov]

Hello,

Currently it should be safe, since multi_create_instance returns NULL
if amount of clients >= max_clients. In this case we won't reach that
"for" loop thanks to "if (mi)" check.

But it probably won't harm to assert if we've reached "for" loop and
could not find available "instance" item. I can implement that.

As for the second question - hard to say. If we make it opt-in, we
probably will need to announce this feature loudly to make users aware
of that. From the other side, it is not inconceivable to assume that
someone might not want this. Maybe we could make it opt-out and have
"-no-peer-id" config option?

-Lev

2014-11-27 16:22 GMT+02:00 Gert Doering :
> Hi,
>
> On Sun, Nov 23, 2014 at 05:17:11PM +0200, Lev Stipakov wrote:
>> Changes in v7:
>> A few nitpicks.
>
> Went in, and has just been pushed.  Time for your dance now :-)
>
> Question to you:
>
>> @@ -75,6 +101,16 @@ multi_get_create_instance_udp (struct multi_context *m)
>>   {
>> hash_add_fast (hash, bucket, >real, hv, mi);
>> mi->did_real_hash = true;
>> +
>> +   for (i = 0; i < m->max_clients; ++i)
>> + {
>> +   if (!m->instances[i])
>> + {
>> +   mi->context.c2.tls_multi->peer_id = i;
>> +   m->instances[i] = mi;
>> +   break;
>> + }
>> + }
>
> Is this code "safe"?  That is, if max_clients is set too low, and you have
> more than this number of clients trying to connect, what will happen?
>
> Will the server check this case before this loop, or will it just "not
> find a free instance" and leave peer_id as "0"?
>
> Maybe we should add an ASSERT() here, to be sure that whatever other pieces
> of the code do, this will always end up in a defined state  (as a separate
> patch).
>
>
>
> Second, question to the group.  Right now, this code is active unconditionally
> on the server side - what if someone does not want this behaviour, for
> whatever reason?  Shall we make it depend on --float (which right now
> doesn't do anything for a --server OpenVPN server)?
>
> This should be a fairly small change, I think, just never send the client
> a "peer-id " push-reply (so it won't use T_PACKET_V2), and check
> "if (options.float)" before checking whether it's a floated client...
> so the question is more along the line "is this a useful thing to do,
> nor not?".
>
> 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



-- 
-Lev

Re: [Openvpn-devel] [PATCH] Peer-id patch v7

[Gert Doering]

Hi,

On Sun, Nov 23, 2014 at 05:17:11PM +0200, Lev Stipakov wrote:
> Changes in v7:
> A few nitpicks.

Went in, and has just been pushed.  Time for your dance now :-)

Question to you:

> @@ -75,6 +101,16 @@ multi_get_create_instance_udp (struct multi_context *m)
>   {
> hash_add_fast (hash, bucket, >real, hv, mi);
> mi->did_real_hash = true;
> +
> +   for (i = 0; i < m->max_clients; ++i)
> + {
> +   if (!m->instances[i])
> + {
> +   mi->context.c2.tls_multi->peer_id = i;
> +   m->instances[i] = mi;
> +   break;
> + }
> + }

Is this code "safe"?  That is, if max_clients is set too low, and you have
more than this number of clients trying to connect, what will happen?

Will the server check this case before this loop, or will it just "not
find a free instance" and leave peer_id as "0"?

Maybe we should add an ASSERT() here, to be sure that whatever other pieces
of the code do, this will always end up in a defined state  (as a separate
patch).



Second, question to the group.  Right now, this code is active unconditionally
on the server side - what if someone does not want this behaviour, for
whatever reason?  Shall we make it depend on --float (which right now 
doesn't do anything for a --server OpenVPN server)?

This should be a fairly small change, I think, just never send the client
a "peer-id " push-reply (so it won't use T_PACKET_V2), and check 
"if (options.float)" before checking whether it's a floated client...  
so the question is more along the line "is this a useful thing to do, 
nor not?".

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


pgp9uijzH9E6v.pgp
Description: PGP signature

Re: [Openvpn-devel] [PATCH] Peer-id patch v7

[Steffan Karger]

On 23-11-14 16:17, Lev Stipakov wrote:
> Added new packet format P_DATA_V2, which includes peer-id. If server
> supports, client sends all data packets in the new format. When data
> packet arrives, server identifies peer by peer-id. If peer's ip/port has
> changed, server assumes that client has floated, verifies HMAC and
> updates ip/port in internal structs.

ACK

During the hackathon, the group decided that this indeed is a nice way
to support floating clients. I did a code review, but no real testing
(just 'make check', including t_client). Lev, Gert and Arne have however
performed real-life tests with multiple iterations of this patch.

A remaining issue is that the memory usage of a client increases for
each 'float', because generate_prefix(mi) allocates memory on each call,
which will only be free'd when the client disconnects. However, we
agreed to fix that in a separate patch.

-Steffan

[Openvpn-devel] [PATCH] Peer-id patch v7

[Lev Stipakov]

Added new packet format P_DATA_V2, which includes peer-id. If server
supports, client sends all data packets in the new format. When data
packet arrives, server identifies peer by peer-id. If peer's ip/port has
changed, server assumes that client has floated, verifies HMAC and
updates ip/port in internal structs.

Changes in v7:
A few nitpicks.

Changes in v6:
Fixed: Make sure float won't happen if hmac check failed (regression).
Fixed: Access outside of bounds of array, which has caused memory corruption 
and crash.
Various review fixes.

Changes in v5:
Protection agains replay attack by commiting float changes only after
existing packet processing flow has completed.

If peer floats to an address which is already taken by another active
session, drop float packet, otherwise disconnect existing session.

Changes in v4:
Handles correctly float to an address which is used by another peer.
This also has fixed crash on assert in multi_client_disconnect.

Changes in v3:
Bugfix: If float happens after TLS renegotiation and there are no
data packets between reneg and float, server will not recognize floated client.
---
 src/openvpn/forward.c| 50 -
 src/openvpn/forward.h| 30 ---
 src/openvpn/init.c   | 12 +-
 src/openvpn/mudp.c   | 57 +---
 src/openvpn/mudp.h   |  2 +-
 src/openvpn/multi.c  | 97 ++--
 src/openvpn/multi.h  | 19 ++
 src/openvpn/options.c|  6 +++
 src/openvpn/options.h|  4 ++
 src/openvpn/push.c   | 13 +++
 src/openvpn/ssl.c| 74 
 src/openvpn/ssl.h| 15 +++-
 src/openvpn/ssl_common.h |  4 ++
 13 files changed, 332 insertions(+), 51 deletions(-)

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 27b775f..b772d9a 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -722,20 +722,11 @@ read_incoming_link (struct context *c)
   perf_pop ();
 }

-/*
- * Input:  c->c2.buf
- * Output: c->c2.to_tun
- */
-
-void
-process_incoming_link (struct context *c)
+bool
+process_incoming_link_part1 (struct context *c, struct link_socket_info *lsi, 
bool floated)
 {
   struct gc_arena gc = gc_new ();
-  bool decrypt_status;
-  struct link_socket_info *lsi = get_link_socket_info (c);
-  const uint8_t *orig_buf = c->c2.buf.data;
-
-  perf_push (PERF_PROC_IN_LINK);
+  bool decrypt_status = false;

   if (c->c2.buf.len > 0)
 {
@@ -805,7 +796,7 @@ process_incoming_link (struct context *c)
   * will load crypto_options with the correct encryption key
   * and return false.
   */
- if (tls_pre_decrypt (c->c2.tls_multi, >c2.from, >c2.buf, 
>c2.crypto_options))
+ if (tls_pre_decrypt (c->c2.tls_multi, >c2.from, >c2.buf, 
>c2.crypto_options, floated))
{
  interval_action (>c2.tmp_int);

@@ -832,11 +823,25 @@ process_incoming_link (struct context *c)
  /* decryption errors are fatal in TCP mode */
  register_signal (c, SIGUSR1, "decryption-error"); /* SOFT-SIGUSR1 -- 
decryption error in TCP mode */
  msg (D_STREAM_ERRORS, "Fatal decryption error 
(process_incoming_link), restarting");
- goto done;
}
-
+#else /* ENABLE_CRYPTO */
+  decrypt_status = true;
 #endif /* ENABLE_CRYPTO */
+}
+  else
+{
+  buf_reset (>c2.to_tun);
+}
+  gc_free ();

+  return decrypt_status;
+}
+
+void
+process_incoming_link_part2 (struct context *c, struct link_socket_info *lsi, 
const uint8_t *orig_buf)
+{
+  if (c->c2.buf.len > 0)
+{
 #ifdef ENABLE_FRAGMENT
   if (c->c2.fragment)
fragment_incoming (c->c2.fragment, >c2.buf, >c2.frame_fragment);
@@ -903,9 +908,20 @@ process_incoming_link (struct context *c)
 {
   buf_reset (>c2.to_tun);
 }
- done:
+}
+
+void
+process_incoming_link (struct context *c)
+{
+  perf_push (PERF_PROC_IN_LINK);
+
+  struct link_socket_info *lsi = get_link_socket_info (c);
+  const uint8_t *orig_buf = c->c2.buf.data;
+
+  process_incoming_link_part1(c, lsi, false);   
+  process_incoming_link_part2(c, lsi, orig_buf);
+
   perf_pop ();
-  gc_free ();
 }

 /*
diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
index 1830a00..eccbf36 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -127,12 +127,11 @@ void encrypt_sign (struct context *c, bool comp_frag);
  */
 void read_incoming_link (struct context *c);

-
 /**
- * Process a packet read from the external network interface.
+ * Starts processing a packet read from the external network interface.
  * @ingroup external_multiplexer
  *
- * This function controls the processing of a data channel packet which
+ * This function starts the processing of a data channel packet which
  * has come out of a VPN tunnel.  It's high-level structure is as follows:
  * - Verify that a nonzero length packet has been received from a valid
  *   source address for the given context \a