Merge remote-tracking branch 'dgoulet/ticket24902_029_05'

4 files changed, 47 insertions, 14 deletions

diff --git a/changes/bug25223 b/changes/bug25223
new file mode 100644
index 0000000000..fdd5563500
--- /dev/null
+++ b/changes/bug25223
@@ -0,0 +1,4 @@
+  o Minor bugfixes (DoS mitigation):
+    - Make sure we don't modify consensus parameters if we aren't a public
+      relay when a new consensus arrives. Fixes bug 25223; bugfix on
+      0.3.3.2-alpha.
diff --git a/changes/ticket25202 b/changes/ticket25202
new file mode 100644
index 0000000000..ba64abad7b
--- /dev/null
+++ b/changes/ticket25202
@@ -0,0 +1,4 @@
+  o Minor bugfixes (DoS mitigation):
+    - Add extra safety checks when refilling the circuit creation bucket to
+      ensure we never set a value that is above the allowed burst. Fixes
+      bug 25202; bugfix on 0.3.3.2-alpha.
diff --git a/doc/tor.1.txt b/doc/tor.1.txt
index 9a4e70c769..2c8135ff05 100644
--- a/doc/tor.1.txt
+++ b/doc/tor.1.txt
@@ -2765,7 +2765,7 @@ Denial of Service mitigation subsystem.
     address is positively identified, tor will activate defenses against the
     address. See the DoSCircuitCreationDefenseType option for more details.
     This is a client to relay detection only. "auto" means use the consensus
-    parameter.
+    parameter. If not defined in the consensus, the value is 0.
     (Default: auto)
 
 [[DoSCircuitCreationMinConnections]] **DoSCircuitCreationMinConnections** __NUM__::
@@ -2774,19 +2774,22 @@ Denial of Service mitigation subsystem.
     flagged as executing a circuit creation DoS. In other words, once a client
     address reaches the circuit rate and has a minimum of NUM concurrent
     connections, a detection is positive. "0" means use the consensus
-    parameter.
+    parameter. If not defined in the consensus, the value is 3.
     (Default: 0)
 
 [[DoSCircuitCreationRate]] **DoSCircuitCreationRate** __NUM__::
 
     The allowed circuit creation rate per second applied per client IP
-    address. If this option is 0, it obeys a consensus parameter. (Default: 0)
+    address. If this option is 0, it obeys a consensus parameter. If not
+    defined in the consensus, the value is 3.
+    (Default: 0)
 
 [[DoSCircuitCreationBurst]] **DoSCircuitCreationBurst** __NUM__::
 
     The allowed circuit creation burst per client IP address. If the circuit
     rate and the burst are reached, a client is marked as executing a circuit
-    creation DoS. "0" means use the consensus parameter.
+    creation DoS. "0" means use the consensus parameter. If not defined in the
+    consensus, the value is 90.
     (Default: 0)
 
 [[DoSCircuitCreationDefenseType]] **DoSCircuitCreationDefenseType** __NUM__::
@@ -2797,28 +2800,31 @@ Denial of Service mitigation subsystem.
       1: No defense.
       2: Refuse circuit creation for the DoSCircuitCreationDefenseTimePeriod period of time.
 +
-    "0" means use the consensus parameter.
+    "0" means use the consensus parameter. If not defined in the consensus,
+    the value is 2.
     (Default: 0)
 
-[[DoSCircuitCreationDefenseTimePeriod]] **DoSCircuitCreationDefenseTimePeriod** __NUM__::
+[[DoSCircuitCreationDefenseTimePeriod]] **DoSCircuitCreationDefenseTimePeriod** __N__ **seconds**|**minutes**|**hours**::
 
-    The base time period that the DoS defense is activated for. The actual
-    value is selected randomly for each activation from NUM+1 to 3/2 * NUM.
-    "0" means use the consensus parameter.
-    (Default: 0)
+    The base time period in seconds that the DoS defense is activated for. The
+    actual value is selected randomly for each activation from N+1 to 3/2 * N.
+    "0" means use the consensus parameter. If not defined in the consensus,
+    the value is 3600 seconds (1 hour).  (Default: 0)
 
 [[DoSConnectionEnabled]] **DoSConnectionEnabled** **0**|**1**|**auto**::
 
     Enable the connection DoS mitigation. For client address only, this allows
     tor to mitigate against large number of concurrent connections made by a
-    single IP address. "auto" means use the consensus parameter.
+    single IP address. "auto" means use the consensus parameter. If not
+    defined in the consensus, the value is 0.
     (Default: auto)
 
 [[DoSConnectionMaxConcurrentCount]] **DoSConnectionMaxConcurrentCount** __NUM__::
 
     The maximum threshold of concurrent connection from a client IP address.
     Above this limit, a defense selected by DoSConnectionDefenseType is
-    applied. "0" means use the consensus parameter.
+    applied. "0" means use the consensus parameter. If not defined in the
+    consensus, the value is 100.
     (Default: 0)
 
 [[DoSConnectionDefenseType]] **DoSConnectionDefenseType** __NUM__::
@@ -2829,7 +2835,8 @@ Denial of Service mitigation subsystem.
       1: No defense.
       2: Immediately close new connections.
 +
-    "0" means use the consensus parameter.
+    "0" means use the consensus parameter. If not defined in the consensus,
+    the value is 2.
     (Default: 0)
 
 [[DoSRefuseSingleHopClientRendezvous]] **DoSRefuseSingleHopClientRendezvous** **0**|**1**|**auto**::
@@ -2837,7 +2844,7 @@ Denial of Service mitigation subsystem.
     Refuse establishment of rendezvous points for single hop clients. In other
     words, if a client directly connects to the relay and sends an
     ESTABLISH_RENDEZVOUS cell, it is silently dropped. "auto" means use the
-    consensus parameter.
+    consensus parameter. If not defined in the consensus, the value is 0.
     (Default: auto)
 
 TESTING NETWORK OPTIONS
diff --git a/src/or/dos.c b/src/or/dos.c
index 9e8a7a9abe..4d1797eece 100644
--- a/src/or/dos.c
+++ b/src/or/dos.c
@@ -309,6 +309,16 @@ cc_stats_refill_bucket(cc_client_stats_t *stats, const tor_addr_t *addr)
     new_circuit_bucket_count = MIN(stats->circuit_bucket + (uint32_t)num_token,
                                    dos_cc_circuit_burst);
   }
+
+  /* This function is not allowed to make the bucket count larger than the
+   * burst value */
+  tor_assert_nonfatal(new_circuit_bucket_count <= dos_cc_circuit_burst);
+  /* This function is not allowed to make the bucket count smaller, unless it
+   * is decreasing it to a newly configured, lower burst value. We allow the
+   * bucket to stay the same size, in case the circuit rate is zero. */
+  tor_assert_nonfatal(new_circuit_bucket_count >= stats->circuit_bucket ||
+                      new_circuit_bucket_count == dos_cc_circuit_burst);
+
   log_debug(LD_DOS, "DoS address %s has its circuit bucket value: %" PRIu32
                     ". Filling it to %" PRIu32 ". Circuit rate is %" PRIu64
                     ". Elapsed time is %" PRIi64,
@@ -738,6 +748,14 @@ dos_close_client_conn(const or_connection_t *or_conn)
 void
 dos_consensus_has_changed(const networkstatus_t *ns)
 {
+  /* There are two ways to configure this subsystem, one at startup through
+   * dos_init() which is called when the options are parsed. And this one
+   * through the consensus. We don't want to enable any DoS mitigation if we
+   * aren't a public relay. */
+  if (!public_server_mode(get_options())) {
+    return;
+  }
+
   cc_consensus_has_changed(ns);
   conn_consensus_has_changed(ns);