commit fb91d647acdf0560fc7479d72eeea52e4e6ff41d Author: Nick Mathewson nickm@torproject.org Date: Thu Oct 16 13:26:42 2014 -0400

Downgrade 'invalid result from curve25519 handshake: 4' warning

Also, refactor the way we handle failed handshakes so that this warning doesn't propagate itself to "onion_skin_client_handshake failed" and "circuit_finish_handshake failed" and "connection_edge_process_relay_cell (at origin) failed."

Resolves warning from 9635. --- changes/bug9635 | 3 +++ src/or/circuitbuild.c | 12 +++++++++--- src/or/command.c | 1 - src/or/onion.c | 24 +++++++++++++++--------- src/or/onion.h | 3 ++- src/or/onion_fast.c | 10 ++++++---- src/or/onion_fast.h | 3 ++- src/or/onion_ntor.c | 19 +++++++++++++++---- src/or/onion_ntor.h | 3 ++- src/or/onion_tap.c | 10 ++++++---- src/or/onion_tap.h | 3 ++- src/or/relay.c | 5 +++-- src/test/bench.c | 6 ++++-- src/test/test.c | 11 ++++++----- src/test/test_ntor_cl.c | 2 +- 15 files changed, 76 insertions(+), 39 deletions(-)

diff --git a/changes/bug9635 b/changes/bug9635 new file mode 100644 index 0000000..17a2ea5 --- /dev/null +++ b/changes/bug9635 @@ -0,0 +1,3 @@ + o Downgraded warnings: + - Don't warn when we've attempted to contact a relay using the wrong + ntor onion key. Closes ticket 9635. diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 897f90f..873eba1 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1256,8 +1256,10 @@ circuit_finish_handshake(origin_circuit_t *circ, crypt_path_t *hop; int rv;

- if ((rv = pathbias_count_build_attempt(circ)) < 0) + if ((rv = pathbias_count_build_attempt(circ)) < 0) { + log_warn(LD_CIRC, "pathbias_count_build_attempt failed: %d", rv); return rv; + }

if (circ->cpath->state == CPATH_STATE_AWAITING_KEYS) { hop = circ->cpath; @@ -1271,12 +1273,15 @@ circuit_finish_handshake(origin_circuit_t *circ, tor_assert(hop->state == CPATH_STATE_AWAITING_KEYS);

{ + const char *msg = NULL; if (onion_skin_client_handshake(hop->handshake_state.tag, &hop->handshake_state, reply->reply, reply->handshake_len, (uint8_t*)keys, sizeof(keys), - (uint8_t*)hop->rend_circ_nonce) < 0) { - log_warn(LD_CIRC,"onion_skin_client_handshake failed."); + (uint8_t*)hop->rend_circ_nonce, + &msg) < 0) { + if (msg) + log_warn(LD_CIRC,"onion_skin_client_handshake failed: %s", msg); return -END_CIRC_REASON_TORPROTOCOL; } } @@ -1284,6 +1289,7 @@ circuit_finish_handshake(origin_circuit_t *circ, onion_handshake_state_release(&hop->handshake_state);

if (circuit_init_cpath_crypto(hop, keys, 0)<0) { + log_warn(LD_BUG, "Couldn't initialize cpath crypto"); return -END_CIRC_REASON_TORPROTOCOL; }

diff --git a/src/or/command.c b/src/or/command.c index 1f6f93a..1f8d0ba 100644 --- a/src/or/command.c +++ b/src/or/command.c @@ -398,7 +398,6 @@ command_process_created_cell(cell_t *cell, channel_t *chan) log_debug(LD_OR,"at OP. Finishing handshake."); if ((err_reason = circuit_finish_handshake(origin_circ, &extended_cell.created_cell)) < 0) { - log_warn(LD_OR,"circuit_finish_handshake failed."); circuit_mark_for_close(circ, -err_reason); return; } diff --git a/src/or/onion.c b/src/or/onion.c index ae39f45..b5e801d 100644 --- a/src/or/onion.c +++ b/src/or/onion.c @@ -541,13 +541,15 @@ onion_skin_server_handshake(int type, * bytes worth of key material in <b>keys_out_len</b>, set * <b>rend_authenticator_out</b> to the "KH" field that can be used to * establish introduction points at this hop, and return 0. On failure, - * return -1. */ + * return -1, and set *msg_out to an error message if this is worth + * complaining to the usre about. */ int onion_skin_client_handshake(int type, const onion_handshake_state_t *handshake_state, const uint8_t *reply, size_t reply_len, uint8_t *keys_out, size_t keys_out_len, - uint8_t *rend_authenticator_out) + uint8_t *rend_authenticator_out, + const char **msg_out) { if (handshake_state->tag != type) return -1; @@ -555,12 +557,14 @@ onion_skin_client_handshake(int type, switch (type) { case ONION_HANDSHAKE_TYPE_TAP: if (reply_len != TAP_ONIONSKIN_REPLY_LEN) { - log_warn(LD_CIRC, "TAP reply was not of the correct length."); + if (msg_out) + *msg_out = "TAP reply was not of the correct length."; return -1; } if (onion_skin_TAP_client_handshake(handshake_state->u.tap, (const char*)reply, - (char *)keys_out, keys_out_len) < 0) + (char *)keys_out, keys_out_len, + msg_out) < 0) return -1;

memcpy(rend_authenticator_out, reply+DH_KEY_LEN, DIGEST_LEN); @@ -568,11 +572,12 @@ onion_skin_client_handshake(int type, return 0; case ONION_HANDSHAKE_TYPE_FAST: if (reply_len != CREATED_FAST_LEN) { - log_warn(LD_CIRC, "CREATED_FAST reply was not of the correct length."); + if (msg_out) + *msg_out = "TAP reply was not of the correct length."; return -1; } if (fast_client_handshake(handshake_state->u.fast, reply, - keys_out, keys_out_len) < 0) + keys_out, keys_out_len, msg_out) < 0) return -1;

memcpy(rend_authenticator_out, reply+DIGEST_LEN, DIGEST_LEN); @@ -580,15 +585,16 @@ onion_skin_client_handshake(int type, #ifdef CURVE25519_ENABLED case ONION_HANDSHAKE_TYPE_NTOR: if (reply_len < NTOR_REPLY_LEN) { - log_warn(LD_CIRC, "ntor reply was not of the correct length."); + if (msg_out) + *msg_out = "ntor reply was not of the correct length."; return -1; } { size_t keys_tmp_len = keys_out_len + DIGEST_LEN; uint8_t *keys_tmp = tor_malloc(keys_tmp_len); if (onion_skin_ntor_client_handshake(handshake_state->u.ntor, - reply, - keys_tmp, keys_tmp_len) < 0) { + reply, + keys_tmp, keys_tmp_len, msg_out) < 0) { tor_free(keys_tmp); return -1; } diff --git a/src/or/onion.h b/src/or/onion.h index d62f032..1a53896 100644 --- a/src/or/onion.h +++ b/src/or/onion.h @@ -51,7 +51,8 @@ int onion_skin_client_handshake(int type, const onion_handshake_state_t *handshake_state, const uint8_t *reply, size_t reply_len, uint8_t *keys_out, size_t key_out_len, - uint8_t *rend_authenticator_out); + uint8_t *rend_authenticator_out, + const char **msg_out);

/** A parsed CREATE, CREATE_FAST, or CREATE2 cell. */ typedef struct create_cell_t { diff --git a/src/or/onion_fast.c b/src/or/onion_fast.c index 38b62de..30f60e8 100644 --- a/src/or/onion_fast.c +++ b/src/or/onion_fast.c @@ -92,7 +92,8 @@ int fast_client_handshake(const fast_handshake_state_t *handshake_state, const uint8_t *handshake_reply_out,/*DIGEST_LEN*2 bytes*/ uint8_t *key_out, - size_t key_out_len) + size_t key_out_len, + const char **msg_out) { uint8_t tmp[DIGEST_LEN+DIGEST_LEN]; uint8_t *out; @@ -104,13 +105,14 @@ fast_client_handshake(const fast_handshake_state_t *handshake_state, out_len = key_out_len+DIGEST_LEN; out = tor_malloc(out_len); if (crypto_expand_key_material_TAP(tmp, sizeof(tmp), out, out_len)) { - log_warn(LD_CIRC, "Failed to expand key material"); + if (msg_out) + *msg_out = "Failed to expand key material"; goto done; } if (tor_memneq(out, handshake_reply_out+DIGEST_LEN, DIGEST_LEN)) { /* H(K) does *not* match. Something fishy. */ - log_warn(LD_PROTOCOL,"Digest DOES NOT MATCH on fast handshake. " - "Bug or attack."); + if (msg_out) + *msg_out = "Digest DOES NOT MATCH on fast handshake. Bug or attack."; goto done; } memcpy(key_out, out+DIGEST_LEN, key_out_len); diff --git a/src/or/onion_fast.h b/src/or/onion_fast.h index 8c07837..e6c213e 100644 --- a/src/or/onion_fast.h +++ b/src/or/onion_fast.h @@ -32,7 +32,8 @@ int fast_server_handshake(const uint8_t *message_in, int fast_client_handshake(const fast_handshake_state_t *handshake_state, const uint8_t *handshake_reply_out, uint8_t *key_out, - size_t key_out_len); + size_t key_out_len, + const char **msg_out);

#endif

diff --git a/src/or/onion_ntor.c b/src/or/onion_ntor.c index ef501f6..32d5344 100644 --- a/src/or/onion_ntor.c +++ b/src/or/onion_ntor.c @@ -3,8 +3,8 @@

#include "orconfig.h"

-#include "crypto.h" #define ONION_NTOR_PRIVATE +#include "crypto.h" #include "onion_ntor.h" #include "torlog.h" #include "util.h" @@ -226,7 +226,8 @@ onion_skin_ntor_client_handshake( const ntor_handshake_state_t *handshake_state, const uint8_t *handshake_reply, uint8_t *key_out, - size_t key_out_len) + size_t key_out_len, + const char **msg_out) { const tweakset_t *T = &proto1_tweaks; /* Sensitive stack-allocated material. Kept in an anonymous struct to make @@ -291,8 +292,18 @@ onion_skin_ntor_client_handshake(

memwipe(&s, 0, sizeof(s));

- if (bad) { - log_warn(LD_PROTOCOL, "Invalid result from curve25519 handshake: %d", bad); + if (bad && msg_out) { + if (bad & 4) { + *msg_out = NULL; /* Don't report this one; we probably just had the + * wrong onion key.*/ + log_fn(LOG_INFO, LD_PROTOCOL, + "Invalid result from curve25519 handshake: %d", bad); + } + if (bad & 3) { + *msg_out = "Zero output from curve25519 handshake"; + log_fn(LOG_WARN, LD_PROTOCOL, + "Invalid result from curve25519 handshake: %d", bad); + } }

return bad ? -1 : 0; diff --git a/src/or/onion_ntor.h b/src/or/onion_ntor.h index c942e6e..3fd6c88 100644 --- a/src/or/onion_ntor.h +++ b/src/or/onion_ntor.h @@ -37,7 +37,8 @@ int onion_skin_ntor_client_handshake( const ntor_handshake_state_t *handshake_state, const uint8_t *handshake_reply, uint8_t *key_out, - size_t key_out_len); + size_t key_out_len, + const char **msg_out);

#ifdef ONION_NTOR_PRIVATE

diff --git a/src/or/onion_tap.c b/src/or/onion_tap.c index 65f8275..668c48c 100644 --- a/src/or/onion_tap.c +++ b/src/or/onion_tap.c @@ -183,7 +183,8 @@ int onion_skin_TAP_client_handshake(crypto_dh_t *handshake_state, const char *handshake_reply, /* TAP_ONIONSKIN_REPLY_LEN bytes */ char *key_out, - size_t key_out_len) + size_t key_out_len, + const char **msg_out) { ssize_t len; char *key_material=NULL; @@ -196,14 +197,15 @@ onion_skin_TAP_client_handshake(crypto_dh_t *handshake_state, handshake_reply, DH_KEY_LEN, key_material, key_material_len); if (len < 0) { - log_warn(LD_PROTOCOL,"DH computation failed."); + if (msg_out) + *msg_out = "DH computation failed."; goto err; }

if (tor_memneq(key_material, handshake_reply+DH_KEY_LEN, DIGEST_LEN)) { /* H(K) does *not* match. Something fishy. */ - log_warn(LD_PROTOCOL,"Digest DOES NOT MATCH on onion handshake. " - "Bug or attack."); + if (msg_out) + *msg_out = "Digest DOES NOT MATCH on onion handshake. Bug or attack."; goto err; }

diff --git a/src/or/onion_tap.h b/src/or/onion_tap.h index b978b66..3b23d62 100644 --- a/src/or/onion_tap.h +++ b/src/or/onion_tap.h @@ -31,7 +31,8 @@ int onion_skin_TAP_server_handshake(const char *onion_skin, int onion_skin_TAP_client_handshake(crypto_dh_t *handshake_state, const char *handshake_reply, char *key_out, - size_t key_out_len); + size_t key_out_len, + const char **msg_out);

#endif

diff --git a/src/or/relay.c b/src/or/relay.c index 9407df0..73961de 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -1643,8 +1643,9 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, } if ((reason = circuit_finish_handshake(TO_ORIGIN_CIRCUIT(circ), &extended_cell.created_cell)) < 0) { - log_warn(domain,"circuit_finish_handshake failed."); - return reason; + circuit_mark_for_close(circ, -reason); + return 0; /* We don't want to cause a warning, so we mark the circuit + * here. */ } } if ((reason=circuit_send_next_onion_skin(TO_ORIGIN_CIRCUIT(circ)))<0) { diff --git a/src/test/bench.c b/src/test/bench.c index f6c3362..fd374b5 100644 --- a/src/test/bench.c +++ b/src/test/bench.c @@ -162,7 +162,8 @@ bench_onion_TAP(void) char key_out[CPATH_KEY_MATERIAL_LEN]; int s; dh = crypto_dh_dup(dh_out); - s = onion_skin_TAP_client_handshake(dh, or, key_out, sizeof(key_out)); + s = onion_skin_TAP_client_handshake(dh, or, key_out, sizeof(key_out), + NULL); crypto_dh_free(dh); tor_assert(s == 0); } @@ -222,7 +223,8 @@ bench_onion_ntor(void) for (i = 0; i < iters; ++i) { uint8_t key_out[CPATH_KEY_MATERIAL_LEN]; int s; - s = onion_skin_ntor_client_handshake(state, or, key_out, sizeof(key_out)); + s = onion_skin_ntor_client_handshake(state, or, key_out, sizeof(key_out), + NULL); tor_assert(s == 0); } end = perftime(); diff --git a/src/test/test.c b/src/test/test.c index 8bce9c9..e6f79a6 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -264,7 +264,8 @@ test_onion_handshake(void)

/* client handshake 2 */ memset(c_keys, 0, 40); - test_assert(! onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40)); + test_assert(! onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40, + NULL));

test_memeq(c_keys, s_keys, 40); memset(s_buf, 0, 40); @@ -337,18 +338,18 @@ test_bad_onion_handshake(void *arg) /* Client: Case 1: The server sent back junk. */ s_buf[64] ^= 33; tt_int_op(-1, ==, - onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40)); + onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40, NULL)); s_buf[64] ^= 33;

/* Let the client finish; make sure it can. */ tt_int_op(0, ==, - onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40)); + onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40, NULL)); test_memeq(s_keys, c_keys, 40);

/* Client: Case 2: The server sent back a degenerate DH. */ memset(s_buf, 0, sizeof(s_buf)); tt_int_op(-1, ==, - onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40)); + onion_skin_TAP_client_handshake(c_dh, s_buf, c_keys, 40, NULL));

done: crypto_dh_free(c_dh); @@ -398,7 +399,7 @@ test_ntor_handshake(void *arg) /* client handshake 2 */ memset(c_keys, 0, 40); tt_int_op(0, ==, onion_skin_ntor_client_handshake(c_state, s_buf, - c_keys, 400)); + c_keys, 400, NULL));

test_memeq(c_keys, s_keys, 400); memset(s_buf, 0, 40); diff --git a/src/test/test_ntor_cl.c b/src/test/test_ntor_cl.c index f2b7a72..6103e80 100644 --- a/src/test/test_ntor_cl.c +++ b/src/test/test_ntor_cl.c @@ -130,7 +130,7 @@ client2(int argc, char **argv)

keys = tor_malloc(keybytes); hexkeys = tor_malloc(keybytes*2+1); - if (onion_skin_ntor_client_handshake(&state, msg, keys, keybytes)<0) { + if (onion_skin_ntor_client_handshake(&state, msg, keys, keybytes, NULL)<0) { fprintf(stderr, "handshake failed"); result = 2; goto done;