From a42131bf4823bed42563f30d4925462b2df02c27 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 5 Jun 2019 11:37:32 -0700 Subject: [PATCH 1/3] Revert "test: Add test_hs_circ.c for HS circuit testing" This reverts commit 41b94722e5c93ec06911f9c63296a65ce295c1ea. --- src/test/include.am | 1 - src/test/test.c | 1 - src/test/test.h | 1 - src/test/test_hs_circ.c | 70 ----------------------------------------- 4 files changed, 73 deletions(-) delete mode 100644 src/test/test_hs_circ.c diff --git a/src/test/include.am b/src/test/include.am index fdfe96c559..85f9c9f880 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -145,7 +145,6 @@ src_test_test_SOURCES += \ src/test/test_hs_common.c \ src/test/test_hs_config.c \ src/test/test_hs_cell.c \ - src/test/test_hs_circ.c \ src/test/test_hs_ntor.c \ src/test/test_hs_service.c \ src/test/test_hs_client.c \ diff --git a/src/test/test.c b/src/test/test.c index d7682b6619..cac98dd839 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -871,7 +871,6 @@ struct testgroup_t testgroups[] = { { "guardfraction/", guardfraction_tests }, { "hs_cache/", hs_cache }, { "hs_cell/", hs_cell_tests }, - { "hs_circ/", hs_circ_tests }, { "hs_client/", hs_client_tests }, { "hs_common/", hs_common_tests }, { "hs_config/", hs_config_tests }, diff --git a/src/test/test.h b/src/test/test.h index b108d795ea..167fd090ac 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -221,7 +221,6 @@ extern struct testcase_t guardfraction_tests[]; extern struct testcase_t handle_tests[]; extern struct testcase_t hs_cache[]; extern struct testcase_t hs_cell_tests[]; -extern struct testcase_t hs_circ_tests[]; extern struct testcase_t hs_client_tests[]; extern struct testcase_t hs_common_tests[]; extern struct testcase_t hs_config_tests[]; diff --git a/src/test/test_hs_circ.c b/src/test/test_hs_circ.c deleted file mode 100644 index af28af2573..0000000000 --- a/src/test/test_hs_circ.c +++ /dev/null @@ -1,70 +0,0 @@ -/* Copyright (c) 2017-2019, The Tor Project, Inc. */ -/* See LICENSE for licensing information */ - -/** - * \file test_hs_circ.c - * \brief Test hidden service circuit functionality. - */ - -#define CIRCUITLIST_PRIVATE - -#include "test/test.h" -#include "test/test_helpers.h" -#include "test/log_test_helpers.h" - -#include "core/or/circuitbuild.h" -#include "core/or/circuitlist.h" -#include "core/or/circuituse.h" -#include "core/or/origin_circuit_st.h" - -#include "feature/hs/hs_circuit.h" -#include "feature/hs/hs_circuitmap.h" - -static void -test_circuit_repurpose(void *arg) -{ - origin_circuit_t *intro_circ = NULL; - const origin_circuit_t *search; - ed25519_keypair_t kp; - - (void) arg; - - hs_init(); - - intro_circ = origin_circuit_init(CIRCUIT_PURPOSE_S_ESTABLISH_INTRO, 0); - tt_assert(intro_circ); - ed25519_keypair_generate(&kp, 0); - - /* Register circuit in global map and make sure it is actually there. */ - hs_circuitmap_register_intro_circ_v3_service_side(intro_circ, - &kp.pubkey); - tt_assert(TO_CIRCUIT(intro_circ)->hs_token); - search = hs_circuitmap_get_intro_circ_v3_service_side(&kp.pubkey); - tt_mem_op(search, OP_EQ, intro_circ, sizeof(origin_circuit_t)); - - /* Setup circuit HS ident. We don't care about the service pubkey. */ - intro_circ->hs_ident = hs_ident_circuit_new(&kp.pubkey, - HS_IDENT_CIRCUIT_INTRO); - tt_assert(intro_circ->hs_ident); - - /* Trigger a repurpose. State should be cleaned up. */ - hs_circ_repurpose(TO_CIRCUIT(intro_circ)); - - /* Removed from map. */ - search = hs_circuitmap_get_intro_circ_v3_service_side(&kp.pubkey); - tt_assert(!search); - /* HS identifier has been removed. */ - tt_assert(!intro_circ->hs_ident); - - done: - circuit_free_(TO_CIRCUIT(intro_circ)); - hs_free_all(); -} - -struct testcase_t hs_circ_tests[] = { - { "repurpose", test_circuit_repurpose, TT_FORK, - NULL, NULL }, - - END_OF_TESTCASES -}; - From 31c34f6524fecd6a104a8dac04d9330b5323e3c0 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 5 Jun 2019 11:38:01 -0700 Subject: [PATCH 2/3] Revert "hs: Implement a helper to repurpose a circuit" This reverts commit 3789f22bcbfbc6de415a838e4c4bfb2555c7d6c3. --- src/core/or/circuituse.c | 6 ------ src/feature/hs/hs_circuit.c | 31 ------------------------------- src/feature/hs/hs_circuit.h | 1 - src/feature/rend/rendcommon.c | 11 ----------- src/feature/rend/rendcommon.h | 2 -- 5 files changed, 51 deletions(-) diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index b3d5c6bb81..485c389054 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -3068,12 +3068,6 @@ circuit_change_purpose(circuit_t *circ, uint8_t new_purpose) if (circ->purpose == new_purpose) return; - /* Take specific actions if we are repurposing a hidden service circuit. */ - if (circuit_purpose_is_hidden_service(circ->purpose) && - !circuit_purpose_is_hidden_service(new_purpose)) { - hs_circ_repurpose(circ); - } - if (CIRCUIT_IS_ORIGIN(circ)) { char old_purpose_desc[80] = ""; diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index 79377eb731..a6e86c5ab3 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -25,7 +25,6 @@ #include "feature/nodelist/describe.h" #include "feature/nodelist/nodelist.h" #include "feature/rend/rendservice.h" -#include "feature/rend/rendcommon.h" #include "feature/stats/rephist.h" #include "lib/crypt_ops/crypto_dh.h" #include "lib/crypt_ops/crypto_rand.h" @@ -1193,33 +1192,3 @@ hs_circ_cleanup(circuit_t *circ) hs_circuitmap_remove_circuit(circ); } } - -/* The given circuit will be repurposed so take the appropriate actions. A - * cleanup from the HS maps and of all HS related structures is done. - * - * Once this function returns, the circuit can be safely repurposed. */ -void -hs_circ_repurpose(circuit_t *circ) -{ - origin_circuit_t *origin_circ; - - tor_assert(circ); - - /* Only repurposing an origin circuit is possible for HS. */ - if (!CIRCUIT_IS_ORIGIN(circ)) { - return; - } - origin_circ = TO_ORIGIN_CIRCUIT(circ); - - /* First, cleanup the circuit from the HS maps. */ - hs_circ_cleanup(circ); - - /* Depending on the version, different cleanup is done. */ - if (origin_circ->rend_data) { - /* v2. */ - rend_circ_cleanup(origin_circ); - } else if (origin_circ->hs_ident) { - /* v3. */ - hs_ident_circuit_free(origin_circ->hs_ident); - } -} diff --git a/src/feature/hs/hs_circuit.h b/src/feature/hs/hs_circuit.h index 0786f3ee45..b8d8b25add 100644 --- a/src/feature/hs/hs_circuit.h +++ b/src/feature/hs/hs_circuit.h @@ -16,7 +16,6 @@ /* Cleanup function when the circuit is closed or/and freed. */ void hs_circ_cleanup(circuit_t *circ); -void hs_circ_repurpose(circuit_t *circ); /* Circuit API. */ int hs_circ_service_intro_has_opened(hs_service_t *service, diff --git a/src/feature/rend/rendcommon.c b/src/feature/rend/rendcommon.c index 265ee368f1..777de2984c 100644 --- a/src/feature/rend/rendcommon.c +++ b/src/feature/rend/rendcommon.c @@ -1046,14 +1046,3 @@ rend_circuit_pk_digest_eq(const origin_circuit_t *ocirc, match: return 1; } - -/* Cleanup the given circuit of all HS v2 data structure. */ -void -rend_circ_cleanup(origin_circuit_t *circ) -{ - tor_assert(circ); - - /* Both fields are set to NULL with these. */ - crypto_pk_free(circ->intro_key); - rend_data_free(circ->rend_data); -} diff --git a/src/feature/rend/rendcommon.h b/src/feature/rend/rendcommon.h index c9a04846d7..f136863c7a 100644 --- a/src/feature/rend/rendcommon.h +++ b/src/feature/rend/rendcommon.h @@ -71,8 +71,6 @@ int rend_non_anonymous_mode_enabled(const or_options_t *options); void assert_circ_anonymity_ok(const origin_circuit_t *circ, const or_options_t *options); -void rend_circ_cleanup(origin_circuit_t *circ); - #ifdef RENDCOMMON_PRIVATE STATIC int From c525135dac354892a45ad3d2f6de9450d393f09f Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 5 Jun 2019 11:50:44 -0700 Subject: [PATCH 3/3] Bug 29034: Cleanup hs circuitmap when purpose changes. Leave the other rend and hs_ident data around until circuit free, since code may still try to inspect it after marking the circuit for close. The circuitmap is the important thing to clean up, since repurposed intropoints must be removed from this map to ensure validity. --- changes/bug29034 | 5 +++++ src/core/or/circuituse.c | 6 ++++++ 2 files changed, 11 insertions(+) create mode 100644 changes/bug29034 diff --git a/changes/bug29034 b/changes/bug29034 new file mode 100644 index 0000000000..e7aa9af00b --- /dev/null +++ b/changes/bug29034 @@ -0,0 +1,5 @@ + o Major bugfixes (Onion service reachability): + - Properly clean up the introduction point map when circuits change purpose + from onion service circuits to pathbias, measurement, or other circuit types. + This should fix some service-side instances of introduction point failure. + Fixes bug 29034; bugfix on 0.3.2.1-alpha. diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index 485c389054..18b419e99d 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -3082,6 +3082,12 @@ circuit_change_purpose(circuit_t *circ, uint8_t new_purpose) circ->purpose, circuit_purpose_to_string(new_purpose), new_purpose); + + /* Take specific actions if we are repurposing a hidden service circuit. */ + if (circuit_purpose_is_hidden_service(circ->purpose) && + !circuit_purpose_is_hidden_service(new_purpose)) { + hs_circ_cleanup(circ); + } } old_purpose = circ->purpose;