Merge remote-tracking branch 'dgoulet/bug23603_032_02' into maint-0.3.2

This commit is contained in:
Nick Mathewson 2017-12-11 09:42:12 -05:00
commit d68abbe358
9 changed files with 151 additions and 25 deletions

7
changes/bug23603 Normal file
View File

@ -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.

View File

@ -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);
@ -2481,8 +2481,8 @@ assert_cpath_ok(const crypt_path_t *cp)
/** Verify that circuit <b>c</b> 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;

View File

@ -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);

View File

@ -1178,3 +1178,31 @@ hs_circ_send_establish_rendezvous(origin_circuit_t *circ)
return -1;
}
/* We are about to close or free this <b>circ</b>. 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);
}
}

View File

@ -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,

View File

@ -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"

View File

@ -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);

View File

@ -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;
}

View File

@ -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,