| Message ID | 20181206171033.61310-1-gert@greenie.muc.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [Openvpn-devel] Rate-limit incoming P_CONTROL_HARD_RESET_* packets. | expand |
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
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 >
Hi, do we still need this patch after having merged Arne's HMAC feature? Regards,
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
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
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
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(+)