Rate-limit incoming P_CONTROL_HARD_RESET_* packets.

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 <[email protected]...>

---
 src/openvpn/ssl.c        | 14 ++++++++++++++
 src/openvpn/ssl_common.h |  1 +
 2 files changed, 15 insertions(+)

src/openvpn/ssl.c +0 -0

index 74b88ce6..3078d76c 100644

src/openvpn/ssl.c +3 -0

+                    goto error;
+                }
+                multi->last_hard_reset_seen = now;
             }
 
             /*

src/openvpn/ssl_common.h +0 -0

index 7bf82b3a..e71696b5 100644

Hi,

Quoted 5 lines · Gert Doering

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

Quoted 5 lines

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?

Quoted 11 lines

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

---
 src/openvpn/ssl.c        | 14 ++++++++++++++
 src/openvpn/ssl_common.h |  1 +
 2 files changed, 15 insertions(+)

src/openvpn/ssl.c +0 -0

index 74b88ce6..3078d76c 100644

src/openvpn/ssl.c tls_pre_decrypt(struct tls_multi *multi, · line 3476

Quoted 9 lines

src/openvpn/ssl.c +6 -0

@@ -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.

src/openvpn/ssl.c tls_pre_decrypt(struct tls_multi *multi, · line 3481

Quoted 5 lines

src/openvpn/ssl.c +5 -0

+                {
+                    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 ).

Quoted 12 lines

src/openvpn/ssl.c +3 -0

+                    goto error;
+                }
+                multi->last_hard_reset_seen = now;
             }
 
             /*

src/openvpn/ssl_common.h +0 -0

index 7bf82b3a..e71696b5 100644

src/openvpn/ssl_common.h struct tls_multi · line 521

Quoted 7 lines

src/openvpn/ssl_common.h +1 -0

@@ -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
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

Hi,

Quoted 1 line · Antonio Quartulli

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.

Quoted 7 lines

*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?