sendme: Always pop last SENDME digest from circuit

We must not accumulate digests on the circuit if the other end point is using
another SENDME version that is not using those digests like v0.

This commit makes it that we always pop the digest regardless of the version.

Part of #30428

Signed-off-by: David Goulet <dgoulet@torproject.org>

src/core/or/sendme.c

@@ -47,47 +47,46 @@ get_accept_min_version(void)
                                  SENDME_ACCEPT_MIN_VERSION_MAX);
 }
 
+/* Pop the first cell digset on the given circuit from the SENDME last digests
+ * list. NULL is returned if the list is uninitialized or empty.
+ *
+ * The caller gets ownership of the returned digest thus is responsible for
+ * freeing the memory. */
+static uint8_t *
+pop_first_cell_digest(const circuit_t *circ)
+{
+  uint8_t *circ_digest;
+
+  tor_assert(circ);
+
+  if (circ->sendme_last_digests == NULL ||
+      smartlist_len(circ->sendme_last_digests) == 0) {
+    return NULL;
+  }
+
+  circ_digest = smartlist_get(circ->sendme_last_digests, 0);
+  smartlist_del_keeporder(circ->sendme_last_digests, 0);
+  return circ_digest;
+}
+
 /* Return true iff the given cell digest matches the first digest in the
  * circuit sendme list. */
 static bool
-v1_digest_matches(const circuit_t *circ, const uint8_t *cell_digest)
+v1_digest_matches(const uint8_t *circ_digest, const uint8_t *cell_digest)
 {
-  bool ret = false;
-  uint8_t *circ_digest = NULL;
-
-  tor_assert(circ);
+  tor_assert(circ_digest);
   tor_assert(cell_digest);
 
-  /* We shouldn't have received a SENDME if we have no digests. Log at
-   * protocol warning because it can be tricked by sending many SENDMEs
-   * without prior data cell. */
-  if (circ->sendme_last_digests == NULL ||
-      smartlist_len(circ->sendme_last_digests) == 0) {
-    log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
-           "We received a SENDME but we have no cell digests to match. "
-           "Closing circuit.");
-    goto no_match;
-  }
-
-  /* Pop the first element that was added (FIFO) and compare it. */
-  circ_digest = smartlist_get(circ->sendme_last_digests, 0);
-  smartlist_del_keeporder(circ->sendme_last_digests, 0);
-
   /* Compare the digest with the one in the SENDME. This cell is invalid
    * without a perfect match. */
   if (tor_memneq(circ_digest, cell_digest, TRUNNEL_SENDME_V1_DIGEST_LEN)) {
     log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
            "SENDME v1 cell digest do not match.");
-    goto no_match;
+    return false;
   }
-  /* Digests matches! */
-  ret = true;
 
- no_match:
-  /* This digest was popped from the circuit list. Regardless of what happens,
-   * we have no more use for it. */
-  tor_free(circ_digest);
-  return ret;
+  /* Digests matches! */
+  return true;
 }
 
 /* Return true iff the given decoded SENDME version 1 cell is valid and
@@ -97,13 +96,13 @@ v1_digest_matches(const circuit_t *circ, const uint8_t *cell_digest)
  * cell we saw which tells us that the other side has in fact seen that cell.
  * See proposal 289 for more details. */
 static bool
-cell_v1_is_valid(const sendme_cell_t *cell, const circuit_t *circ)
+cell_v1_is_valid(const sendme_cell_t *cell, const uint8_t *circ_digest)
 {
   tor_assert(cell);
-  tor_assert(circ);
+  tor_assert(circ_digest);
 
   const uint8_t *cell_digest = sendme_cell_getconstarray_data_v1_digest(cell);
-  return v1_digest_matches(circ, cell_digest);
+  return v1_digest_matches(circ_digest, cell_digest);
 }
 
 /* Return true iff the given cell version can be handled or if the minimum
@@ -160,6 +159,7 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload,
                 size_t cell_payload_len)
 {
   uint8_t cell_version;
+  uint8_t *circ_digest = NULL;
   sendme_cell_t *cell = NULL;
 
   tor_assert(circ);
@@ -184,10 +184,24 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload,
     goto invalid;
   }
 
+  /* Pop the first element that was added (FIFO). We do that regardless of the
+   * version so we don't accumulate on the circuit if v0 is used by the other
+   * end point. */
+  circ_digest = pop_first_cell_digest(circ);
+  if (circ_digest == NULL) {
+    /* We shouldn't have received a SENDME if we have no digests. Log at
+     * protocol warning because it can be tricked by sending many SENDMEs
+     * without prior data cell. */
+    log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
+           "We received a SENDME but we have no cell digests to match. "
+           "Closing circuit.");
+    goto invalid;
+  }
+
   /* Validate depending on the version now. */
   switch (cell_version) {
   case 0x01:
-    if (!cell_v1_is_valid(cell, circ)) {
+    if (!cell_v1_is_valid(cell, circ_digest)) {
       goto invalid;
     }
     break;
@@ -204,9 +218,11 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload,
 
   /* Valid cell. */
   sendme_cell_free(cell);
+  tor_free(circ_digest);
   return true;
  invalid:
   sendme_cell_free(cell);
+  tor_free(circ_digest);
   return false;
 }
 

src/test/test_relaycell.c

@@ -17,6 +17,7 @@
 #include "core/or/circuitbuild.h"
 #include "core/or/circuitlist.h"
 #include "core/or/connection_edge.h"
+#include "core/or/sendme.h"
 #include "core/or/relay.h"
 #include "test/test.h"
 #include "test/log_test_helpers.h"
@@ -813,6 +814,10 @@ test_circbw_relay(void *arg)
 
   /* Sendme on circuit with non-full window: counted */
   PACK_CELL(0, RELAY_COMMAND_SENDME, "");
+  /* Recording a cell, the window is updated after decryption so off by one in
+   * order to record and then we process it with the proper window. */
+  circ->cpath->package_window = 901;
+  sendme_record_cell_digest_on_circ(TO_CIRCUIT(circ), circ->cpath);
   circ->cpath->package_window = 900;
   connection_edge_process_relay_cell(&cell, TO_CIRCUIT(circ), edgeconn,
                                      circ->cpath);

src/test/test_sendme.c

@@ -143,11 +143,13 @@ test_v1_build_cell(void *arg)
 
   or_circ = or_circuit_new(1, NULL);
   circ = TO_CIRCUIT(or_circ);
+  circ->sendme_last_digests = smartlist_new();
 
   cell_digest = crypto_digest_new();
   tt_assert(cell_digest);
   crypto_digest_add_bytes(cell_digest, "AAAAAAAAAAAAAAAAAAAA", 20);
   crypto_digest_get_digest(cell_digest, (char *) digest, sizeof(digest));
+  smartlist_add(circ->sendme_last_digests, tor_memdup(digest, sizeof(digest)));
 
   /* SENDME v1 payload is 3 bytes + 20 bytes digest. See spec. */
   ret = build_cell_payload_v1(digest, payload);
@@ -157,6 +159,8 @@ test_v1_build_cell(void *arg)
 
   /* An empty payload means SENDME version 0 thus valid. */
   tt_int_op(sendme_is_valid(circ, payload, 0), OP_EQ, true);
+  /* Current phoney digest should have been popped. */
+  tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 0);
 
   /* An unparseable cell means invalid. */
   setup_full_capture_of_logs(LOG_INFO);