From e78e00411820973c9f69322ea1ad2dd133620a25 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Mon, 1 Sep 2008 22:08:13 +0000 Subject: [PATCH] backport r16605: relays reject risky extend cells svn:r16728 --- ChangeLog | 6 +++++- doc/TODO.020 | 2 +- src/or/circuitbuild.c | 34 ++++++++++++++++++++++++++++++---- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9c072952ea..54c6008be0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,8 +1,12 @@ -Changes in version 0.2.0.31 - 2008-08-?? +Changes in version 0.2.0.31 - 2008-09-?? o Major bugfixes: - Make sure that two circuits can never exist on the same connection with the same circuit ID, even if one is marked for close. This is conceivably a bugfix for bug 779; fixes a bug on 0.1.0.4-rc. + - Relays now reject risky extend cells: if the extend cell includes + a digest of all zeroes, or asks to extend back to the relay that + sent the extend cell, tear down the circuit. Ideas suggested + by rovv. o Minor bugfixes: - Fix a small alignment and memory-wasting bug on buffer chunks. Spotted diff --git a/doc/TODO.020 b/doc/TODO.020 index d87db8c940..a5c9669894 100644 --- a/doc/TODO.020 +++ b/doc/TODO.020 @@ -12,6 +12,6 @@ Backport for 0.2.0 once better tested: o r16136: prevent circid collision. [Also backport to 0.1.2.x??] - r16143: generate stream close events from connection_edge_destroy(). o r16450: open /dev/pf before dropping privileges. - - r16605: relays reject risky extend cells. + o r16605: relays reject risky extend cells. - r16698: don't use a new entry guard that's also your exit. diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 3be2689995..b121c6e196 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -705,10 +705,13 @@ circuit_note_clock_jumped(int seconds_elapsed) circuit_expire_all_dirty_circs(); } -/** Take the 'extend' cell, pull out addr/port plus the onion skin. Make - * sure we're connected to the next hop, and pass it the onion skin using - * a create cell. Return -1 if we want to warn and tear down the circuit, - * else return 0. +/** Take the 'extend' cell, pull out addr/port plus the onion + * skin and identity digest for the next hop. If we're already connected, + * pass the onion skin to the next hop using a create cell; otherwise + * launch a new OR connection, and circ will notice when the + * connection succeeds or fails. + * + * Return -1 if we want to warn and tear down the circuit, else return 0. */ int circuit_extend(cell_t *cell, circuit_t *circ) @@ -744,6 +747,29 @@ circuit_extend(cell_t *cell, circuit_t *circ) onionskin = cell->payload+RELAY_HEADER_SIZE+4+2; id_digest = cell->payload+RELAY_HEADER_SIZE+4+2+ONIONSKIN_CHALLENGE_LEN; + + /* First, check if they asked us for 0000..0000. We support using + * an empty fingerprint for the first hop (e.g. for a bridge relay), + * but we don't want to let people send us extend cells for empty + * fingerprints -- a) because it opens the user up to a mitm attack, + * and b) because it lets an attacker force the relay to hold open a + * new TLS connection for each extend request. */ + if (tor_digest_is_zero(id_digest)) { + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Client asked me to extend without specifying an id_digest."); + return -1; + } + + /* Next, check if we're being asked to connect to the hop that the + * extend cell came from. There isn't any reason for that, and it can + * assist circular-path attacks. */ + if (!memcmp(id_digest, TO_OR_CIRCUIT(circ)->p_conn->identity_digest, + DIGEST_LEN)) { + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Client asked me to extend back to the previous hop."); + return -1; + } + n_conn = connection_or_get_by_identity_digest(id_digest); /* If we don't have an open conn, or the conn we have is obsolete -- 2.47.3