Merge branch 'maint-0.4.7'

changes/bug40673

@@ -1,5 +1,5 @@
   o Minor bugfixes (relay overload statistics):
-    - Count total create cells vs dropped rate cells properly, when
+    - Count total create cells vs dropped create cells properly, when
       assessing if our fraction of dropped cells is too high. We only
       count non-client circuits in the denominator, but we would include
       client circuits in the numerator, leading to surprising log lines

changes/ticket40680

@@ -0,0 +1,6 @@
+  o Minor feature (relay, DoS):
+    - Apply circuit creation anti-DoS defenses if the outbound circuit max cell
+      queue size is reached too many times. This introduces two new consensus
+      parameters to control the queue size limit and number of times allowed to
+      go over that limit. Close ticket 40680.
+

src/core/or/dos.c

@@ -49,6 +49,7 @@ static int32_t dos_cc_defense_time_period;
 /* Keep some stats for the heartbeat so we can report out. */
 static uint64_t cc_num_rejected_cells;
 static uint32_t cc_num_marked_addrs;
+static uint32_t cc_num_marked_addrs_max_queue;
 
 /*
  * Concurrent connection denial of service mitigation.
@@ -72,6 +73,10 @@ static int32_t dos_conn_connect_defense_time_period =
 static uint64_t conn_num_addr_rejected;
 static uint64_t conn_num_addr_connect_rejected;
 
+/** Consensus parameter: How many times a client IP is allowed to hit the
+ * circ_max_cell_queue_size_out limit before being marked. */
+static uint32_t dos_num_circ_max_outq;
+
 /*
  * General interface of the denial of service mitigation subsystem.
  */
@@ -79,6 +84,22 @@ static uint64_t conn_num_addr_connect_rejected;
 /* Keep stats for the heartbeat. */
 static uint64_t num_single_hop_client_refused;
 
+/** Return the consensus parameter for the outbound circ_max_cell_queue_size
+ * limit. */
+static uint32_t
+get_param_dos_num_circ_max_outq(const networkstatus_t *ns)
+{
+#define DOS_NUM_CIRC_MAX_OUTQ_DEFAULT 3
+#define DOS_NUM_CIRC_MAX_OUTQ_MIN 0
+#define DOS_NUM_CIRC_MAX_OUTQ_MAX INT32_MAX
+
+  /* Update the circuit max cell queue size from the consensus. */
+  return networkstatus_get_param(ns, "dos_num_circ_max_outq",
+                                 DOS_NUM_CIRC_MAX_OUTQ_DEFAULT,
+                                 DOS_NUM_CIRC_MAX_OUTQ_MIN,
+                                 DOS_NUM_CIRC_MAX_OUTQ_MAX);
+}
+
 /* Return true iff the circuit creation mitigation is enabled. We look at the
  * consensus for this else a default value is returned. */
 MOCK_IMPL(STATIC unsigned int,
@@ -258,6 +279,9 @@ set_dos_parameters(const networkstatus_t *ns)
   dos_conn_connect_burst = get_param_conn_connect_burst(ns);
   dos_conn_connect_defense_time_period =
     get_param_conn_connect_defense_time_period(ns);
+
+  /* Circuit. */
+  dos_num_circ_max_outq = get_param_dos_num_circ_max_outq(ns);
 }
 
 /* Free everything for the circuit creation DoS mitigation subsystem. */
@@ -745,6 +769,69 @@ dos_geoip_entry_init(clientmap_entry_t *geoip_ent)
                         (uint32_t) approx_time());
 }
 
+/** Note that the given channel has sent outbound the maximum amount of cell
+ * allowed on the next channel. */
+void
+dos_note_circ_max_outq(const channel_t *chan)
+{
+  tor_addr_t addr;
+  clientmap_entry_t *entry;
+
+  tor_assert(chan);
+
+  /* Skip everything if circuit creation defense is disabled. */
+  if (!dos_cc_enabled) {
+    goto end;
+  }
+
+  /* Must be a client connection else we ignore. */
+  if (!channel_is_client(chan)) {
+    goto end;
+  }
+  /* Without an IP address, nothing can work. */
+  if (!channel_get_addr_if_possible(chan, &addr)) {
+    goto end;
+  }
+
+  /* We are only interested in client connection from the geoip cache. */
+  entry = geoip_lookup_client(&addr, NULL, GEOIP_CLIENT_CONNECT);
+  if (entry == NULL) {
+    goto end;
+  }
+
+  /* Is the client marked? If yes, just ignore. */
+  if (entry->dos_stats.cc_stats.marked_until_ts >= approx_time()) {
+    goto end;
+  }
+
+  /* If max outq parameter is 0, it means disabled, just ignore. */
+  if (dos_num_circ_max_outq == 0) {
+    goto end;
+  }
+
+  entry->dos_stats.num_circ_max_cell_queue_size++;
+
+  /* This is the detection. If we have reached the maximum amount of times a
+   * client IP is allowed to reach this limit, mark client. */
+  if (entry->dos_stats.num_circ_max_cell_queue_size >=
+      dos_num_circ_max_outq) {
+    /* Only account for this marked address if this is the first time we block
+     * it else our counter is inflated with non unique entries. */
+    if (entry->dos_stats.cc_stats.marked_until_ts == 0) {
+      cc_num_marked_addrs_max_queue++;
+    }
+    log_info(LD_DOS, "Detected outbound max circuit queue from addr: %s",
+             fmt_addr(&addr));
+    cc_mark_client(&entry->dos_stats.cc_stats);
+
+    /* Reset after being marked so once unmarked, we start back clean. */
+    entry->dos_stats.num_circ_max_cell_queue_size = 0;
+  }
+
+ end:
+  return;
+}
+
 /* Note down that we've just refused a single hop client. This increments a
  * counter later used for the heartbeat. */
 void
@@ -786,8 +873,10 @@ dos_log_heartbeat(void)
   if (dos_cc_enabled) {
     smartlist_add_asprintf(elems,
                            "%" PRIu64 " circuits rejected, "
-                           "%" PRIu32 " marked addresses",
-                           cc_num_rejected_cells, cc_num_marked_addrs);
+                           "%" PRIu32 " marked addresses, "
+                           "%" PRIu32 " marked addresses for max queue",
+                           cc_num_rejected_cells, cc_num_marked_addrs,
+                           cc_num_marked_addrs_max_queue);
   } else {
     smartlist_add_asprintf(elems, "[DoSCircuitCreationEnabled disabled]");
   }

src/core/or/dos.h

@@ -58,6 +58,9 @@ typedef struct dos_client_stats_t {
   /* Circuit creation statistics. This is only used if the circuit creation
    * subsystem has been enabled (dos_cc_enabled). */
   cc_client_stats_t cc_stats;
+
+  /** Number of times the circ_max_cell_queue_size limit has been reached. */
+  uint32_t num_circ_max_cell_queue_size;
 } dos_client_stats_t;
 
 /* General API. */
@@ -79,6 +82,7 @@ void dos_close_client_conn(const or_connection_t *or_conn);
 
 int dos_should_refuse_single_hop_client(void);
 void dos_note_refuse_single_hop_client(void);
+void dos_note_circ_max_outq(const channel_t *chan);
 
 /*
  * Circuit creation DoS mitigation subsystemn interface.

src/core/or/relay.c

@@ -3147,6 +3147,9 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max))
 }
 
 /* Minimum value is the maximum circuit window size.
+ *
+ * This value is set to a lower bound we believe is reasonable with congestion
+ * control and basic network tunning parameters.
  *
  * SENDME cells makes it that we can control how many cells can be inflight on
  * a circuit from end to end. This logic makes it that on any circuit cell
@@ -3170,12 +3173,12 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max))
  * DoS memory pressure so the default size is a middle ground between not
  * having any limit and having a very restricted one. This is why we can also
  * control it through a consensus parameter. */
-#define RELAY_CIRC_CELL_QUEUE_SIZE_MIN CIRCWINDOW_START_MAX
+#define RELAY_CIRC_CELL_QUEUE_SIZE_MIN 50
 /* We can't have a consensus parameter above this value. */
 #define RELAY_CIRC_CELL_QUEUE_SIZE_MAX INT32_MAX
 /* Default value is set to a large value so we can handle padding cells
- * properly which aren't accounted for in the SENDME window. Default is 50000
- * allowed cells in the queue resulting in ~25MB. */
+ * properly which aren't accounted for in the SENDME window. Default is 2500
+ * allowed cells in the queue resulting in ~1MB. */
 #define RELAY_CIRC_CELL_QUEUE_SIZE_DEFAULT \
   (50 * RELAY_CIRC_CELL_QUEUE_SIZE_MIN)
 
@@ -3183,6 +3186,33 @@ channel_flush_from_first_active_circuit, (channel_t *chan, int max))
  * every new consensus and controlled by a parameter. */
 static int32_t max_circuit_cell_queue_size =
   RELAY_CIRC_CELL_QUEUE_SIZE_DEFAULT;
+/** Maximum number of cell on an outbound circuit queue. This is updated at
+ * every new consensus and controlled by a parameter. This default is incorrect
+ * and won't be used at all except in unit tests. */
+static int32_t max_circuit_cell_queue_size_out =
+  RELAY_CIRC_CELL_QUEUE_SIZE_DEFAULT;
+
+/** Return consensus parameter "circ_max_cell_queue_size". The given ns can be
+ * NULL. */
+static uint32_t
+get_param_max_circuit_cell_queue_size(const networkstatus_t *ns)
+{
+  return networkstatus_get_param(ns, "circ_max_cell_queue_size",
+                                 RELAY_CIRC_CELL_QUEUE_SIZE_DEFAULT,
+                                 RELAY_CIRC_CELL_QUEUE_SIZE_MIN,
+                                 RELAY_CIRC_CELL_QUEUE_SIZE_MAX);
+}
+
+/** Return consensus parameter "circ_max_cell_queue_size_out". The given ns can
+ * be NULL. */
+static uint32_t
+get_param_max_circuit_cell_queue_size_out(const networkstatus_t *ns)
+{
+  return networkstatus_get_param(ns, "circ_max_cell_queue_size_out",
+                                 get_param_max_circuit_cell_queue_size(ns),
+                                 RELAY_CIRC_CELL_QUEUE_SIZE_MIN,
+                                 RELAY_CIRC_CELL_QUEUE_SIZE_MAX);
+}
 
 /* Called when the consensus has changed. At this stage, the global consensus
  * object has NOT been updated. It is called from
@@ -3194,10 +3224,9 @@ relay_consensus_has_changed(const networkstatus_t *ns)
 
   /* Update the circuit max cell queue size from the consensus. */
   max_circuit_cell_queue_size =
-    networkstatus_get_param(ns, "circ_max_cell_queue_size",
-                            RELAY_CIRC_CELL_QUEUE_SIZE_DEFAULT,
-                            RELAY_CIRC_CELL_QUEUE_SIZE_MIN,
-                            RELAY_CIRC_CELL_QUEUE_SIZE_MAX);
+    get_param_max_circuit_cell_queue_size(ns);
+  max_circuit_cell_queue_size_out =
+    get_param_max_circuit_cell_queue_size_out(ns);
 }
 
 /** Add <b>cell</b> to the queue of <b>circ</b> writing to <b>chan</b>
@@ -3214,6 +3243,7 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
 {
   or_circuit_t *orcirc = NULL;
   cell_queue_t *queue;
+  int32_t max_queue_size;
   int streams_blocked;
   int exitward;
   if (circ->marked_for_close)
@@ -3223,13 +3253,21 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan,
   if (exitward) {
     queue = &circ->n_chan_cells;
     streams_blocked = circ->streams_blocked_on_n_chan;
+    max_queue_size = max_circuit_cell_queue_size_out;
   } else {
     orcirc = TO_OR_CIRCUIT(circ);
     queue = &orcirc->p_chan_cells;
     streams_blocked = circ->streams_blocked_on_p_chan;
+    max_queue_size = max_circuit_cell_queue_size;
   }
 
-  if (PREDICT_UNLIKELY(queue->n >= max_circuit_cell_queue_size)) {
+  if (PREDICT_UNLIKELY(queue->n >= max_queue_size)) {
+    /* This DoS defense only applies at the Guard as in the p_chan is likely
+     * a client IP attacking the network. */
+    if (exitward && CIRCUIT_IS_ORCIRC(circ)) {
+      dos_note_circ_max_outq(CONST_TO_OR_CIRCUIT(circ)->p_chan);
+    }
+
     log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
            "%s circuit has %d cells in its queue, maximum allowed is %d. "
            "Closing circuit for safety reasons.",

src/core/or/relay.h

@@ -17,6 +17,8 @@ extern uint64_t stats_n_relay_cells_delivered;
 extern uint64_t stats_n_circ_max_cell_reached;
 
 void relay_consensus_has_changed(const networkstatus_t *ns);
+uint32_t relay_get_param_max_circuit_cell_queue_size(
+                                     const networkstatus_t *ns);
 int circuit_receive_relay_cell(cell_t *cell, circuit_t *circ,
                                cell_direction_t cell_direction);
 size_t cell_queues_get_total_allocation(void);