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 <g...@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
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
