| Message ID | 20181206171033.61310-1-gert@greenie.muc.de |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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
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)
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(+)