[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.
Related show

Commit Message

Gert Doering Dec. 6, 2018, 5:10 p.m.
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. | #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

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)