sendme: Better handle the random padding

We add random padding to every cell if there is room. This commit not only
fixes how we compute that random padding length/offset but also improves its
safety with helper functions and a unit test.

Part of #26288

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

src/core/or/relay.c

@@ -529,6 +529,60 @@ relay_command_to_string(uint8_t command)
   }
 }
 
+/** Return the offset where the padding should start. The <b>data_len</b> is
+ * the relay payload length expected to be put in the cell. It can not be
+ * bigger than RELAY_PAYLOAD_SIZE else this function assert().
+ *
+ * Value will always be smaller than CELL_PAYLOAD_SIZE because this offset is
+ * for the entire cell length not just the data payload length. Zero is
+ * returned if there is no room for padding.
+ *
+ * This function always skips the first 4 bytes after the payload because
+ * having some unused zero bytes has saved us a lot of times in the past. */
+
+STATIC size_t
+get_pad_cell_offset(size_t data_len)
+{
+  /* This is never suppose to happen but in case it does, stop right away
+   * because if tor is tricked somehow into not adding random bytes to the
+   * payload with this function returning 0 for a bad data_len, the entire
+   * authenticated SENDME design can be bypassed leading to bad denial of
+   * service attacks. */
+  tor_assert(data_len <= RELAY_PAYLOAD_SIZE);
+
+  /* If the offset is larger than the cell payload size, we return an offset
+   * of zero indicating that no padding needs to be added. */
+  size_t offset = RELAY_HEADER_SIZE + data_len + 4;
+  if (offset >= CELL_PAYLOAD_SIZE) {
+    return 0;
+  }
+  return offset;
+}
+
+/* Add random bytes to the unused portion of the payload, to foil attacks
+ * where the other side can predict all of the bytes in the payload and thus
+ * compute the authenticated SENDME cells without seeing the traffic. See
+ * proposal 289. */
+static void
+pad_cell_payload(uint8_t *cell_payload, size_t data_len)
+{
+  size_t pad_offset, pad_len;
+
+  tor_assert(cell_payload);
+
+  pad_offset = get_pad_cell_offset(data_len);
+  if (pad_offset == 0) {
+    /* We can't add padding so we are done. */
+    return;
+  }
+
+  /* Remember here that the cell_payload is the length of the header and
+   * payload size so we offset it using the full lenght of the cell. */
+  pad_len = CELL_PAYLOAD_SIZE - pad_offset;
+  crypto_fast_rng_getbytes(get_thread_fast_rng(),
+                           cell_payload + pad_offset, pad_len);
+}
+
 /** Make a relay cell out of <b>relay_command</b> and <b>payload</b>, and send
  * it onto the open circuit <b>circ</b>. <b>stream_id</b> is the ID on
  * <b>circ</b> for the stream that's sending the relay cell, or 0 if it's a
@@ -547,8 +601,6 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ,
   cell_t cell;
   relay_header_t rh;
   cell_direction_t cell_direction;
-  int random_bytes_len;
-  size_t random_bytes_offset = 0;
   /* XXXX NM Split this function into a separate versions per circuit type? */
 
   tor_assert(circ);
@@ -574,22 +626,8 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *circ,
   if (payload_len)
     memcpy(cell.payload+RELAY_HEADER_SIZE, payload, payload_len);
 
-  /* Add random bytes to the unused portion of the payload, to foil attacks
-   * where the other side can predict all of the bytes in the payload and thus
-   * compute authenticated sendme cells without seeing the traffic. See
-   * proposal 289.
-   *
-   * We'll skip the first 4 bytes of unused data because having some unused
-   * zero bytes has saved us a lot of times in the past. */
-  random_bytes_len = RELAY_PAYLOAD_SIZE -
-                     (RELAY_HEADER_SIZE + payload_len + 4);
-  if (random_bytes_len < 0) {
-    random_bytes_len = 0;
-  }
-  random_bytes_offset = RELAY_PAYLOAD_SIZE - random_bytes_len;
-  crypto_fast_rng_getbytes(get_thread_fast_rng(),
-                           cell.payload + random_bytes_offset,
-                           random_bytes_len);
+  /* Add random padding to the cell if we can. */
+  pad_cell_payload(cell.payload, payload_len);
 
   log_debug(LD_OR,"delivering %d cell %s.", relay_command,
             cell_direction == CELL_DIRECTION_OUT ? "forward" : "backward");

src/core/or/relay.h

@@ -120,6 +120,7 @@ STATIC int cell_queues_check_size(void);
 STATIC int connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ,
                                    edge_connection_t *conn,
                                    crypt_path_t *layer_hint);
+STATIC size_t get_pad_cell_offset(size_t payload_len);
 
 #endif /* defined(RELAY_PRIVATE) */
 

src/test/test_sendme.c

@@ -6,11 +6,13 @@
 #define CIRCUITLIST_PRIVATE
 #define NETWORKSTATUS_PRIVATE
 #define SENDME_PRIVATE
+#define RELAY_PRIVATE
 
 #include "core/or/circuit_st.h"
 #include "core/or/or_circuit_st.h"
 #include "core/or/origin_circuit_st.h"
 #include "core/or/circuitlist.h"
+#include "core/or/relay.h"
 #include "core/or/sendme.h"
 
 #include "feature/nodelist/networkstatus.h"
@@ -209,6 +211,48 @@ test_v1_build_cell(void *arg)
   circuit_free_(circ);
 }
 
+static void
+test_cell_payload_pad(void *arg)
+{
+  size_t pad_offset, payload_len, expected_offset;
+
+  (void) arg;
+
+  /* Offset should be 0, not enough room for padding. */
+  payload_len = RELAY_PAYLOAD_SIZE;
+  pad_offset = get_pad_cell_offset(payload_len);
+  tt_int_op(pad_offset, OP_EQ, 0);
+  tt_int_op(CELL_PAYLOAD_SIZE - pad_offset, OP_LE, CELL_PAYLOAD_SIZE);
+
+  /* Still no room because we keep 4 extra bytes. */
+  pad_offset = get_pad_cell_offset(payload_len - 4);
+  tt_int_op(pad_offset, OP_EQ, 0);
+  tt_int_op(CELL_PAYLOAD_SIZE - pad_offset, OP_LE, CELL_PAYLOAD_SIZE);
+
+  /* We should have 1 byte of padding. Meaning, the offset should be the
+   * CELL_PAYLOAD_SIZE minus 1 byte. */
+  expected_offset = CELL_PAYLOAD_SIZE - 1;
+  pad_offset = get_pad_cell_offset(payload_len - 5);
+  tt_int_op(pad_offset, OP_EQ, expected_offset);
+  tt_int_op(CELL_PAYLOAD_SIZE - pad_offset, OP_LE, CELL_PAYLOAD_SIZE);
+
+  /* Now some arbitrary small payload length. The cell size is header + 10 +
+   * extra 4 bytes we keep so the offset should be there. */
+  expected_offset = RELAY_HEADER_SIZE + 10 + 4;
+  pad_offset = get_pad_cell_offset(10);
+  tt_int_op(pad_offset, OP_EQ, expected_offset);
+  tt_int_op(CELL_PAYLOAD_SIZE - pad_offset, OP_LE, CELL_PAYLOAD_SIZE);
+
+  /* Data length of 0. */
+  expected_offset = RELAY_HEADER_SIZE + 4;
+  pad_offset = get_pad_cell_offset(0);
+  tt_int_op(pad_offset, OP_EQ, expected_offset);
+  tt_int_op(CELL_PAYLOAD_SIZE - pad_offset, OP_LE, CELL_PAYLOAD_SIZE);
+
+ done:
+  ;
+}
+
 struct testcase_t sendme_tests[] = {
   { "v1_note_digest", test_v1_note_digest, TT_FORK,
     NULL, NULL },
@@ -216,6 +260,8 @@ struct testcase_t sendme_tests[] = {
     NULL, NULL },
   { "v1_build_cell", test_v1_build_cell, TT_FORK,
     NULL, NULL },
+  { "cell_payload_pad", test_cell_payload_pad, TT_FORK,
+    NULL, NULL },
 
   END_OF_TESTCASES
 };