[tor-commits] [tor/master] sendme: Record cell digest on both client and exit

[nickm at torproject.org]

commit 59b9eecc19877f38b2c9d8b4f7964c6e9875f4c0
Author: David Goulet <dgoulet at torproject.org>
Date:   Tue May 7 09:16:39 2019 -0400

    sendme: Record cell digest on both client and exit
    
    It turns out that only the exit side is validating the authenticated SENDME v1
    logic and never the client side. Which means that if a client ever uploaded
    data towards an exit, the authenticated SENDME logic wouldn't apply.
    
    For this to work, we have to record the cell digest client side as well which
    introduced a new function that supports both type of edges.
    
    This also removes a test that is not valid anymore which was that we didn't
    allow cell recording on an origin circuit (client).
    
    Part of #30428
    
    Signed-off-by: David Goulet <dgoulet at torproject.org>
---
 scripts/maint/practracker/exceptions.txt |  2 +-
 src/core/or/relay.c                      |  2 +-
 src/core/or/sendme.c                     | 49 +++++++++++++++++++++++---------
 src/core/or/sendme.h                     |  3 +-
 src/test/test_sendme.c                   | 28 +++++-------------
 5 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt
index cba3ddc1b..a83bb62bb 100644
--- a/scripts/maint/practracker/exceptions.txt
+++ b/scripts/maint/practracker/exceptions.txt
@@ -122,7 +122,7 @@ problem function-size /src/core/or/policies.c:policy_summarize() 107
 problem function-size /src/core/or/protover.c:protover_all_supported() 117
 problem file-size /src/core/or/relay.c 3173
 problem function-size /src/core/or/relay.c:circuit_receive_relay_cell() 123
-problem function-size /src/core/or/relay.c:relay_send_command_from_edge_() 112
+problem function-size /src/core/or/relay.c:relay_send_command_from_edge_() 116
 problem function-size /src/core/or/relay.c:connection_ap_process_end_not_open() 194
 problem function-size /src/core/or/relay.c:connection_edge_process_relay_cell_not_open() 139
 problem function-size /src/core/or/relay.c:connection_edge_process_relay_cell() 520
diff --git a/src/core/or/relay.c b/src/core/or/relay.c
index 8b3a1be18..91236e5fe 100644
--- a/src/core/or/relay.c
+++ b/src/core/or/relay.c
@@ -701,7 +701,7 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ,
    * we need to. This call needs to be after the circuit_package_relay_cell()
    * because the cell digest is set within that function. */
   if (relay_command == RELAY_COMMAND_DATA) {
-    sendme_record_cell_digest(circ);
+    sendme_record_cell_digest_on_circ(circ, cpath_layer);
   }
 
   return 0;
diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c
index e7c65d99e..586d4d0ae 100644
--- a/src/core/or/sendme.c
+++ b/src/core/or/sendme.c
@@ -287,6 +287,21 @@ send_circuit_level_sendme(circuit_t *circ, crypt_path_t *layer_hint,
   return 0;
 }
 
+/* Record the cell digest only if the next cell is expected to be a SENDME. */
+static void
+record_cell_digest_on_circ(circuit_t *circ, const uint8_t *sendme_digest)
+{
+  tor_assert(circ);
+  tor_assert(sendme_digest);
+
+  /* Add the digest to the last seen list in the circuit. */
+  if (circ->sendme_last_digests == NULL) {
+    circ->sendme_last_digests = smartlist_new();
+  }
+  smartlist_add(circ->sendme_last_digests,
+                tor_memdup(sendme_digest, DIGEST_LEN));
+}
+
 /*
  * Public API
  */
@@ -571,31 +586,37 @@ sendme_note_stream_data_packaged(edge_connection_t *conn)
   return --conn->package_window;
 }
 
-/* Note the cell digest in the circuit sendme last digests FIFO if applicable.
- * It is safe to pass a circuit that isn't meant to track those digests. */
+/* Record the cell digest into the circuit sendme digest list depending on
+ * which edge we are. The digest is recorded only if we expect the next cell
+ * that we will receive is a SENDME so we can match the digest. */
 void
-sendme_record_cell_digest(circuit_t *circ)
+sendme_record_cell_digest_on_circ(circuit_t *circ, crypt_path_t *cpath)
 {
-  const uint8_t *digest;
+  int package_window;
+  uint8_t *sendme_digest;
 
   tor_assert(circ);
 
-  /* We only keep the cell digest if we are the Exit on that circuit and if
-   * this cell is the last one before the client should send a SENDME. */
-  if (CIRCUIT_IS_ORIGIN(circ)) {
-    return;
+  package_window = circ->package_window;
+  if (cpath) {
+    package_window = cpath->package_window;
   }
+
   /* Is this the last cell before a SENDME? The idea is that if the
    * package_window reaches a multiple of the increment, after this cell, we
    * should expect a SENDME. */
-  if (!sendme_circuit_cell_is_next(circ->package_window)) {
+  if (!sendme_circuit_cell_is_next(package_window)) {
     return;
   }
 
-  /* Add the digest to the last seen list in the circuit. */
-  digest = relay_crypto_get_sendme_digest(&TO_OR_CIRCUIT(circ)->crypto);
-  if (circ->sendme_last_digests == NULL) {
-    circ->sendme_last_digests = smartlist_new();
+  /* Getting the digest is expensive so we only do it once we are certain to
+   * record it on the circuit. */
+  if (cpath) {
+    sendme_digest = cpath_get_sendme_digest(cpath);
+  } else {
+    sendme_digest =
+      relay_crypto_get_sendme_digest(&TO_OR_CIRCUIT(circ)->crypto);
   }
-  smartlist_add(circ->sendme_last_digests, tor_memdup(digest, DIGEST_LEN));
+
+  record_cell_digest_on_circ(circ, sendme_digest);
 }
diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h
index ac18bbdd3..2fb73f76d 100644
--- a/src/core/or/sendme.h
+++ b/src/core/or/sendme.h
@@ -35,8 +35,9 @@ int sendme_note_circuit_data_packaged(circuit_t *circ,
 int sendme_note_stream_data_packaged(edge_connection_t *conn);
 
 /* Track cell digest. */
-void sendme_record_cell_digest(circuit_t *circ);
 void sendme_circuit_record_outbound_cell(or_circuit_t *or_circ);
+/* Record cell digest on circuit. */
+void sendme_record_cell_digest_on_circ(circuit_t *circ, crypt_path_t *cpath);
 
 /* Circuit level information. */
 bool sendme_circuit_cell_is_next(int window);
diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c
index d40fbaf86..463a0ec08 100644
--- a/src/test/test_sendme.c
+++ b/src/test/test_sendme.c
@@ -46,26 +46,12 @@ static void
 test_v1_record_digest(void *arg)
 {
   or_circuit_t *or_circ = NULL;
-  origin_circuit_t *orig_circ = NULL;
   circuit_t *circ = NULL;
 
   (void) arg;
 
-  /* Create our dummy circuits. */
-  orig_circ = origin_circuit_new();
-  tt_assert(orig_circ);
+  /* Create our dummy circuit. */
   or_circ = or_circuit_new(1, NULL);
-
-  /* Start by pointing to the origin circuit. */
-  circ = TO_CIRCUIT(orig_circ);
-  circ->purpose = CIRCUIT_PURPOSE_S_REND_JOINED;
-
-  /* We should never note SENDME digest on origin circuit. */
-  sendme_record_cell_digest(circ);
-  tt_assert(!circ->sendme_last_digests);
-  /* We do not need the origin circuit for now. */
-  orig_circ = NULL;
-  circuit_free_(circ);
   /* Points it to the OR circuit now. */
   circ = TO_CIRCUIT(or_circ);
 
@@ -73,23 +59,23 @@ test_v1_record_digest(void *arg)
    * in order to catched the CIRCWINDOW_INCREMENT-nth cell. Try something that
    * shouldn't be noted. */
   circ->package_window = CIRCWINDOW_INCREMENT;
-  sendme_record_cell_digest(circ);
+  sendme_record_cell_digest_on_circ(circ, NULL);
   tt_assert(!circ->sendme_last_digests);
 
   /* This should work now. Package window at CIRCWINDOW_INCREMENT + 1. */
   circ->package_window++;
-  sendme_record_cell_digest(circ);
+  sendme_record_cell_digest_on_circ(circ, NULL);
   tt_assert(circ->sendme_last_digests);
   tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 1);
 
   /* Next cell in the package window shouldn't do anything. */
   circ->package_window++;
-  sendme_record_cell_digest(circ);
+  sendme_record_cell_digest_on_circ(circ, NULL);
   tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 1);
 
   /* The next CIRCWINDOW_INCREMENT should add one more digest. */
   circ->package_window = (CIRCWINDOW_INCREMENT * 2) + 1;
-  sendme_record_cell_digest(circ);
+  sendme_record_cell_digest_on_circ(circ, NULL);
   tt_int_op(smartlist_len(circ->sendme_last_digests), OP_EQ, 2);
 
  done:
@@ -188,7 +174,7 @@ test_v1_build_cell(void *arg)
 
   /* Note the wrong digest in the circuit, cell should fail validation. */
   circ->package_window = CIRCWINDOW_INCREMENT + 1;
-  sendme_record_cell_digest(circ);
+  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);
@@ -200,7 +186,7 @@ test_v1_build_cell(void *arg)
   /* Record the cell digest into the circuit, cell should validate. */
   memcpy(or_circ->crypto.sendme_digest, digest, sizeof(digest));
   circ->package_window = CIRCWINDOW_INCREMENT + 1;
-  sendme_record_cell_digest(circ);
+  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);
   /* After a validation, the last digests is always popped out. */