[tor-commits] [tor/master] sendme: Better handle the random padding
asn at torproject.org
asn at torproject.org
Thu May 2 15:16:21 UTC 2019
commit d084f9115d7d46ad5e029b9c75cea716fa7d65a5
Author: David Goulet <dgoulet at torproject.org>
Date: Wed Apr 24 15:39:10 2019 -0400
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 at torproject.org>
---
src/core/or/relay.c | 74 ++++++++++++++++++++++++++++++++++++++------------
src/core/or/relay.h | 1 +
src/test/test_sendme.c | 46 +++++++++++++++++++++++++++++++
3 files changed, 103 insertions(+), 18 deletions(-)
diff --git a/src/core/or/relay.c b/src/core/or/relay.c
index 504f391d9..d273facd5 100644
--- a/src/core/or/relay.c
+++ b/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");
diff --git a/src/core/or/relay.h b/src/core/or/relay.h
index ea1b358ff..2248cdf38 100644
--- a/src/core/or/relay.h
+++ b/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) */
diff --git a/src/test/test_sendme.c b/src/test/test_sendme.c
index d6410a748..50943e5f4 100644
--- a/src/test/test_sendme.c
+++ b/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
};
More information about the tor-commits
mailing list