Merge branch 'ticket40637_047_01' into 'main'

relay: Keep geoip cache entry if connection attempts are seen

Closes #40637

See merge request tpo/core/tor!609

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.

changes/ticket40918

@@ -0,0 +1,3 @@
+  o Minor bugfix (relay, sandbox):
+    - Disable a sandbox unit test that is failing on Debian Sid breaking our
+      nightly packages. Fixes bug 40918; bugfix on 0.3.5.1-alpha.

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"
@@ -2027,6 +2028,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, conn->socket_family);
         tor_close_socket(news);

src/core/or/dos.c

@@ -905,8 +905,7 @@ dos_geoip_entry_about_to_free(const clientmap_entry_t *geoip_ent)
   SMARTLIST_FOREACH_BEGIN(get_connection_array(), connection_t *, conn) {
     if (conn->type == CONN_TYPE_OR) {
       or_connection_t *or_conn = TO_OR_CONN(conn);
-      if (!tor_addr_compare(&geoip_ent->addr, &TO_CONN(or_conn)->addr,
+      if (!tor_addr_compare(&geoip_ent->addr, &conn->addr, CMP_EXACT)) {
-                            CMP_EXACT)) {
         or_conn->tracked_for_dos_mitigation = 0;
       }
     }
@@ -916,6 +915,37 @@ dos_geoip_entry_about_to_free(const clientmap_entry_t *geoip_ent)
   return;
 }
 
+/** Return true iff we think there are established connections from <b>ent</b>
+ * at this moment. Else return false if there aren't any. Used for deciding
+ * whether it's time to free a clientmap stats entry. */
+bool
+dos_geoip_any_existing_conns_on_clientmap(struct clientmap_entry_t *ent)
+{
+  if (!dos_enabled() || !ent->dos_stats.conn_stats.concurrent_count) {
+    goto end; /* all set to clean+free it */
+  }
+
+  /* We could just return 1 here, because concurrent_count is non-zero.
+   * But for bullet-proofing we do the expensive checks below. */
+  SMARTLIST_FOREACH_BEGIN(get_connection_array(), const connection_t *, conn) {
+    if (conn->type == CONN_TYPE_OR) {
+      const or_connection_t *or_conn = CONST_TO_OR_CONN(conn);
+      if (tor_addr_eq(&ent->addr, &conn->addr) &&
+          or_conn->tracked_for_dos_mitigation) {
+        return true;
+      }
+    }
+  } SMARTLIST_FOREACH_END(conn);
+
+  /* If we get here, it means we have a bug: concurrent_count was non-zero
+   * but we couldn't find any conns that were being tracked! */
+  log_warn(LD_BUG, "When cleaning clientmap, we expected %d conns "
+                   "but we found none.",
+           ent->dos_stats.conn_stats.concurrent_count);
+ end:
+  return false;
+}
+
 /** A new geoip client entry has been allocated, initialize its DoS object. */
 void
 dos_geoip_entry_init(clientmap_entry_t *geoip_ent)

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_in_minutes;
 } conn_client_stats_t;
 
 /* This object is a top level object that contains everything related to the
@@ -75,6 +81,7 @@ int dos_enabled(void);
 void dos_log_heartbeat(void);
 void dos_geoip_entry_init(struct clientmap_entry_t *geoip_ent);
 void dos_geoip_entry_about_to_free(const struct clientmap_entry_t *geoip_ent);
+bool dos_geoip_any_existing_conns_on_clientmap(struct clientmap_entry_t *ent);
 
 void dos_new_client_conn(or_connection_t *or_conn,
                          const char *transport_name);

src/feature/stats/geoip_stats.c

@@ -114,7 +114,8 @@ should_record_bridge_info(const or_options_t *options)
   return options->BridgeRelay && options->BridgeRecordUsageByCountry;
 }
 
-/** Largest allowable value for last_seen_in_minutes.  (It's a 30-bit field,
+/** Largest allowable value for last_seen_in_minutes or for
+ * last_attempt_in_minutes.  (It's a 30-bit field,
  * so it can hold up to (1u<<30)-1, or 0x3fffffffu.
  */
 #define MAX_LAST_SEEN_IN_MINUTES 0X3FFFFFFFu
@@ -222,6 +223,37 @@ client_history_clear(void)
   }
 }
 
+/** Note an OR connection attempt. This is called when a TCP connection
+ * comes in but has not yet completed the Tor negotiation. We use 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, it 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) {
+    time_t now_in_min = now / 60;
+    if (now_in_min <= MAX_LAST_SEEN_IN_MINUTES && now >= 0) {
+      ent->dos_stats.conn_stats.last_attempt_in_minutes = now_in_min;
+    } else {
+      ent->dos_stats.conn_stats.last_attempt_in_minutes = 0;
+    }
+  }
+}
+
 /** Note that we've seen a client connect from the IP <b>addr</b>
  * at time <b>now</b>. Ignored by all but bridges and directories if
  * configured accordingly. */
@@ -274,18 +306,43 @@ geoip_note_client_seen(geoip_client_action_t action,
   }
 }
 
+/** Decide whether a clientmap_entry_t from the hashtable is still fresh
++ * enough to use in our stats. It is fresh if we last saw a connection
++ * from it (or a connection attempt, if we recorded one of those)
++ * since cutoff.
++ */
+static int
+client_entry_is_still_fresh(struct clientmap_entry_t *ent,
+                            time_t cutoff_in_seconds)
+{
+  time_t cutoff_in_minutes = cutoff_in_seconds / 60;
+  if (ent->last_seen_in_minutes < cutoff_in_minutes) {
+    /* 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_in_minutes <
+          cutoff_in_minutes) {
+      return 0;
+    }
+  }
+  return 1;
+}
+
 /** HT_FOREACH helper: remove a clientmap_entry_t from the hashtable if it's
- * older than a certain time. */
+ * older than a certain time and also if there are no existing connections
+ * open from it. We allow non-fresh entries in the clientmap (so we can keep
+ * an accurate total count of open conns for the dos subsystem) and we skip
+ * over the non-fresh ones when we're exporting our stats. */
 static int
 remove_old_client_helper_(struct clientmap_entry_t *ent, void *_cutoff)
 {
-  time_t cutoff = *(time_t*)_cutoff / 60;
+  if (!client_entry_is_still_fresh(ent, *(time_t*)_cutoff) &&
-  if (ent->last_seen_in_minutes < cutoff) {
+      !dos_geoip_any_existing_conns_on_clientmap(ent)) {
     clientmap_entry_free(ent);
     return 1;
-  } else {
-    return 0;
   }
+  return 0;
 }
 
 /** Forget about all clients that haven't connected since <b>cutoff</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);

src/test/test_sandbox.c

@@ -377,9 +377,19 @@ struct testcase_t sandbox_tests[] = {
 #else
   SANDBOX_TEST_IN_SANDBOX(open_filename),
   SANDBOX_TEST_IN_SANDBOX(openat_filename),
-  SANDBOX_TEST_IN_SANDBOX(opendir_dirname),
 #endif /* defined(ENABLE_FRAGILE_HARDENING) */
 
+  /* Ok why... Quick answer is #40918. This has been failing on Debian SID
+   * making us unable to have nightly packages which is a problem as we have
+   * several relay operators using them and actively reporting us issues with
+   * them. This test fails due to the sandbox denying it.
+   *
+   * We are deprecating C-tor slowly and honestly, the Sandbox feature has
+   * always been a source of pain and unhappiness. Disable this as finding why,
+   * fixing it and hoping it doesn't come back will mostly be a waste of our
+   * time at this point. */
+  SANDBOX_TEST_SKIPPED(opendir_dirname),
+
   SANDBOX_TEST_IN_SANDBOX(chmod_filename),
   SANDBOX_TEST_IN_SANDBOX(chown_filename),
   SANDBOX_TEST_IN_SANDBOX(rename_filename),