[tor-commits] [tor/master] hs-v3: Move DoS parameter check against 0

[nickm at torproject.org]

commit 385f6bcfccbc327f42e5139ac8136086e79fbb17
Author: David Goulet <dgoulet at torproject.org>
Date:   Tue Aug 20 10:50:31 2019 -0400

    hs-v3: Move DoS parameter check against 0
    
    Move it outside of the validation function since 0 is a valid value but
    disables defenses.
    
    Signed-off-by: David Goulet <dgoulet at torproject.org>
---
 src/feature/hs/hs_intropoint.c | 54 +++++++++++++++++++++++++++++-------------
 src/test/test_hs_dos.c         | 11 ++++-----
 2 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/src/feature/hs/hs_intropoint.c b/src/feature/hs/hs_intropoint.c
index 9b6a96628..fb2ac52e5 100644
--- a/src/feature/hs/hs_intropoint.c
+++ b/src/feature/hs/hs_intropoint.c
@@ -191,28 +191,40 @@ validate_cell_dos_extension_parameters(uint64_t intro2_rate_per_sec,
 {
   bool ret = false;
 
-  /* A value of 0 is valid in the sense that we accept it but we still disable
-   * the defenses so return false. */
-  if (intro2_rate_per_sec == 0 || intro2_burst_per_sec == 0) {
-    log_info(LD_REND, "Intro point DoS defenses parameter set to 0.");
+  /* Check that received value is not below the minimum. Don't check if minimum
+     is set to 0, since the param is a positive value and gcc will complain. */
+#if HS_CONFIG_V3_DOS_DEFENSE_RATE_PER_SEC_MIN > 0
+  if (intro2_rate_per_sec < HS_CONFIG_V3_DOS_DEFENSE_RATE_PER_SEC_MIN) {
+    log_fn(LOG_PROTOCOL_WARN, LD_REND,
+           "Intro point DoS defenses rate per second is "
+           "too small. Received value: %" PRIu64, intro2_rate_per_sec);
     goto end;
   }
+#endif
 
-  /* Bound check the received rate per second. MIN/MAX are inclusive. */
-  if (!(intro2_rate_per_sec <= HS_CONFIG_V3_DOS_DEFENSE_RATE_PER_SEC_MAX &&
-        intro2_rate_per_sec > HS_CONFIG_V3_DOS_DEFENSE_RATE_PER_SEC_MIN)) {
-    log_info(LD_REND, "Intro point DoS defenses rate per second is "
-                      "invalid. Received value: %" PRIu64,
-             intro2_rate_per_sec);
+  /* Check that received value is not above maximum */
+  if (intro2_rate_per_sec > HS_CONFIG_V3_DOS_DEFENSE_RATE_PER_SEC_MAX) {
+    log_fn(LOG_PROTOCOL_WARN, LD_REND,
+           "Intro point DoS defenses rate per second is "
+           "too big. Received value: %" PRIu64, intro2_rate_per_sec);
+    goto end;
+  }
+
+  /* Check that received value is not below the minimum */
+#if HS_CONFIG_V3_DOS_DEFENSE_BURST_PER_SEC_MIN > 0
+  if (intro2_burst_per_sec < HS_CONFIG_V3_DOS_DEFENSE_BURST_PER_SEC_MIN) {
+    log_fn(LOG_PROTOCOL_WARN, LD_REND,
+           "Intro point DoS defenses burst per second is "
+           "too small. Received value: %" PRIu64, intro2_burst_per_sec);
     goto end;
   }
+#endif
 
-  /* Bound check the received burst per second. MIN/MAX are inclusive. */
-  if (!(intro2_burst_per_sec <= HS_CONFIG_V3_DOS_DEFENSE_BURST_PER_SEC_MAX &&
-        intro2_burst_per_sec > HS_CONFIG_V3_DOS_DEFENSE_BURST_PER_SEC_MIN)) {
-    log_info(LD_REND, "Intro point DoS defenses burst per second is "
-                      "invalid. Received value: %" PRIu64,
-             intro2_burst_per_sec);
+  /* Check that received value is not above maximum */
+  if (intro2_burst_per_sec > HS_CONFIG_V3_DOS_DEFENSE_BURST_PER_SEC_MAX) {
+    log_fn(LOG_PROTOCOL_WARN, LD_REND,
+           "Intro point DoS defenses burst per second is "
+           "too big. Received value: %" PRIu64, intro2_burst_per_sec);
     goto end;
   }
 
@@ -273,6 +285,16 @@ handle_establish_intro_cell_dos_extension(
     }
   }
 
+  /* A value of 0 is valid in the sense that we accept it but we still disable
+   * the defenses so return false. */
+  if (intro2_rate_per_sec == 0 || intro2_burst_per_sec == 0) {
+    log_info(LD_REND, "Intro point DoS defenses parameter set to 0. "
+                      "Disabling INTRO2 DoS defenses on circuit id %u",
+             circ->p_circ_id);
+    circ->introduce2_dos_defense_enabled = 0;
+    goto end;
+  }
+
   /* If invalid, we disable the defense on the circuit. */
   if (!validate_cell_dos_extension_parameters(intro2_rate_per_sec,
                                               intro2_burst_per_sec)) {
diff --git a/src/test/test_hs_dos.c b/src/test/test_hs_dos.c
index 25a04d779..03c755acb 100644
--- a/src/test/test_hs_dos.c
+++ b/src/test/test_hs_dos.c
@@ -143,6 +143,8 @@ test_validate_dos_extension_params(void *arg)
   /* Valid custom rate/burst. */
   ret = validate_cell_dos_extension_parameters(17, 42);
   tt_assert(ret);
+  ret = cell_dos_extension_parameters_are_valid(INT32_MAX, INT32_MAX);
+  tt_assert(ret);
 
   /* Invalid rate. */
   ret = validate_cell_dos_extension_parameters(UINT64_MAX, 42);
@@ -152,11 +154,9 @@ test_validate_dos_extension_params(void *arg)
   ret = validate_cell_dos_extension_parameters(42, UINT64_MAX);
   tt_assert(!ret);
 
-  /* Value of 0 should return invalid so defenses can be disabled. */
-  ret = validate_cell_dos_extension_parameters(0, 42);
-  tt_assert(!ret);
-  ret = validate_cell_dos_extension_parameters(42, 0);
-  tt_assert(!ret);
+  /* Value of 0 is valid (but should disable defenses) */
+  ret = cell_dos_extension_parameters_are_valid(0, 0);
+  tt_assert(ret);
 
   /* Can't have burst smaller than rate. */
   ret = validate_cell_dos_extension_parameters(42, 40);
@@ -174,4 +174,3 @@ struct testcase_t hs_dos_tests[] = {
 
   END_OF_TESTCASES
 };
-