Add 'printing of port number' to mroute_addr_print_ex() for v4-mapped v6.

For whatever reason, this function never printed port numbers for IPv6 addresses (but it did for IPv4) - which creates a bit of confusion for IPv6-mapped v4 addresses on a dual stack socket, that will have ports numbers printed or not, depending on whether it's a dual-stack v6 socket or single-stack v4.

This will not(!) add printing of port numbers for "proper" v6 addresses yet, because that might have adverse side effects to address parsing elsewhere.

Signed-off-by Gert Doering <[email protected]...>

---
 src/openvpn/mroute.c | 7 +++++++
 1 file changed, 7 insertions(+)

src/openvpn/mroute.c +0 -0

index 28940a8d..134f4c00 100644

src/openvpn/mroute.c +0 -0

                 }
                 else
                 {

src/openvpn/mroute.c mroute_addr_print_ex(const struct mroute_addr *ma, · line 482

Quoted 3 lines

src/openvpn/mroute.c +3 -0

+		    /* we only print port numbers for v4mapped v6 as of
+		     * today, because "v6addr:port" is too ambiguous
+		     */

The comments above are indented with tab+spaces, while the code below is just all spaces. Please use the all spaces everywhere.

src/openvpn/mroute.c mroute_addr_print_ex(const struct mroute_addr *ma, · line 486

Quoted 4 lines

src/openvpn/mroute.c +4 -0

+                    if (maddr.type & MR_WITH_PORT)
+                    {
+                        buf_printf(&out, ":%d", ntohs(maddr.v6.port));
+                    }

I don't understand how is this solving the ambiguity?

Or you are just
saying: "we can't do much, let's just print the port anyway"? My suggestion would be to surround the address with [], so basically change the printf format above from %s to [%s]. Does it make sense?

Hi,

Quoted 6 lines · Antonio Quartulli

src/openvpn/mroute.c mroute_addr_print_ex(const struct mroute_addr *ma, · line 482

Quoted 3 lines

src/openvpn/mroute.c +3 -0

+		    /* we only print port numbers for v4mapped v6 as of
+		     * today, because "v6addr:port" is too ambiguous
+		     */

The comments above are indented with tab+spaces, while the code below is just all spaces. Please use the all spaces everywhere.

Gah. That was the intention, but the comment was added later on and using the wrong editor :-)

Quoted 6 lines

src/openvpn/mroute.c mroute_addr_print_ex(const struct mroute_addr *ma, · line 486

Quoted 4 lines

src/openvpn/mroute.c +4 -0

+                    if (maddr.type & MR_WITH_PORT)
+                    {
+                        buf_printf(&out, ":%d", ntohs(maddr.v6.port));
+                    }

I don't understand how is this solving the ambiguity?

These are *v4* addresses, just masquerading as v6 in the socket structure.

So 1.2.3.4:567 is never ambiguous.

For "true v6 addresses" (the other branch) we keep on not printing the port number.

Quoted 4 lines

Or you are just
saying: "we can't do much, let's just print the port anyway"? My suggestion would be to surround the address with [], so basically change the printf format above from %s to [%s]. Does it make sense?

This is something I want to discuss. Changing mroute_print_addr_ex() for "true v6 addresses" and printing the port number in a new format will affect status file printing, management interface, etc. - so it needs to be well considered.

*This* patch just fixes the discrepancy that v4 addresses are printed "with port" if doing "proto udp4", and "without port" if a v4 connect comes in on a "proto udp / v6only=no" socket. (We do not need to discuss how many problems the v4mapped v6 address format brings with it...)

If an attacker sends lots of RESET packets from different source ports (but the same source address), every packet will create a new multi_instance, leading to resource waste on the server, and to lots of reply packets while OpenVPN tries to establish a TLS handshake.

This can be rate-limited with "connect-freq", but if this is set too tightly, an attacker can drown out legitimate users of the same OpenVPN server.

So, when deciding whether or not to create a new instance, iterate over all existing instances, counting all from the same source IP (ignoring source port info) that are "in TLS negotiation" state - if more than <n> instances are already active, refuse new instance.

The cutoff parameter can be configured by a newly introduced 3rd argument to "connect-freq". So something like this might be reasonable for a medium-sized server:

   connect-freq 20 20 3

("permit 20 new connections per 20 seconds, but only 3 from the same source IP address")

Drawback: if many users are sitting behind a shared NAT ip address and they all reconnect at the same time, session setup will take longer for some of the users while the server is still handshaking with others.

If an attacker sends lots of RESET packets from different source ports (but the same source address), every packet will create a new multi_instance, leading to resource waste on the server, and to lots of reply packets while OpenVPN tries to establish a TLS handshake.

This can be rate-limited with "connect-freq", but if this is set too tightly, an attacker can drown out legitimate users of the same OpenVPN server.

So, when deciding whether or not to create a new instance, iterate over all existing instances, counting all from the same source IP (ignoring source port info) that are "in TLS negotiation" state - if more than <n> instances are already active, refuse new instance.

The cutoff parameter can be configured by a newly introduced 3rd argument to "connect-freq". So something like this might be reasonable for a medium-sized server:

   connect-freq 20 20 3

("permit 20 new connections per 20 seconds, but only 3 from the same source IP address")

Drawback: if many users are sitting behind a shared NAT ip address and they all reconnect at the same time, session setup will take longer for some of the users while the server is still handshaking with others.

v1:
this is really a "request for discussion" and needs removal of lots of debugging printout. Also, it needs to be configurable.

v2:
make it configurable

v3:
Swap order of checks - check "per instance dangling limit" before generic "connect-freq" limiter. Otherwise a really massive flood of incoming RESET packets from a single host will trigger the global "connect-freq" limiter *first*, and thus drown out other sessions (again).
Clarify wording of refusal message.

Signed-off-by Gert Doering <[email protected]...>

---
 src/openvpn/mudp.c    | 108 +++++++++++++++++++++++++++++++++++++++++-
 src/openvpn/options.c |   6 ++-
 src/openvpn/options.h |   1 +
 3 files changed, 113 insertions(+), 2 deletions(-)

src/openvpn/mudp.c +74 -0

index a604d217..be8e0fe7 100644
@@ -41,6 +41,111 @@
 #include <sys/inotify.h>
 #endif
 
+/* compare two mroute_addr, ignoring the port info
+ */
+static inline bool
+mroute_addr_equal_no_port(const struct mroute_addr *a1,
+                          const struct mroute_addr *a2)
+{
+    if (a1->type != a2->type)
+    {
+        return false;
+    }
+    if (a1->netbits != a2->netbits)
+    {
+        return false;
+    }
+    if (a1->len != a2->len)
+    {
+        return false;
+    }
+    int len = a1->len;
+    if ( len == 6 || len == 18 )	/* AF_INET or AF_INET6 + Port */
+    {
+	len -= 2;
+    }
+    return memcmp(a1->raw_addr, a2->raw_addr, len) == 0;
+}
+
+/*
+ * allow no more than <n> instances for the same source IP
+ * in "key state not active yet" state - this is almost always a
+ * state exhaustion and/or reflection attack
+ *
+ * real use cases might hit "multiple instances for the same source IP"
+ * if there are multiple users behind the same NAT gateway - but those
+ * will not be in "ks->state < S_START" for more than a few seconds
+ * (so we might see a few false positives if many users behind the same
+ * NAT gateway restart their VPNs at the same time, but TLS handshake
+ * will succeed for one after another, eventually for all)
+ */
+
+
+bool
+dangling_instances_per_source_allowed(struct multi_context *m,
+				      struct mroute_addr *new_source)
+{
+    /* nothing configured -> default = allow all */
+    if ( m->top.options.cf_max_dangling <= 0 )
+    {
+	return true;
+    }
+
+    struct gc_arena gc = gc_new();
+    msg(M_INFO, "MULTI dipsa: new_source=%s (len=%d)",
+		mroute_addr_print(new_source, &gc), new_source->len );
+
+    struct hash_iterator hi;
+    struct hash_element *he;
+    int count = 0;
+
+    hash_iterator_init(m->iter, &hi);
+    while ((he = hash_iterator_next(&hi)))
+    {
+	struct multi_instance *mi = (struct multi_instance *) he->value;
+
+	msg(M_INFO, "MULTI dipsa: instance: halt=%d real=%s",
+		mi->halt, mroute_addr_print(&mi->real, &gc));
+	if (!mi->halt && mroute_addr_equal_no_port(new_source, &mi->real))
+	{
+	    msg(M_INFO, "MULTI dipsa: ip match found!");
+
+	    struct tls_multi * tls_multi = mi->context.c2.tls_multi;
+
+	    for (int i=0; i<TM_SIZE; i++ )
+	    {
+	        msg(M_INFO, "MULTI dipsa: session[%d].key[KS_PRIMARY].state=%d",

src/openvpn/mudp.c +21 -0

+	    {
+	        msg(M_INFO, "MULTI dipsa: dangling session (ks->state=%d)",
+			    ks->state );
+	        count++;
+	    }
+	}
+    }
+    hash_iterator_free(&hi);
+
+    gc_free(&gc);
+
+    msg( M_INFO, "MULTI dipsa: count=%d", count );
+    if ( count >= m->top.options.cf_max_dangling )
+    {
+	msg( M_INFO, "MULTI dipsa: maximum number of instances doing TLS handshake for this source IP reached (%d)", count );
+	return false;
+    }
+
+    return true;
+}
+
 /*
  * Get a client instance based on real address.  If
  * the instance doesn't exist, create it while

src/openvpn/mudp.c +0 -0

                 {
                     mi = multi_create_instance(m, &real);
                     if (mi)

src/openvpn/options.c +5 -1

index 26f056fc..b8e28df6 100644
@@ -6644,7 +6644,7 @@ add_option(struct options *options,
         options->real_hash_size = real;
         options->virtual_hash_size = real;
     }
-    else if (streq(p[0], "connect-freq") && p[1] && p[2] && !p[3])
+    else if (streq(p[0], "connect-freq") && p[1] && p[2] && !p[4])
     {
         int cf_max, cf_per;
 

@@ -6658,6 +6658,10 @@ add_option(struct options *options,
         }
         options->cf_max = cf_max;
         options->cf_per = cf_per;
+	if ( p[3] )
+	{
+	    options->cf_max_dangling = atoi(p[3]);
+	}
     }
     else if (streq(p[0], "max-clients") && p[1] && !p[2])
     {

src/openvpn/options.h +0 -0

index e2b38939..26a78878 100644