From 628d52e21f9dbca5bf9ae4aca494b66fcf5ea0e6 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 3 Aug 2022 09:21:57 -0400 Subject: [PATCH] relay: Keep geoip cache entry if connection attempts are seen Fixes #40637 Signed-off-by: David Goulet --- changes/ticket40637 | 4 ++++ src/core/mainloop/connection.c | 2 ++ src/core/or/dos.h | 6 ++++++ src/feature/stats/geoip_stats.c | 38 +++++++++++++++++++++++++++++---- src/feature/stats/geoip_stats.h | 1 + 5 files changed, 47 insertions(+), 4 deletions(-) create mode 100644 changes/ticket40637 diff --git a/changes/ticket40637 b/changes/ticket40637 new file mode 100644 index 0000000000..5d88b6f980 --- /dev/null +++ b/changes/ticket40637 @@ -0,0 +1,4 @@ + o Minor bugfixes (relay, DoS): + - Keep geoip entries from being deleted if the address is still trying to + connect but the DoS subsystem refuses them. Fixes bug 40637; bugfix on + 0.4.6.1-alpha. diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c index 8bb3534b28..9061284407 100644 --- a/src/core/mainloop/connection.c +++ b/src/core/mainloop/connection.c @@ -110,6 +110,7 @@ #include "feature/stats/connstats.h" #include "feature/stats/rephist.h" #include "feature/stats/bwhist.h" +#include "feature/stats/geoip_stats.h" #include "lib/crypt_ops/crypto_util.h" #include "lib/crypt_ops/crypto_format.h" #include "lib/geoip/geoip.h" @@ -2031,6 +2032,7 @@ connection_handle_listener_read(connection_t *conn, int new_type) if (new_type == CONN_TYPE_OR) { /* Assess with the connection DoS mitigation subsystem if this address * can open a new connection. */ + geoip_note_client_attempt(&addr, approx_time()); if (dos_conn_addr_get_defense_type(&addr) == DOS_CONN_DEFENSE_CLOSE) { rep_hist_note_conn_rejected(new_type); tor_close_socket(news); diff --git a/src/core/or/dos.h b/src/core/or/dos.h index 6dcfa3cb94..7be57b691b 100644 --- a/src/core/or/dos.h +++ b/src/core/or/dos.h @@ -46,6 +46,12 @@ typedef struct conn_client_stats_t { * rules, and thus is marked so defense(s) can be applied. It is * synchronized using the approx_time(). */ time_t marked_until_ts; + + /* Denotes when was the last connection attempt (successful or not). We use + * this to decide if we can keep or purge the stats entry in our cache. We + * can't rely on the "last_seen_in_minutes" because that is set when a + * successful channel is opened meaning TCP connect and Tor negotiation. */ + time_t last_attempt; } conn_client_stats_t; /* This object is a top level object that contains everything related to the diff --git a/src/feature/stats/geoip_stats.c b/src/feature/stats/geoip_stats.c index a0fe8597c1..3b33f15c69 100644 --- a/src/feature/stats/geoip_stats.c +++ b/src/feature/stats/geoip_stats.c @@ -222,6 +222,32 @@ client_history_clear(void) } } +/** Note that an OR connection attempt. This is called basically when a TCP + * connection comes in but has not completed the Tor negotiation. We used this + * information to keep entries in the geoip cache for the DoS subsystem. + * + * This won't find any bridge transport connections because at this stage, we + * don't know the transport name if any. And so, only applies to naked OR + * connections. */ +void +geoip_note_client_attempt(const tor_addr_t *addr, time_t now) +{ + /* This is only useful to the DoS subsystem so ignore if not enabled. */ + if (!dos_enabled()) { + return; + } + + /* We lookup if this address/transport name has already connected + * successfully and if so, we will not this attempt time. Else, we do not add + * an entry on an attempt so to not clobber the cache with unsuccessful + * connections. */ + clientmap_entry_t *ent = geoip_lookup_client(addr, NULL, + GEOIP_CLIENT_CONNECT); + if (ent) { + ent->dos_stats.conn_stats.last_attempt = now; + } +} + /** Note that we've seen a client connect from the IP addr * at time now. Ignored by all but bridges and directories if * configured accordingly. */ @@ -281,11 +307,15 @@ remove_old_client_helper_(struct clientmap_entry_t *ent, void *_cutoff) { time_t cutoff = *(time_t*)_cutoff / 60; if (ent->last_seen_in_minutes < cutoff) { - clientmap_entry_free(ent); - return 1; - } else { - return 0; + /* If the DoS subsystem is disabled, this is an automatic clean up. If it + * is enabled, check if the last attempt is below cutoff else keep it + * around. */ + if (!dos_enabled() || ent->dos_stats.conn_stats.last_attempt < cutoff) { + clientmap_entry_free(ent); + return 1; + } } + return 0; } /** Forget about all clients that haven't connected since cutoff. */ diff --git a/src/feature/stats/geoip_stats.h b/src/feature/stats/geoip_stats.h index b54304337a..220cc3c6da 100644 --- a/src/feature/stats/geoip_stats.h +++ b/src/feature/stats/geoip_stats.h @@ -97,6 +97,7 @@ typedef struct clientmap_entry_t { int should_record_bridge_info(const or_options_t *options); +void geoip_note_client_attempt(const tor_addr_t *addr, time_t now); void geoip_note_client_seen(geoip_client_action_t action, const tor_addr_t *addr, const char *transport_name, time_t now);