Skip to contentSkip to conversation

Support for disabled peer-id

PATCH Superseded Lev Stipakov 2 participants 5 messages Oct 9, 2015 – Sep 17, 2016

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Lev Stipakov

author
#0 lore raw

PATCH v2 1/1 Support for disabled peer-id

v2:
* Add round brackets for clarity.
* Rephrase comment.

v1:
* When peer-id value is 0xFFFFFF, server should ignore it and treat packet
in a same way as P_DATA_V1.
* Make sure that issued peer-id does not exceed 0xFFFFFF.

---
 src/openvpn/mudp.c  | 14 +++++++++++---
 src/openvpn/multi.c |  3 ++-
 2 files changed, 13 insertions(+), 4 deletions(-)
src/openvpn/mudp.c +11 -3
index 57118f8..fcbb47d 100644
@@ -60,12 +60,16 @@ multi_get_create_instance_udp (struct multi_context *m, bool *floated)
       struct hash_bucket *bucket = hash_bucket (hash, hv);
       uint8_t* ptr = BPTR(&m->top.c2.buf);
       uint8_t op = ptr[0] >> P_OPCODE_SHIFT;
+      bool v2 = (op == P_DATA_V2) && (m->top.c2.buf.len >= (1 + 3));
+      bool peer_id_disabled = false;
 
       /* make sure buffer has enough length to read opcode (1 byte) and peer-id (3 bytes) */
-      if (op == P_DATA_V2 && m->top.c2.buf.len >= (1 + 3))
+      if (v2)
 	{
 	  uint32_t peer_id = ntohl(*(uint32_t*)ptr) & 0xFFFFFF;
-	  if ((peer_id < m->max_clients) && (m->instances[peer_id]))
+	  peer_id_disabled = (peer_id == 0xFFFFFF);
+
+	  if (!peer_id_disabled && (peer_id < m->max_clients) && (m->instances[peer_id]))
 	    {
 	      mi = m->instances[peer_id];
 

@@ -80,7 +84,7 @@ multi_get_create_instance_udp (struct multi_context *m, bool *floated)
 	      }
 	    }
 	}
-      else
+      if (!v2 || peer_id_disabled)
 	{
 	  he = hash_lookup_fast (hash, bucket, &real, hv);
 	  if (he)

@@ -103,10 +107,14 @@ multi_get_create_instance_udp (struct multi_context *m, bool *floated)
 		      hash_add_fast (hash, bucket, &mi->real, hv, mi);
 		      mi->did_real_hash = true;
 
+		      /* In future we might want to use P_DATA_V2 but not need peer-id/float functionality */
 		      for (i = 0; i < m->max_clients; ++i)
 			{
 			  if (!m->instances[i])
 			    {
+			      /* issued peer-id should fit into 3 bytes to avoid wrap and cannot have reserved value 0xFFFFFF */
+			      ASSERT(i < 0xFFFFFF);
+
 			      mi->context.c2.tls_multi->peer_id = i;
 			      m->instances[i] = mi;
 			      break;
src/openvpn/multi.c +2 -1
index 902c4dc..76f5a44 100644
@@ -562,7 +562,8 @@ multi_close_instance (struct multi_context *m,
 	}
 #endif
 
-      m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+      if (mi->context.c2.tls_multi->peer_id != 0xFFFFFF)
+        m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
 
       schedule_remove_entry (m->schedule, (struct schedule_entry *) mi);
Signature · 1 line
1.9.1
#1 lore raw

Hi,

The patch looks functionally ok. Just one minor remark:

Quoted 5 lines · Lev Stipakov
src/openvpn/mudp.c +1 -0
@@ -103,10 +107,14 @@ multi_get_create_instance_udp (struct multi_context *m, bool *floated)
  		      hash_add_fast (hash, bucket, &mi->real, hv, mi);
  		      mi->did_real_hash = true;
 
+		      /* In future we might want to use P_DATA_V2 but not need peer-id/float functionality */

src/openvpn/mudp.c multi_get_create_instance_udp (struct multi_context *m, bool *floated) · line 120

Quoted 10 lines
src/openvpn/mudp.c +3 -0
  		      for (i = 0; i < m->max_clients; ++i)
  			{
  			  if (!m->instances[i])
  			    {
+			      /* issued peer-id should fit into 3 bytes to avoid wrap and cannot have reserved value 0xFFFFFF */
+			      ASSERT(i < 0xFFFFFF);
+
  			      mi->context.c2.tls_multi->peer_id = i;
  			      m->instances[i] = mi;
  			      break;

Checking against the max peer-id indeed makes sense, but this assert can be made simpler by just putting

   ASSERT (m->max-clients < 0xFFFFFF);

before the loop. Additionally, I think we should add a user-friendly check in options.c, to give a proper error message on startup when a user tries to set --max-clients >= 0xFFFFFF.

Finally, why did you put this comment at this location? Shouldn't it be in the place where we *send* the peer-id? (That is the part that is not implemented now, right?)

-Steffan

author
#2 lore raw

PATCH v3 1/1 Support for disabled peer-id

From: Lev Stipakov <[email protected]...>

v3:
* move assert outside of loop
* add max-clients value check to options

v2:
* Add round brackets for clarity
* Rephrase comment

Support for disabled peer-id

When peer-id value is 0xFFFFFF, server should ignore it and treat packet in a same way as P_DATA_V1.

---
 src/openvpn/mudp.c    | 13 ++++++++++---
 src/openvpn/multi.c   |  3 ++-
 src/openvpn/options.c |  5 +++++
 3 files changed, 17 insertions(+), 4 deletions(-)
src/openvpn/mudp.c +10 -3
index 21a7e97..59795eb 100644
@@ -64,12 +64,16 @@ multi_get_create_instance_udp (struct multi_context *m, bool *floated)
       struct hash_bucket *bucket = hash_bucket (hash, hv);
       uint8_t* ptr = BPTR(&m->top.c2.buf);
       uint8_t op = ptr[0] >> P_OPCODE_SHIFT;
+      bool v2 = (op == P_DATA_V2) && (m->top.c2.buf.len >= (1 + 3));
+      bool peer_id_disabled = false;
 
       /* make sure buffer has enough length to read opcode (1 byte) and peer-id (3 bytes) */
-      if (op == P_DATA_V2 && m->top.c2.buf.len >= (1 + 3))
+      if (v2)
 	{
 	  uint32_t peer_id = ntohl(*(uint32_t*)ptr) & 0xFFFFFF;
-	  if ((peer_id < m->max_clients) && (m->instances[peer_id]))
+	  peer_id_disabled = (peer_id == 0xFFFFFF);
+
+	  if (!peer_id_disabled && (peer_id < m->max_clients) && (m->instances[peer_id]))
 	    {
 	      mi = m->instances[peer_id];
 

@@ -84,7 +88,7 @@ multi_get_create_instance_udp (struct multi_context *m, bool *floated)
 	      }
 	    }
 	}
-      else
+      if (!v2 || peer_id_disabled)
 	{
 	  he = hash_lookup_fast (hash, bucket, &real, hv);
 	  if (he)

@@ -107,6 +111,9 @@ multi_get_create_instance_udp (struct multi_context *m, bool *floated)
 		      hash_add_fast (hash, bucket, &mi->real, hv, mi);
 		      mi->did_real_hash = true;
 
+		      /* max_clients must be less then max peer-id value */
+		      ASSERT(m->max_clients < 0xFFFFFF);
+
 		      for (i = 0; i < m->max_clients; ++i)
 			{
 			  if (!m->instances[i])
src/openvpn/multi.c +2 -1
index ba7f2c0..73b211e 100644
@@ -605,7 +605,8 @@ multi_close_instance (struct multi_context *m,
 	}
 #endif
 
-      m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+      if (mi->context.c2.tls_multi->peer_id != 0xFFFFFF)
+	m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
 
       schedule_remove_entry (m->schedule, (struct schedule_entry *) mi);
src/openvpn/options.c +5 -0
index c9688c3..493ffe6 100644
@@ -5893,6 +5893,11 @@ add_option (struct options *options,
 	  msg (msglevel, "--max-clients must be at least 1");
 	  goto err;
 	}
+      if (max_clients >= 0xFFFFFF) /* max peer-id value */
+	{
+	  msg (msglevel, "--max-clients must be less than 0xFFFFFF");
+	  goto err;
+	}
       options->max_clients = max_clients;
     }
   else if (streq (p[0], "max-routes-per-client") && p[1] && !p[2])
Signature · 1 line
1.9.1
#3 lore raw

Hi,

Quoted 12 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>

v3:
* move assert outside of loop
* add max-clients value check to options

[...]

src/openvpn/options.c +0 -0
@@ -5893,6 +5893,11 @@ add_option (struct options *options,
          msg (msglevel, "--max-clients must be at least 1");

src/openvpn/options.c add_option (struct options *options, · line 5903

Quoted 10 lines
src/openvpn/options.c +5 -0
          goto err;
        }
+      if (max_clients >= 0xFFFFFF) /* max peer-id value */
+       {
+         msg (msglevel, "--max-clients must be less than 0xFFFFFF");
+         goto err;
+       }
       options->max_clients = max_clients;
     }
   else if (streq (p[0], "max-routes-per-client") && p[1] && !p[2])

Thanks. One more nitpick though. There are quite some occurrences of 0xFFFFFF now. I think a #define MAX_PEER_ID 0xFFFFFF is warranted.

And if you do so, please change to error message to print the decimal value, since that is also how we expect users to specify the --max-clients argument.

-Steffan

author
#4 lore raw

PATCH v4 1/1 Support for disabled peer-id

From: Lev Stipakov <[email protected]...>

v4:
- replace magic number with define
- show user a decimal value instead of hex

v3:
* move assert outside of loop
* add max-clients value check to options

v2:
* Add round brackets for clarity
* Rephrase comment

Support for disabled peer-id

When peer-id value is 0xFFFFFF, server should ignore it and treat packet in a same way as P_DATA_V1.

---
 src/openvpn/mudp.c    | 13 ++++++++++---
 src/openvpn/multi.c   |  3 ++-
 src/openvpn/openvpn.h |  2 ++
 src/openvpn/options.c |  5 +++++
 4 files changed, 19 insertions(+), 4 deletions(-)
src/openvpn/mudp.c +10 -3
index 21a7e97..fec5e8d 100644
@@ -64,12 +64,16 @@ multi_get_create_instance_udp (struct multi_context *m, bool *floated)
       struct hash_bucket *bucket = hash_bucket (hash, hv);
       uint8_t* ptr = BPTR(&m->top.c2.buf);
       uint8_t op = ptr[0] >> P_OPCODE_SHIFT;
+      bool v2 = (op == P_DATA_V2) && (m->top.c2.buf.len >= (1 + 3));
+      bool peer_id_disabled = false;
 
       /* make sure buffer has enough length to read opcode (1 byte) and peer-id (3 bytes) */
-      if (op == P_DATA_V2 && m->top.c2.buf.len >= (1 + 3))
+      if (v2)
 	{
 	  uint32_t peer_id = ntohl(*(uint32_t*)ptr) & 0xFFFFFF;
-	  if ((peer_id < m->max_clients) && (m->instances[peer_id]))
+	  peer_id_disabled = (peer_id == MAX_PEER_ID);
+
+	  if (!peer_id_disabled && (peer_id < m->max_clients) && (m->instances[peer_id]))
 	    {
 	      mi = m->instances[peer_id];
 

@@ -84,7 +88,7 @@ multi_get_create_instance_udp (struct multi_context *m, bool *floated)
 	      }
 	    }
 	}
-      else
+      if (!v2 || peer_id_disabled)
 	{
 	  he = hash_lookup_fast (hash, bucket, &real, hv);
 	  if (he)

@@ -107,6 +111,9 @@ multi_get_create_instance_udp (struct multi_context *m, bool *floated)
 		      hash_add_fast (hash, bucket, &mi->real, hv, mi);
 		      mi->did_real_hash = true;
 
+		      /* max_clients must be less then max peer-id value */
+		      ASSERT(m->max_clients < MAX_PEER_ID);
+
 		      for (i = 0; i < m->max_clients; ++i)
 			{
 			  if (!m->instances[i])
src/openvpn/multi.c +2 -1
index ba7f2c0..3bc6ee9 100644
@@ -605,7 +605,8 @@ multi_close_instance (struct multi_context *m,
 	}
 #endif
 
-      m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+      if (mi->context.c2.tls_multi->peer_id != MAX_PEER_ID)
+	m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
 
       schedule_remove_entry (m->schedule, (struct schedule_entry *) mi);
src/openvpn/openvpn.h +2 -0
index 1a458f1..ec8075d 100644
@@ -595,4 +595,6 @@ struct context
 #define CIPHER_ENABLED(c) (false)
 #endif
 
+#define MAX_PEER_ID 0xFFFFFF
+
 #endif
src/openvpn/options.c +5 -0
index c9688c3..29c76a6 100644
@@ -5893,6 +5893,11 @@ add_option (struct options *options,
 	  msg (msglevel, "--max-clients must be at least 1");
 	  goto err;
 	}
+      if (max_clients >= MAX_PEER_ID) /* max peer-id value */
+	{
+	  msg (msglevel, "--max-clients must be less than 16777215");
+	  goto err;
+	}
       options->max_clients = max_clients;
     }
   else if (streq (p[0], "max-routes-per-client") && p[1] && !p[2])
Signature · 1 line
1.9.1