[Openvpn-devel] Rate-limit incoming P_CONTROL_HARD_RESET_* packets.

Message ID 20181206171033.61310-1-gert@greenie.muc.de
State New
Headers show
Series [Openvpn-devel] Rate-limit incoming P_CONTROL_HARD_RESET_* packets. | expand

Commit Message

Gert Doering Dec. 6, 2018, 5:10 p.m. UTC
Creation of new instances (= new incoming reset packets with different
source IP addresses / source ports) can be rate-limited in the current
code by use of the "--connect-freq" option.

For packets sent with the same source port, OpenVPN would dilligently
reply to every single packet, causing route reflection issues (plus
using somewhat more server cycles).  This patch introduces a timestamp
in the tls_multi context which records when the last "reset" packet
was seen, and ignores all further "reset" packets coming in in the
next 10 second interval.

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/ssl.c        | 14 ++++++++++++++
 src/openvpn/ssl_common.h |  1 +
 2 files changed, 15 insertions(+)

Comments

Steffan Karger July 14, 2019, 9:45 a.m. UTC | #1
Hi,

On 06-12-18 18:10, Gert Doering wrote:
> Creation of new instances (= new incoming reset packets with different
> source IP addresses / source ports) can be rate-limited in the current
> code by use of the "--connect-freq" option.
> 
> For packets sent with the same source port, OpenVPN would dilligently
> reply to every single packet, causing route reflection issues (plus
> using somewhat more server cycles).  This patch introduces a timestamp
> in the tls_multi context which records when the last "reset" packet
> was seen, and ignores all further "reset" packets coming in in the
> next 10 second interval.

This makes sense, but why 10s? Our default connect-retry value is 5
seconds. Wouldn't 5 (or 4?) be a better value in that regard?

> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>  src/openvpn/ssl.c        | 14 ++++++++++++++
>  src/openvpn/ssl_common.h |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 74b88ce6..3078d76c 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -3468,6 +3468,20 @@ tls_pre_decrypt(struct tls_multi *multi,
>                          print_link_socket_actual(from, &gc));
>                      goto error;
>                  }
> +
> +                /* only permit one is_hard_reset() packet per 10 seconds,
> +                 * ignore more frequent packets
> +                 */
> +                time_t now = time(NULL);
> +                if ( now - multi->last_hard_reset_seen < 10 )

I think we use the "if (now - multi->last_hard_reset_seen < 10)" style
in the vast majority of the code base.

> +                {
> +                    msg(D_MULTI_ERRORS, "TLS: tls_pre_decrypt: ignore incoming"
> +                                        " '%s' (only %ds since last RESET)",
> +                        packet_opcode_name(op),
> +                        (int)(now - multi->last_hard_reset_seen) );

Here too, no space before ).

> +                    goto error;
> +                }
> +                multi->last_hard_reset_seen = now;
>              }
>  
>              /*
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index 7bf82b3a..e71696b5 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -515,6 +515,7 @@ struct tls_multi
>       */
>      int n_hard_errors; /* errors due to TLS negotiation failure */
>      int n_soft_errors; /* errors due to unrecognized or failed-to-authenticate incoming packets */
> +    time_t last_hard_reset_seen;	/* rate-limit incoming hard reset */
>  
>      /*
>       * Our locked common name, username, and cert hashes (cannot change during the life of this tls_multi object)
> 

Otherwise this looks good to me. I'll ACK once I understand your pick
for the timeout value.

-Steffan
Antonio Quartulli March 25, 2021, 11:30 p.m. UTC | #2
Hi,

On 14/07/2019 11:45, Steffan Karger wrote:
> Hi,
> 
> On 06-12-18 18:10, Gert Doering wrote:
>> Creation of new instances (= new incoming reset packets with different
>> source IP addresses / source ports) can be rate-limited in the current
>> code by use of the "--connect-freq" option.
>>
>> For packets sent with the same source port, OpenVPN would dilligently

Did you mean "source IP"? or you really meant same port?
Same IP/port should allow the server to match the request to an existing
session, so I presume the packet would be ignored?

(Maybe I am wrong)

>> reply to every single packet, causing route reflection issues (plus
>> using somewhat more server cycles).  This patch introduces a timestamp
>> in the tls_multi context which records when the last "reset" packet
>> was seen, and ignores all further "reset" packets coming in in the
>> next 10 second interval.
> 

Is this patch something you are considering on top of "Stop
state-exhaustion attacks from a single source address." ?


Regards,


> This makes sense, but why 10s? Our default connect-retry value is 5
> seconds. Wouldn't 5 (or 4?) be a better value in that regard?
> 
>> Signed-off-by: Gert Doering <gert@greenie.muc.de>
>> ---
>>  src/openvpn/ssl.c        | 14 ++++++++++++++
>>  src/openvpn/ssl_common.h |  1 +
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
>> index 74b88ce6..3078d76c 100644
>> --- a/src/openvpn/ssl.c
>> +++ b/src/openvpn/ssl.c
>> @@ -3468,6 +3468,20 @@ tls_pre_decrypt(struct tls_multi *multi,
>>                          print_link_socket_actual(from, &gc));
>>                      goto error;
>>                  }
>> +
>> +                /* only permit one is_hard_reset() packet per 10 seconds,
>> +                 * ignore more frequent packets
>> +                 */
>> +                time_t now = time(NULL);
>> +                if ( now - multi->last_hard_reset_seen < 10 )
> 
> I think we use the "if (now - multi->last_hard_reset_seen < 10)" style
> in the vast majority of the code base.
> 
>> +                {
>> +                    msg(D_MULTI_ERRORS, "TLS: tls_pre_decrypt: ignore incoming"
>> +                                        " '%s' (only %ds since last RESET)",
>> +                        packet_opcode_name(op),
>> +                        (int)(now - multi->last_hard_reset_seen) );
> 
> Here too, no space before ).
> 
>> +                    goto error;
>> +                }
>> +                multi->last_hard_reset_seen = now;
>>              }
>>  
>>              /*
>> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
>> index 7bf82b3a..e71696b5 100644
>> --- a/src/openvpn/ssl_common.h
>> +++ b/src/openvpn/ssl_common.h
>> @@ -515,6 +515,7 @@ struct tls_multi
>>       */
>>      int n_hard_errors; /* errors due to TLS negotiation failure */
>>      int n_soft_errors; /* errors due to unrecognized or failed-to-authenticate incoming packets */
>> +    time_t last_hard_reset_seen;	/* rate-limit incoming hard reset */
>>  
>>      /*
>>       * Our locked common name, username, and cert hashes (cannot change during the life of this tls_multi object)
>>
> 
> Otherwise this looks good to me. I'll ACK once I understand your pick
> for the timeout value.
> 
> -Steffan
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
Antonio Quartulli June 24, 2022, 9:13 a.m. UTC | #3
Hi,

do we still need this patch after having merged Arne's HMAC feature?

Regards,
Gert Doering June 24, 2022, 10:26 a.m. UTC | #4
Hi,

On Fri, Jun 24, 2022 at 11:13:40AM +0200, Antonio Quartulli wrote:
> do we still need this patch after having merged Arne's HMAC feature?

Yes and no.  

*This* patch won't apply anymore, but Arne said "we're now much faster 
in replying to packets than ever before" so we might indeed need a 
per-source-ip rate-limiter, to something like "10 per 10 seconds" or 
so (inventing arbitrary number that should be more than enough even 
for "5 users behind the same NAT reconnect at the same time", while 
at the same time too low to cause harm as a reflector) for the 
initial reply.

gert
Arne Schwabe June 24, 2022, 11:15 a.m. UTC | #5
Am 24.06.22 um 12:26 schrieb Gert Doering:
> Hi,
> 
> On Fri, Jun 24, 2022 at 11:13:40AM +0200, Antonio Quartulli wrote:
>> do we still need this patch after having merged Arne's HMAC feature?
> 
> Yes and no.
> 
> *This* patch won't apply anymore, but Arne said "we're now much faster
> in replying to packets than ever before" so we might indeed need a
> per-source-ip rate-limiter, to something like "10 per 10 seconds" or
> so (inventing arbitrary number that should be more than enough even
> for "5 users behind the same NAT reconnect at the same time", while
> at the same time too low to cause harm as a reflector) for the
> initial reply.

Yeah. Keeping a per IP table is adding a lot of state to manage that. 
Maybe instead to a (configurable) overall limit like 100/s?

Arne
Gert Doering June 24, 2022, 11:20 a.m. UTC | #6
Hi,

On Fri, Jun 24, 2022 at 01:15:05PM +0200, Arne Schwabe wrote:
> > *This* patch won't apply anymore, but Arne said "we're now much faster
> > in replying to packets than ever before" so we might indeed need a
> > per-source-ip rate-limiter, to something like "10 per 10 seconds" or
> > so (inventing arbitrary number that should be more than enough even
> > for "5 users behind the same NAT reconnect at the same time", while
> > at the same time too low to cause harm as a reflector) for the
> > initial reply.
> 
> Yeah. Keeping a per IP table is adding a lot of state to manage that. 
> Maybe instead to a (configurable) overall limit like 100/s?

This would permit an attacker sending 1000 packets/s to an openvpn server
to drown out 90% of all legitimate connection requests from everyone 
else...

So while I see your argument about "please do not introduce state again,
we just got rid of it", I'm not convinced we can get away so easily ;-)

OTOH, 100/s inside OpenVPN, and good documentation how to offload the
state to iptables...  can we teach iptables to recognize CLIENT_RESET
(or whatever they are) packets - only - and apply per-source rate-limiting
there?   iptables seems to be quite good at doing this for TCP SYNs
already...

gert

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 74b88ce6..3078d76c 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -3468,6 +3468,20 @@  tls_pre_decrypt(struct tls_multi *multi,
                         print_link_socket_actual(from, &gc));
                     goto error;
                 }
+
+                /* only permit one is_hard_reset() packet per 10 seconds,
+                 * ignore more frequent packets
+                 */
+                time_t now = time(NULL);
+                if ( now - multi->last_hard_reset_seen < 10 )
+                {
+                    msg(D_MULTI_ERRORS, "TLS: tls_pre_decrypt: ignore incoming"
+                                        " '%s' (only %ds since last RESET)",
+                        packet_opcode_name(op),
+                        (int)(now - multi->last_hard_reset_seen) );
+                    goto error;
+                }
+                multi->last_hard_reset_seen = now;
             }
 
             /*
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 7bf82b3a..e71696b5 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -515,6 +515,7 @@  struct tls_multi
      */
     int n_hard_errors; /* errors due to TLS negotiation failure */
     int n_soft_errors; /* errors due to unrecognized or failed-to-authenticate incoming packets */
+    time_t last_hard_reset_seen;	/* rate-limit incoming hard reset */
 
     /*
      * Our locked common name, username, and cert hashes (cannot change during the life of this tls_multi object)