Enforce that SENDME tags have the expected length

The length is no longer a constant 20, but now depends on the
relay crypto algorithm in use.

diff --git a/src/core/crypto/relay_crypto.c b/src/core/crypto/relay_crypto.c
index f9d72a1f48a2e6344c7518770c27defdcd8d9d59..42dadbf8c557a11626f33c8b6add38bb1322f5a0 100644 (file)
--- a/src/core/crypto/relay_crypto.c
+++ b/src/core/crypto/relay_crypto.c
@@ -51,6 +51,20 @@ relay_crypto_get_sendme_tag(relay_crypto_t *crypto,
   tor_assert_unreached();
 }
 
+/** Return the length of SENDME tags generated by `crypto`. */
+size_t
+relay_crypto_sendme_tag_len(const relay_crypto_t *crypto)
+{
+  tor_assert(crypto);
+  switch (crypto->kind) {
+    case RCK_TOR1:
+      return SENDME_TAG_LEN_TOR1;
+    case RCK_CGO:
+      return SENDME_TAG_LEN_CGO;
+  }
+  tor_assert_unreached();
+}
+
 /**
  * Handle a single layer of client-side backward encryption
  * with crypto of an arbitary type.

diff --git a/src/core/crypto/relay_crypto.h b/src/core/crypto/relay_crypto.h
index 5cb29af673e2b20e2d183bb55e4d370e2051f5b6..28de4c50489ccb24fb9ee588db82ae153dbbca9e 100644 (file)
--- a/src/core/crypto/relay_crypto.h
+++ b/src/core/crypto/relay_crypto.h
@@ -54,5 +54,6 @@ void relay_crypto_assert_ok(const relay_crypto_t *crypto);
 
 const uint8_t *relay_crypto_get_sendme_tag(relay_crypto_t *crypto,
                                            size_t *len_out);
+size_t relay_crypto_sendme_tag_len(const relay_crypto_t *crypto);
 
 #endif /* !defined(TOR_RELAY_CRYPTO_H) */

diff --git a/src/core/or/circuit_st.h b/src/core/or/circuit_st.h
index c28900e59fb55da6402515d54f85cbbba74ffae6..0bc20ba1a2e9d4cda6a15dfb2ad33dfb61455f97 100644 (file)
--- a/src/core/or/circuit_st.h
+++ b/src/core/or/circuit_st.h
@@ -144,7 +144,17 @@ struct circuit_t {
    * For example, position 2 (starting at 0) means that we've received 300
    * cells so the 300th cell digest is kept at index 2.
    *
-   * At maximum, this list contains 200 bytes plus the smartlist overhead. */
+   * At maximum, this list contains 200 bytes plus the smartlist overhead.
+   *
+   * The elements in this list are always of length SENDME_TAG_LEN_TOR1
+   * (== DIGEST_LEN, == 20).  The actual digests stored in those elements
+   * may be smaller, however, if another relay crypto algorithm is in use.
+   **/
+  /* Note that this is a per-circuit field, although logically it might make
+   * more sense for it to be a per-hop field.  That doesn't matter in C tor,
+   * since we don't send more than a single window of cells to any given
+   * relay except for the exit.
+   */
   smartlist_t *sendme_last_digests;
 
   /** Temporary field used during circuits_handle_oom. */

diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c
index 07b2f3984bc5213efdbb7f4adf8d3c7143573c5c..be9c15ed39965687b70be220ba1deec90024fddc 100644 (file)
--- a/src/core/or/sendme.c
+++ b/src/core/or/sendme.c
@@ -7,7 +7,9 @@
  *        creating/parsing cells and handling the content.
  */
 
+// For access to cpath pvt_crypto field.
 #define SENDME_PRIVATE
+#define CRYPT_PATH_PRIVATE
 
 #include "core/or/or.h"
 
@@ -108,18 +110,19 @@ v1_tag_matches(const uint8_t *circ_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 uint8_t *circ_digest)
+cell_v1_is_valid(const sendme_cell_t *cell, const uint8_t *circ_digest,
+                 size_t circ_digest_len)
 {
   tor_assert(cell);
   tor_assert(circ_digest);
 
-  // XXXX TODO: make sure that length is the length we _expected_.
-
   size_t tag_len = sendme_cell_get_data_len(cell);
   if (! tag_len_ok(tag_len))
     return false;
   if (sendme_cell_getlen_data_v1_digest(cell) < tag_len)
     return false;
+  if (tag_len != circ_digest_len)
+    return false;
 
   const uint8_t *cell_digest = sendme_cell_getconstarray_data_v1_digest(cell);
   return v1_tag_matches(circ_digest, cell_digest, tag_len);
@@ -174,8 +177,16 @@ cell_version_can_be_handled(uint8_t cell_version)
  * This is the main critical function to make sure we can continue to
  * send/recv cells on a circuit. If the SENDME is invalid, the circuit should
  * be marked for close by the caller. */
+/*
+ * NOTE: This function uses `layer_hint` to determine
+ * what the sendme tag length will be, and nothing else.
+ * Notably, we _don't_ keep a separate queue
+ * of expected tags for each layer!
+ */
 STATIC bool
-sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload,
+sendme_is_valid(const circuit_t *circ,
+                const crypt_path_t *layer_hint,
+                const uint8_t *cell_payload,
                 size_t cell_payload_len)
 {
   uint8_t cell_version;
@@ -204,6 +215,19 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload,
     goto invalid;
   }
 
+  /* Determine the expected tag length for this sendme. */
+  size_t circ_expects_tag_len;
+  if (layer_hint) {
+    circ_expects_tag_len =
+      relay_crypto_sendme_tag_len(&layer_hint->pvt_crypto);
+  } else if (CIRCUIT_IS_ORCIRC(circ)) {
+    const or_circuit_t *or_circ = CONST_TO_OR_CIRCUIT(circ);
+    circ_expects_tag_len = relay_crypto_sendme_tag_len(&or_circ->crypto);
+  } else {
+    tor_assert_nonfatal_unreached();
+    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. */
@@ -216,12 +240,10 @@ sendme_is_valid(const circuit_t *circ, const uint8_t *cell_payload,
            "We received a SENDME but we have no cell digests to match. "
            "Closing circuit.");
     goto invalid;
-  }
-
-  /* Validate depending on the version now. */
+  }  /* Validate depending on the version now. */
   switch (cell_version) {
   case 0x01:
-    if (!cell_v1_is_valid(cell, circ_digest)) {
+    if (!cell_v1_is_valid(cell, circ_digest, circ_expects_tag_len)) {
       goto invalid;
     }
     break;
@@ -507,7 +529,7 @@ sendme_process_circuit_level(crypt_path_t *layer_hint,
 
   /* Validate the SENDME cell. Depending on the version, different validation
    * can be done. An invalid SENDME requires us to close the circuit. */
-  if (!sendme_is_valid(circ, cell_payload, cell_payload_len)) {
+  if (!sendme_is_valid(circ, layer_hint, cell_payload, cell_payload_len)) {
     return -END_CIRC_REASON_TORPROTOCOL;
   }
 

diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h
index c21726f5c8e45d7a31c06d81165daf2798c9336e..c76b6d3947052019de8847fb050f7d5f6eafb798 100644 (file)
--- a/src/core/or/sendme.h
+++ b/src/core/or/sendme.h
@@ -69,6 +69,7 @@ STATIC ssize_t build_cell_payload_v1(const uint8_t *cell_tag,
                                      size_t tag_len,
                                      uint8_t *payload);
 STATIC bool sendme_is_valid(const circuit_t *circ,
+                            const crypt_path_t *layer_hint,
                             const uint8_t *cell_payload,
                             size_t cell_payload_len);
 STATIC bool circuit_sendme_cell_is_next(int deliver_window,

diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c
index b633f3500141517219e36daa07b1f661e3628546..cde6dd5d2915c5a5f655136ca9fcdd2478d065d5 100644 (file)
--- a/src/test/test_sendme.c
+++ b/src/test/test_sendme.c
@@ -159,20 +159,22 @@ test_v1_build_cell(void *arg)
   /* Validation. */
 
   /* An empty payload means SENDME version 0 thus valid. */
-  tt_int_op(sendme_is_valid(circ, payload, 0), OP_EQ, true);
+  tt_int_op(sendme_is_valid(circ, NULL, 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);
-  tt_int_op(sendme_is_valid(circ, (const uint8_t *) "A", 1), OP_EQ, false);
+  tt_int_op(sendme_is_valid(circ, NULL, (const uint8_t *) "A", 1),
+            OP_EQ, false);
   expect_log_msg_containing("Unparseable SENDME cell received. "
                             "Closing circuit.");
   teardown_capture_of_logs();
 
   /* No cell digest recorded for this. */
   setup_full_capture_of_logs(LOG_INFO);
-  tt_int_op(sendme_is_valid(circ, payload, sizeof(payload)), OP_EQ, false);
+  tt_int_op(sendme_is_valid(circ, NULL, payload, sizeof(payload)),
+            OP_EQ, false);
   expect_log_msg_containing("We received a SENDME but we have no cell digests "
                             "to match. Closing circuit.");
   teardown_capture_of_logs();
@@ -182,7 +184,8 @@ test_v1_build_cell(void *arg)
   sendme_record_cell_digest_on_circ(circ, NULL);
   tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 1);
   setup_full_capture_of_logs(LOG_INFO);
-  tt_int_op(sendme_is_valid(circ, payload, sizeof(payload)), OP_EQ, false);
+  tt_int_op(sendme_is_valid(circ, NULL, payload, sizeof(payload)),
+            OP_EQ, false);
   /* After a validation, the last digests is always popped out. */
   tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 0);
   expect_log_msg_containing("SENDME v1 cell digest do not match.");
@@ -193,7 +196,8 @@ test_v1_build_cell(void *arg)
   circ->package_window = CIRCWINDOW_INCREMENT + 1;
   sendme_record_cell_digest_on_circ(circ, NULL);
   tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 1);
-  tt_int_op(sendme_is_valid(circ, payload, sizeof(payload)), OP_EQ, true);
+  tt_int_op(sendme_is_valid(circ, NULL, payload, sizeof(payload)),
+            OP_EQ, true);
   /* After a validation, the last digests is always popped out. */
   tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 0);