December 2017 - tor-commits - lists.torproject.org

[tor/master] Merge remote-tracking branch 'ffmancera/github/bug23459'
by nickm＠torproject.org 11 Dec '17

11 Dec '17

commit 7803b19c43b02596c3393667ab1cba9571347823 Merge: 030d3d52f 855982728 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Dec 11 09:53:39 2017 -0500 Merge remote-tracking branch 'ffmancera/github/bug23459' changes/ticket23459 | 4 ++++ src/or/hs_circuitmap.c | 36 ++++++++++++++++++++++++++++++------ src/or/hs_circuitmap.h | 2 ++ src/or/hs_client.c | 3 ++- 4 files changed, 38 insertions(+), 7 deletions(-)

1 0

[tor/master] Fix compilation of 23459 patch.
by nickm＠torproject.org 11 Dec '17

11 Dec '17

commit c2822bbc63e4ba0dcd4f8939b03a5fe0d9a7031a Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Dec 11 09:55:38 2017 -0500 Fix compilation of 23459 patch. The free() changes of 23447 had broken the pending 23459 patch. --- src/test/test_hs_service.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 28830e903..73200a5a6 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -825,7 +825,7 @@ test_closing_intro_circs(void *arg) /* Now pretend we are freeing this intro circuit. We want to see that our * destructor is not gonna kill our intro point structure since that's the * job of the cleanup routine. */ - circuit_free(TO_CIRCUIT(intro_circ)); + circuit_free_(TO_CIRCUIT(intro_circ)); intro_circ = NULL; entry = service_intro_point_find(service, &ip->auth_key_kp.pubkey); tt_assert(entry); @@ -857,7 +857,7 @@ test_closing_intro_circs(void *arg) done: if (intro_circ) { - circuit_free(TO_CIRCUIT(intro_circ)); + circuit_free_(TO_CIRCUIT(intro_circ)); } /* Frees the service object. */ hs_free_all();

1 0

[tor/master] Merge remote-tracking branch 'teor/comment24575'
by nickm＠torproject.org 11 Dec '17

11 Dec '17

commit 030d3d52f5bf03d2e611c7f208a56b3e45d47210 Merge: 98682f689 35055cfa7 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Dec 11 09:46:23 2017 -0500 Merge remote-tracking branch 'teor/comment24575' src/or/config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

1 0

[tor/master] Update the comment in options_transition_affects_guards()
by nickm＠torproject.org 11 Dec '17

11 Dec '17

commit 35055cfa7fcf11e3c5fae91368d4207d8d9b7333 Author: teor <teor2345(a)gmail.com> Date: Mon Dec 11 03:18:48 2017 +1100 Update the comment in options_transition_affects_guards() It referred to the an old function name. Fixes #24575, bugfix on 0.3.0. --- src/or/config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/config.c b/src/or/config.c index bcfac1dae..23184af31 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1685,7 +1685,7 @@ options_transition_affects_guards(const or_options_t *old, const or_options_t *new) { /* NOTE: Make sure this function stays in sync with - * entry_guards_set_filtered_flags */ + * node_passes_guard_filter */ tor_assert(old); tor_assert(new);

1 0

[tor/maint-0.3.2] hs-v3: Don't cleanup intro point in has_closed()
by nickm＠torproject.org 11 Dec '17

11 Dec '17

commit ba63c4099a491c3f96160005f94cf3a61433c145 Author: David Goulet <dgoulet(a)torproject.org> Date: Mon Sep 25 16:47:16 2017 -0400 hs-v3: Don't cleanup intro point in has_closed() The hs_service_intro_circ_has_closed() was removing intro point objects if too many retries. We shouldn't cleanup those objects in that function at all but rather let cleanup_intro_points() do its job and clean it properly. This was causing an issue in #23603. Furthermore, this moves the logic of remembering failing intro points in the cleanup_intro_points() function which should really be the only function to know when to cleanup and thus when an introduction point should be remembered as a failed one. Fixes #23603 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/hs_service.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index a2082b391..99965ddc3 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -1839,6 +1839,12 @@ cleanup_intro_points(hs_service_t *service, time_t now) (node == NULL) ? " fell off the consensus" : "", ip->circuit_retries); + /* We've retried too many times, remember it as a failed intro point + * so we don't pick it up again for INTRO_CIRC_RETRY_PERIOD sec. */ + if (ip->circuit_retries > MAX_INTRO_POINT_CIRCUIT_RETRIES) { + remember_failing_intro_point(ip, desc, approx_time()); + } + /* Remove intro point from descriptor map. We'll add it to the failed * map if we retried it too many times. */ MAP_DEL_CURRENT(key); @@ -2946,15 +2952,6 @@ hs_service_intro_circ_has_closed(origin_circuit_t *circ) * keeping the object in the descriptor, we'll be able to retry. */ ip->circuit_established = 0; - /* We've retried too many times, remember it as a failed intro point so we - * don't pick it up again. It will be retried in INTRO_CIRC_RETRY_PERIOD - * seconds. */ - if (ip->circuit_retries > MAX_INTRO_POINT_CIRCUIT_RETRIES) { - remember_failing_intro_point(ip, desc, approx_time()); - service_intro_point_remove(service, ip); - service_intro_point_free(ip); - } - end: return; }

1 0

[tor/master] test: Add HSv3 unit test for expiring intro point
by nickm＠torproject.org 11 Dec '17

11 Dec '17

commit 68601a85ca48bf2cd54087fe21fdcf8aaa23ebb2 Author: David Goulet <dgoulet(a)torproject.org> Date: Mon Dec 4 11:45:41 2017 -0500 test: Add HSv3 unit test for expiring intro point Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/circuitlist.c | 4 +-- src/or/circuitlist.h | 2 +- src/test/test_hs_service.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 3 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 45d4521c2..d442887c9 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -2481,8 +2481,8 @@ assert_cpath_ok(const crypt_path_t *cp) /** Verify that circuit c has all of its invariants * correct. Trigger an assert if anything is invalid. */ -void -assert_circuit_ok(const circuit_t *c) +MOCK_IMPL(void, +assert_circuit_ok,(const circuit_t *c)) { edge_connection_t *conn; const or_circuit_t *or_circ = NULL; diff --git a/src/or/circuitlist.h b/src/or/circuitlist.h index 5d0da15ca..4190c1f82 100644 --- a/src/or/circuitlist.h +++ b/src/or/circuitlist.h @@ -68,7 +68,7 @@ int circuit_count_pending_on_channel(channel_t *chan); circuit_mark_for_close_((c), (reason), __LINE__, SHORT_FILE__) void assert_cpath_layer_ok(const crypt_path_t *cp); -void assert_circuit_ok(const circuit_t *c); +MOCK_DECL(void, assert_circuit_ok,(const circuit_t *c)); void circuit_free_all(void); void circuits_handle_oom(size_t current_allocation); diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 3084c6b95..8407eccfa 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -778,6 +778,92 @@ test_rdv_circuit_opened(void *arg) UNMOCK(relay_send_command_from_edge_); } +static void +mock_assert_circuit_ok(const circuit_t *c) +{ + (void) c; + return; +} + +/** Test for the general mechanism for closing intro circs. + * Also a way to identify that #23603 has been fixed. */ +static void +test_closing_intro_circs(void *arg) +{ + hs_service_t *service = NULL; + hs_service_intro_point_t *ip = NULL, *entry = NULL; + origin_circuit_t *intro_circ = NULL, *tmp_circ; + int flags = CIRCLAUNCH_NEED_UPTIME | CIRCLAUNCH_IS_INTERNAL; + + (void) arg; + + MOCK(assert_circuit_ok, mock_assert_circuit_ok); + + hs_init(); + + /* Initialize service */ + service = helper_create_service(); + /* Initialize intro point */ + ip = helper_create_service_ip(); + tt_assert(ip); + service_intro_point_add(service->desc_current->intro_points.map, ip); + + /* Initialize intro circuit */ + intro_circ = origin_circuit_init(CIRCUIT_PURPOSE_S_ESTABLISH_INTRO, flags); + intro_circ->hs_ident = hs_ident_circuit_new(&service->keys.identity_pk, + HS_IDENT_CIRCUIT_INTRO); + /* Register circuit in the circuitmap . */ + hs_circuitmap_register_intro_circ_v3_service_side(intro_circ, + &ip->auth_key_kp.pubkey); + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_ptr_op(tmp_circ, OP_EQ, intro_circ); + + /* Pretend that intro point has failed too much */ + ip->circuit_retries = MAX_INTRO_POINT_CIRCUIT_RETRIES+1; + + /* Now pretend we are freeing this intro circuit. We want to see that our + * destructor is not gonna kill our intro point structure since that's the + * job of the cleanup routine. */ + circuit_free(TO_CIRCUIT(intro_circ)); + intro_circ = NULL; + entry = service_intro_point_find(service, &ip->auth_key_kp.pubkey); + tt_assert(entry); + /* The free should also remove the circuit from the circuitmap. */ + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_assert(!tmp_circ); + + /* Now pretend that a new intro point circ was launched and opened. Check + * that the intro point will be established correctly. */ + intro_circ = origin_circuit_init(CIRCUIT_PURPOSE_S_ESTABLISH_INTRO, flags); + intro_circ->hs_ident = hs_ident_circuit_new(&service->keys.identity_pk, + HS_IDENT_CIRCUIT_INTRO); + ed25519_pubkey_copy(&intro_circ->hs_ident->intro_auth_pk, + &ip->auth_key_kp.pubkey); + /* Register circuit in the circuitmap . */ + hs_circuitmap_register_intro_circ_v3_service_side(intro_circ, + &ip->auth_key_kp.pubkey); + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_ptr_op(tmp_circ, OP_EQ, intro_circ); + tt_int_op(TO_CIRCUIT(intro_circ)->marked_for_close, OP_EQ, 0); + circuit_mark_for_close(TO_CIRCUIT(intro_circ), END_CIRC_REASON_INTERNAL); + tt_int_op(TO_CIRCUIT(intro_circ)->marked_for_close, OP_NE, 0); + /* At this point, we should not be able to find it in the circuitmap. */ + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_assert(!tmp_circ); + + done: + if (intro_circ) { + circuit_free(TO_CIRCUIT(intro_circ)); + } + /* Frees the service object. */ + hs_free_all(); + UNMOCK(assert_circuit_ok); +} + /** Test sending and receiving introduce2 cells */ static void test_introduce2(void *arg) @@ -1512,6 +1598,8 @@ struct testcase_t hs_service_tests[] = { NULL, NULL }, { "intro_established", test_intro_established, TT_FORK, NULL, NULL }, + { "closing_intro_circs", test_closing_intro_circs, TT_FORK, + NULL, NULL }, { "rdv_circuit_opened", test_rdv_circuit_opened, TT_FORK, NULL, NULL }, { "introduce2", test_introduce2, TT_FORK,

1 0

[tor/maint-0.3.2] test: Add HSv3 unit test for expiring intro point
by nickm＠torproject.org 11 Dec '17

11 Dec '17

commit 68601a85ca48bf2cd54087fe21fdcf8aaa23ebb2 Author: David Goulet <dgoulet(a)torproject.org> Date: Mon Dec 4 11:45:41 2017 -0500 test: Add HSv3 unit test for expiring intro point Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/circuitlist.c | 4 +-- src/or/circuitlist.h | 2 +- src/test/test_hs_service.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 3 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 45d4521c2..d442887c9 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -2481,8 +2481,8 @@ assert_cpath_ok(const crypt_path_t *cp) /** Verify that circuit c has all of its invariants * correct. Trigger an assert if anything is invalid. */ -void -assert_circuit_ok(const circuit_t *c) +MOCK_IMPL(void, +assert_circuit_ok,(const circuit_t *c)) { edge_connection_t *conn; const or_circuit_t *or_circ = NULL; diff --git a/src/or/circuitlist.h b/src/or/circuitlist.h index 5d0da15ca..4190c1f82 100644 --- a/src/or/circuitlist.h +++ b/src/or/circuitlist.h @@ -68,7 +68,7 @@ int circuit_count_pending_on_channel(channel_t *chan); circuit_mark_for_close_((c), (reason), __LINE__, SHORT_FILE__) void assert_cpath_layer_ok(const crypt_path_t *cp); -void assert_circuit_ok(const circuit_t *c); +MOCK_DECL(void, assert_circuit_ok,(const circuit_t *c)); void circuit_free_all(void); void circuits_handle_oom(size_t current_allocation); diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 3084c6b95..8407eccfa 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -778,6 +778,92 @@ test_rdv_circuit_opened(void *arg) UNMOCK(relay_send_command_from_edge_); } +static void +mock_assert_circuit_ok(const circuit_t *c) +{ + (void) c; + return; +} + +/** Test for the general mechanism for closing intro circs. + * Also a way to identify that #23603 has been fixed. */ +static void +test_closing_intro_circs(void *arg) +{ + hs_service_t *service = NULL; + hs_service_intro_point_t *ip = NULL, *entry = NULL; + origin_circuit_t *intro_circ = NULL, *tmp_circ; + int flags = CIRCLAUNCH_NEED_UPTIME | CIRCLAUNCH_IS_INTERNAL; + + (void) arg; + + MOCK(assert_circuit_ok, mock_assert_circuit_ok); + + hs_init(); + + /* Initialize service */ + service = helper_create_service(); + /* Initialize intro point */ + ip = helper_create_service_ip(); + tt_assert(ip); + service_intro_point_add(service->desc_current->intro_points.map, ip); + + /* Initialize intro circuit */ + intro_circ = origin_circuit_init(CIRCUIT_PURPOSE_S_ESTABLISH_INTRO, flags); + intro_circ->hs_ident = hs_ident_circuit_new(&service->keys.identity_pk, + HS_IDENT_CIRCUIT_INTRO); + /* Register circuit in the circuitmap . */ + hs_circuitmap_register_intro_circ_v3_service_side(intro_circ, + &ip->auth_key_kp.pubkey); + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_ptr_op(tmp_circ, OP_EQ, intro_circ); + + /* Pretend that intro point has failed too much */ + ip->circuit_retries = MAX_INTRO_POINT_CIRCUIT_RETRIES+1; + + /* Now pretend we are freeing this intro circuit. We want to see that our + * destructor is not gonna kill our intro point structure since that's the + * job of the cleanup routine. */ + circuit_free(TO_CIRCUIT(intro_circ)); + intro_circ = NULL; + entry = service_intro_point_find(service, &ip->auth_key_kp.pubkey); + tt_assert(entry); + /* The free should also remove the circuit from the circuitmap. */ + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_assert(!tmp_circ); + + /* Now pretend that a new intro point circ was launched and opened. Check + * that the intro point will be established correctly. */ + intro_circ = origin_circuit_init(CIRCUIT_PURPOSE_S_ESTABLISH_INTRO, flags); + intro_circ->hs_ident = hs_ident_circuit_new(&service->keys.identity_pk, + HS_IDENT_CIRCUIT_INTRO); + ed25519_pubkey_copy(&intro_circ->hs_ident->intro_auth_pk, + &ip->auth_key_kp.pubkey); + /* Register circuit in the circuitmap . */ + hs_circuitmap_register_intro_circ_v3_service_side(intro_circ, + &ip->auth_key_kp.pubkey); + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_ptr_op(tmp_circ, OP_EQ, intro_circ); + tt_int_op(TO_CIRCUIT(intro_circ)->marked_for_close, OP_EQ, 0); + circuit_mark_for_close(TO_CIRCUIT(intro_circ), END_CIRC_REASON_INTERNAL); + tt_int_op(TO_CIRCUIT(intro_circ)->marked_for_close, OP_NE, 0); + /* At this point, we should not be able to find it in the circuitmap. */ + tmp_circ = + hs_circuitmap_get_intro_circ_v3_service_side(&ip->auth_key_kp.pubkey); + tt_assert(!tmp_circ); + + done: + if (intro_circ) { + circuit_free(TO_CIRCUIT(intro_circ)); + } + /* Frees the service object. */ + hs_free_all(); + UNMOCK(assert_circuit_ok); +} + /** Test sending and receiving introduce2 cells */ static void test_introduce2(void *arg) @@ -1512,6 +1598,8 @@ struct testcase_t hs_service_tests[] = { NULL, NULL }, { "intro_established", test_intro_established, TT_FORK, NULL, NULL }, + { "closing_intro_circs", test_closing_intro_circs, TT_FORK, + NULL, NULL }, { "rdv_circuit_opened", test_rdv_circuit_opened, TT_FORK, NULL, NULL }, { "introduce2", test_introduce2, TT_FORK,

1 0

[tor/maint-0.3.2] Merge remote-tracking branch 'dgoulet/bug23603_032_02' into maint-0.3.2
by nickm＠torproject.org 11 Dec '17

11 Dec '17

commit d68abbe358af41b18c9c54444e49689e2626328d Merge: 4b41ffa64 68601a85c Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Dec 11 09:42:12 2017 -0500 Merge remote-tracking branch 'dgoulet/bug23603_032_02' into maint-0.3.2 changes/bug23603 | 7 ++++ src/or/circuitlist.c | 30 ++++++++-------- src/or/circuitlist.h | 2 +- src/or/hs_circuit.c | 28 +++++++++++++++ src/or/hs_circuit.h | 3 ++ src/or/hs_common.c | 1 + src/or/hs_common.h | 2 ++ src/or/hs_service.c | 15 ++++---- src/test/test_hs_service.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++ 9 files changed, 151 insertions(+), 25 deletions(-)

1 0

[tor/maint-0.3.2] hs-v3: Cleanup HS circuits when marking as closed
by nickm＠torproject.org 11 Dec '17

11 Dec '17

commit e80893e51b0c0320838cbed8c46fd5b0fe608bef Author: David Goulet <dgoulet(a)torproject.org> Date: Mon Dec 4 11:49:48 2017 -0500 hs-v3: Cleanup HS circuits when marking as closed First, hs_service_intro_circ_has_closed() is now called in circuit_mark_for close() because the HS subsystem needs to learn when an intro point is actually not established anymore as soon as possible. There is a time window between a close and a free. Second, when we mark for close, we also remove it from the circuitmap because between the close and the free, a service can launch an new circuit to that same intro point and thus register it which only succeeds if the intro point authentication key is not already in the map. However, we still do a remove from the circuitmap in circuit_free() in order to also cleanup the circuit if it wasn't marked for close prior to the free. Fixes #23603 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/bug23603 | 7 +++++++ src/or/circuitlist.c | 26 +++++++++++++------------- src/or/hs_circuit.c | 28 ++++++++++++++++++++++++++++ src/or/hs_circuit.h | 3 +++ src/or/hs_common.c | 1 + src/or/hs_common.h | 2 ++ 6 files changed, 54 insertions(+), 13 deletions(-) diff --git a/changes/bug23603 b/changes/bug23603 new file mode 100644 index 000000000..dfb2052c9 --- /dev/null +++ b/changes/bug23603 @@ -0,0 +1,7 @@ + o Minor bugfixes (hidden service v3): + - Fix a race between the circuit close and free where the service would + launch a new intro circuit after the close, and then fail to register it + before the free of the previously closed circuit. This was making the + service unable to find the established intro circuit and thus not upload + its descriptor. It can make a service unavailable for up to 24 hours. + Fixes bug 23603; bugfix on 0.3.2.1-alpha. diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index d37d986f1..45d4521c2 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -67,7 +67,6 @@ #include "main.h" #include "hs_circuit.h" #include "hs_circuitmap.h" -#include "hs_common.h" #include "hs_ident.h" #include "networkstatus.h" #include "nodelist.h" @@ -938,6 +937,12 @@ circuit_free(circuit_t *circ) circuit_clear_testing_cell_stats(circ); + /* Cleanup circuit from anything HS v3 related. We also do this when the + * circuit is closed. This is to avoid any code path that free registered + * circuits without closing them before. This needs to be done before the + * hs identifier is freed. */ + hs_circ_cleanup(circ); + if (CIRCUIT_IS_ORIGIN(circ)) { origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); mem = ocirc; @@ -963,7 +968,11 @@ circuit_free(circuit_t *circ) crypto_pk_free(ocirc->intro_key); rend_data_free(ocirc->rend_data); + + /* Finally, free the identifier of the circuit and nullify it so multiple + * cleanup will work. */ hs_ident_circuit_free(ocirc->hs_ident); + ocirc->hs_ident = NULL; tor_free(ocirc->dest_address); if (ocirc->socks_username) { @@ -1022,11 +1031,6 @@ circuit_free(circuit_t *circ) /* Remove from map. */ circuit_set_n_circid_chan(circ, 0, NULL); - /* Clear HS circuitmap token from this circ (if any) */ - if (circ->hs_token) { - hs_circuitmap_remove_circuit(circ); - } - /* Clear cell queue _after_ removing it from the map. Otherwise our * "active" checks will be violated. */ cell_queue_clear(&circ->n_chan_cells); @@ -1917,6 +1921,9 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line, } } + /* Notify the HS subsystem that this circuit is closing. */ + hs_circ_cleanup(circ); + if (circuits_pending_close == NULL) circuits_pending_close = smartlist_new(); @@ -1997,13 +2004,6 @@ circuit_about_to_free(circuit_t *circ) orig_reason); } - /* Notify the HS subsystem for any intro point circuit closing so it can be - * dealt with cleanly. */ - if (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || - circ->purpose == CIRCUIT_PURPOSE_S_INTRO) { - hs_service_intro_circ_has_closed(TO_ORIGIN_CIRCUIT(circ)); - } - if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT) { origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); int timed_out = (reason == END_CIRC_REASON_TIMEOUT); diff --git a/src/or/hs_circuit.c b/src/or/hs_circuit.c index ee952f4d6..a58166ccd 100644 --- a/src/or/hs_circuit.c +++ b/src/or/hs_circuit.c @@ -1178,3 +1178,31 @@ hs_circ_send_establish_rendezvous(origin_circuit_t *circ) return -1; } +/* We are about to close or free this circ. Clean it up from any + * related HS data structures. This function can be called multiple times + * safely for the same circuit. */ +void +hs_circ_cleanup(circuit_t *circ) +{ + tor_assert(circ); + + /* If it's a service-side intro circ, notify the HS subsystem for the intro + * point circuit closing so it can be dealt with cleanly. */ + if (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || + circ->purpose == CIRCUIT_PURPOSE_S_INTRO) { + hs_service_intro_circ_has_closed(TO_ORIGIN_CIRCUIT(circ)); + } + + /* Clear HS circuitmap token for this circ (if any). Very important to be + * done after the HS subsystem has been notified of the close else the + * circuit will not be found. + * + * We do this at the close if possible because from that point on, the + * circuit is good as dead. We can't rely on removing it in the circuit + * free() function because we open a race window between the close and free + * where we can't register a new circuit for the same intro point. */ + if (circ->hs_token) { + hs_circuitmap_remove_circuit(circ); + } +} + diff --git a/src/or/hs_circuit.h b/src/or/hs_circuit.h index 0a1186dba..63ff5e463 100644 --- a/src/or/hs_circuit.h +++ b/src/or/hs_circuit.h @@ -15,6 +15,9 @@ #include "hs_service.h" +/* Cleanup function when the circuit is closed or/and freed. */ +void hs_circ_cleanup(circuit_t *circ); + /* Circuit API. */ int hs_circ_service_intro_has_opened(hs_service_t *service, hs_service_intro_point_t *ip, diff --git a/src/or/hs_common.c b/src/or/hs_common.c index a0f2af29c..ad783a36f 100644 --- a/src/or/hs_common.c +++ b/src/or/hs_common.c @@ -22,6 +22,7 @@ #include "hs_client.h" #include "hs_ident.h" #include "hs_service.h" +#include "hs_circuitmap.h" #include "policies.h" #include "rendcommon.h" #include "rendservice.h" diff --git a/src/or/hs_common.h b/src/or/hs_common.h index c95e59a6f..18c4e7014 100644 --- a/src/or/hs_common.h +++ b/src/or/hs_common.h @@ -161,6 +161,8 @@ typedef struct hsdir_index_t { void hs_init(void); void hs_free_all(void); +void hs_cleanup_circ(circuit_t *circ); + int hs_check_service_private_dir(const char *username, const char *path, unsigned int dir_group_readable, unsigned int create);

1 0

[tor/master] hs-v3: Cleanup HS circuits when marking as closed
by nickm＠torproject.org 11 Dec '17

11 Dec '17

commit e80893e51b0c0320838cbed8c46fd5b0fe608bef Author: David Goulet <dgoulet(a)torproject.org> Date: Mon Dec 4 11:49:48 2017 -0500 hs-v3: Cleanup HS circuits when marking as closed First, hs_service_intro_circ_has_closed() is now called in circuit_mark_for close() because the HS subsystem needs to learn when an intro point is actually not established anymore as soon as possible. There is a time window between a close and a free. Second, when we mark for close, we also remove it from the circuitmap because between the close and the free, a service can launch an new circuit to that same intro point and thus register it which only succeeds if the intro point authentication key is not already in the map. However, we still do a remove from the circuitmap in circuit_free() in order to also cleanup the circuit if it wasn't marked for close prior to the free. Fixes #23603 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/bug23603 | 7 +++++++ src/or/circuitlist.c | 26 +++++++++++++------------- src/or/hs_circuit.c | 28 ++++++++++++++++++++++++++++ src/or/hs_circuit.h | 3 +++ src/or/hs_common.c | 1 + src/or/hs_common.h | 2 ++ 6 files changed, 54 insertions(+), 13 deletions(-) diff --git a/changes/bug23603 b/changes/bug23603 new file mode 100644 index 000000000..dfb2052c9 --- /dev/null +++ b/changes/bug23603 @@ -0,0 +1,7 @@ + o Minor bugfixes (hidden service v3): + - Fix a race between the circuit close and free where the service would + launch a new intro circuit after the close, and then fail to register it + before the free of the previously closed circuit. This was making the + service unable to find the established intro circuit and thus not upload + its descriptor. It can make a service unavailable for up to 24 hours. + Fixes bug 23603; bugfix on 0.3.2.1-alpha. diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index d37d986f1..45d4521c2 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -67,7 +67,6 @@ #include "main.h" #include "hs_circuit.h" #include "hs_circuitmap.h" -#include "hs_common.h" #include "hs_ident.h" #include "networkstatus.h" #include "nodelist.h" @@ -938,6 +937,12 @@ circuit_free(circuit_t *circ) circuit_clear_testing_cell_stats(circ); + /* Cleanup circuit from anything HS v3 related. We also do this when the + * circuit is closed. This is to avoid any code path that free registered + * circuits without closing them before. This needs to be done before the + * hs identifier is freed. */ + hs_circ_cleanup(circ); + if (CIRCUIT_IS_ORIGIN(circ)) { origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); mem = ocirc; @@ -963,7 +968,11 @@ circuit_free(circuit_t *circ) crypto_pk_free(ocirc->intro_key); rend_data_free(ocirc->rend_data); + + /* Finally, free the identifier of the circuit and nullify it so multiple + * cleanup will work. */ hs_ident_circuit_free(ocirc->hs_ident); + ocirc->hs_ident = NULL; tor_free(ocirc->dest_address); if (ocirc->socks_username) { @@ -1022,11 +1031,6 @@ circuit_free(circuit_t *circ) /* Remove from map. */ circuit_set_n_circid_chan(circ, 0, NULL); - /* Clear HS circuitmap token from this circ (if any) */ - if (circ->hs_token) { - hs_circuitmap_remove_circuit(circ); - } - /* Clear cell queue _after_ removing it from the map. Otherwise our * "active" checks will be violated. */ cell_queue_clear(&circ->n_chan_cells); @@ -1917,6 +1921,9 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line, } } + /* Notify the HS subsystem that this circuit is closing. */ + hs_circ_cleanup(circ); + if (circuits_pending_close == NULL) circuits_pending_close = smartlist_new(); @@ -1997,13 +2004,6 @@ circuit_about_to_free(circuit_t *circ) orig_reason); } - /* Notify the HS subsystem for any intro point circuit closing so it can be - * dealt with cleanly. */ - if (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || - circ->purpose == CIRCUIT_PURPOSE_S_INTRO) { - hs_service_intro_circ_has_closed(TO_ORIGIN_CIRCUIT(circ)); - } - if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT) { origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); int timed_out = (reason == END_CIRC_REASON_TIMEOUT); diff --git a/src/or/hs_circuit.c b/src/or/hs_circuit.c index ee952f4d6..a58166ccd 100644 --- a/src/or/hs_circuit.c +++ b/src/or/hs_circuit.c @@ -1178,3 +1178,31 @@ hs_circ_send_establish_rendezvous(origin_circuit_t *circ) return -1; } +/* We are about to close or free this circ. Clean it up from any + * related HS data structures. This function can be called multiple times + * safely for the same circuit. */ +void +hs_circ_cleanup(circuit_t *circ) +{ + tor_assert(circ); + + /* If it's a service-side intro circ, notify the HS subsystem for the intro + * point circuit closing so it can be dealt with cleanly. */ + if (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || + circ->purpose == CIRCUIT_PURPOSE_S_INTRO) { + hs_service_intro_circ_has_closed(TO_ORIGIN_CIRCUIT(circ)); + } + + /* Clear HS circuitmap token for this circ (if any). Very important to be + * done after the HS subsystem has been notified of the close else the + * circuit will not be found. + * + * We do this at the close if possible because from that point on, the + * circuit is good as dead. We can't rely on removing it in the circuit + * free() function because we open a race window between the close and free + * where we can't register a new circuit for the same intro point. */ + if (circ->hs_token) { + hs_circuitmap_remove_circuit(circ); + } +} + diff --git a/src/or/hs_circuit.h b/src/or/hs_circuit.h index 0a1186dba..63ff5e463 100644 --- a/src/or/hs_circuit.h +++ b/src/or/hs_circuit.h @@ -15,6 +15,9 @@ #include "hs_service.h" +/* Cleanup function when the circuit is closed or/and freed. */ +void hs_circ_cleanup(circuit_t *circ); + /* Circuit API. */ int hs_circ_service_intro_has_opened(hs_service_t *service, hs_service_intro_point_t *ip, diff --git a/src/or/hs_common.c b/src/or/hs_common.c index a0f2af29c..ad783a36f 100644 --- a/src/or/hs_common.c +++ b/src/or/hs_common.c @@ -22,6 +22,7 @@ #include "hs_client.h" #include "hs_ident.h" #include "hs_service.h" +#include "hs_circuitmap.h" #include "policies.h" #include "rendcommon.h" #include "rendservice.h" diff --git a/src/or/hs_common.h b/src/or/hs_common.h index c95e59a6f..18c4e7014 100644 --- a/src/or/hs_common.h +++ b/src/or/hs_common.h @@ -161,6 +161,8 @@ typedef struct hsdir_index_t { void hs_init(void); void hs_free_all(void); +void hs_cleanup_circ(circuit_t *circ); + int hs_check_service_private_dir(const char *username, const char *path, unsigned int dir_group_readable, unsigned int create);

1 0