From a561a10da726b426d326515ca7f75988b405bab7 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Thu, 3 Aug 2017 16:04:25 +0300 Subject: [PATCH] Fix small easy bugs all around - Fix log message format string. - Do extra circuit purpose check. - wipe memory in a clear function - Make sure we don't double add intro points in our list - Make sure we don't double close intro circuits. - s/tt_u64_op/tt_i64_op/ --- src/or/hs_cell.c | 4 ++-- src/or/hs_circuit.c | 4 ++++ src/or/hs_intropoint.c | 1 + src/or/hs_service.c | 12 ++++++++---- src/test/test_hs_cell.c | 2 +- 5 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/or/hs_cell.c b/src/or/hs_cell.c index 922ff73468..064e26334d 100644 --- a/src/or/hs_cell.c +++ b/src/or/hs_cell.c @@ -169,9 +169,9 @@ parse_introduce2_encrypted(const uint8_t *decrypted_data, if (trn_cell_introduce_encrypted_getlen_onion_key(enc_cell) != CURVE25519_PUBKEY_LEN) { - log_info(LD_REND, "INTRODUCE2 onion key length is invalid. Got %ld but " + log_info(LD_REND, "INTRODUCE2 onion key length is invalid. Got %u but " "expected %d on circuit %u for service %s", - trn_cell_introduce_encrypted_getlen_onion_key(enc_cell), + (unsigned)trn_cell_introduce_encrypted_getlen_onion_key(enc_cell), CURVE25519_PUBKEY_LEN, TO_CIRCUIT(circ)->n_circ_id, safe_str_client(service->onion_address)); goto err; diff --git a/src/or/hs_circuit.c b/src/or/hs_circuit.c index 75c946799f..527439599a 100644 --- a/src/or/hs_circuit.c +++ b/src/or/hs_circuit.c @@ -898,6 +898,10 @@ hs_circ_handle_intro_established(const hs_service_t *service, tor_assert(circ); tor_assert(payload); + if (BUG(TO_CIRCUIT(circ)->purpose != CIRCUIT_PURPOSE_S_ESTABLISH_INTRO)) { + goto done; + } + /* Try to parse the payload into a cell making sure we do actually have a * valid cell. For a legacy node, it's an empty payload so as long as we * have the cell, we are good. */ diff --git a/src/or/hs_intropoint.c b/src/or/hs_intropoint.c index a0453841f9..644611feab 100644 --- a/src/or/hs_intropoint.c +++ b/src/or/hs_intropoint.c @@ -607,5 +607,6 @@ hs_intropoint_clear(hs_intropoint_t *ip) SMARTLIST_FOREACH(ip->link_specifiers, hs_desc_link_specifier_t *, ls, hs_desc_link_specifier_free(ls)); smartlist_free(ip->link_specifiers); + memset(ip, 0, sizeof(hs_intropoint_t)); } diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 592b1f2552..f1592efa81 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -433,14 +433,18 @@ service_intro_point_new(const extend_info_t *ei, unsigned int is_legacy) STATIC void service_intro_point_add(digest256map_t *map, hs_service_intro_point_t *ip) { + hs_service_intro_point_t *old_ip_entry; + tor_assert(map); tor_assert(ip); - digest256map_set(map, ip->auth_key_kp.pubkey.pubkey, ip); + old_ip_entry = digest256map_set(map, ip->auth_key_kp.pubkey.pubkey, ip); + /* Make sure we didn't just try to double-add an intro point */ + tor_assert_nonfatal(!old_ip_entry); } -/* For a given service, remove the intro point from that service which will - * look in both descriptors. */ +/* For a given service, remove the intro point from that service's descriptors + * (check both current and next descriptor) */ STATIC void service_intro_point_remove(const hs_service_t *service, const hs_service_intro_point_t *ip) @@ -1623,7 +1627,7 @@ cleanup_intro_points(hs_service_t *service, time_t now) * descriptor created and uploaded. There is no difference to an * attacker between the timing of a new consensus and intro point * rotation (possibly?). */ - if (ocirc) { + if (ocirc && !TO_CIRCUIT(ocirc)->marked_for_close) { /* After this, no new cells will be handled on the circuit. */ circuit_mark_for_close(TO_CIRCUIT(ocirc), END_CIRC_REASON_FINISHED); } diff --git a/src/test/test_hs_cell.c b/src/test/test_hs_cell.c index 9c963bcb16..1686fccee7 100644 --- a/src/test/test_hs_cell.c +++ b/src/test/test_hs_cell.c @@ -106,7 +106,7 @@ test_gen_establish_intro_cell_bad(void *arg) expect_log_msg_containing("Unable to make signature for " "ESTABLISH_INTRO cell."); teardown_capture_of_logs(); - tt_u64_op(cell_len, OP_EQ, -1); + tt_i64_op(cell_len, OP_EQ, -1); done: trn_cell_establish_intro_free(cell);