commit b6c24eb484862cf8741362de792d364d31dc3b0e Author: David Goulet dgoulet@torproject.org Date: Wed Oct 23 11:37:33 2019 -0400

hs-v3: Remove the circuit_established intro flag

Only use the HS circuit map to know if an introduction circuit is established or not. No need for a flag to keep state of something we already have in the circuit map. Furthermore, the circuit map gets cleaned up properly so it will always have the "latest truth".

This commit also removes a unit test that was testing specifically that flag but now we rely solely on the HS circuit map which is also tested few lines below the removed test.

Fixes #32094

Signed-off-by: David Goulet dgoulet@torproject.org --- changes/ticket32094 | 4 ++++ src/feature/hs/hs_circuit.c | 21 +++++++++++++++++++++ src/feature/hs/hs_circuit.h | 2 ++ src/feature/hs/hs_service.c | 17 ++--------------- src/feature/hs/hs_service.h | 3 --- src/test/test_hs_service.c | 8 -------- 6 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/changes/ticket32094 b/changes/ticket32094 new file mode 100644 index 000000000..f6d0aba16 --- /dev/null +++ b/changes/ticket32094 @@ -0,0 +1,4 @@ + o Minor bugfixes (hidden service v3): + - Do not rely on a "circuit established" flag for intro circuit but instead + always query the HS circuit map. This is to avoid sync issue with that + flag and the map. Fixes bug 32094; bugfix on 0.3.2.1-alpha. diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index 5e213b5ab..e1e9c7c79 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -637,6 +637,27 @@ hs_circ_service_get_intro_circ(const hs_service_intro_point_t *ip) } }

+/* Return an introduction point established circuit matching the given intro + * point object. The circuit purpose has to be CIRCUIT_PURPOSE_S_INTRO. NULL + * is returned is no such circuit can be found. */ +origin_circuit_t * +hs_circ_service_get_established_intro_circ(const hs_service_intro_point_t *ip) +{ + origin_circuit_t *circ; + + tor_assert(ip); + + if (ip->base.is_only_legacy) { + circ = hs_circuitmap_get_intro_circ_v2_service_side(ip->legacy_key_digest); + } else { + circ = hs_circuitmap_get_intro_circ_v3_service_side( + &ip->auth_key_kp.pubkey); + } + + /* Only return circuit if it is established. */ + return (TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_INTRO) ? circ : NULL; +} + /* Called when we fail building a rendezvous circuit at some point other than * the last hop: launches a new circuit to the same rendezvous point. This * supports legacy service. diff --git a/src/feature/hs/hs_circuit.h b/src/feature/hs/hs_circuit.h index e168b301f..c817f3e37 100644 --- a/src/feature/hs/hs_circuit.h +++ b/src/feature/hs/hs_circuit.h @@ -35,6 +35,8 @@ void hs_circ_retry_service_rendezvous_point(origin_circuit_t *circ);

origin_circuit_t *hs_circ_service_get_intro_circ( const hs_service_intro_point_t *ip); +origin_circuit_t *hs_circ_service_get_established_intro_circ( + const hs_service_intro_point_t *ip);

/* Cell API. */ int hs_circ_handle_intro_established(const hs_service_t *service, diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 15db5dd1d..6587d57ec 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -709,7 +709,7 @@ count_desc_circuit_established(const hs_service_descriptor_t *desc)

DIGEST256MAP_FOREACH(desc->intro_points.map, key, const hs_service_intro_point_t *, ip) { - count += ip->circuit_established; + count += !!hs_circ_service_get_established_intro_circ(ip); } DIGEST256MAP_FOREACH_END;

return count; @@ -1660,7 +1660,7 @@ build_desc_intro_points(const hs_service_t *service,

DIGEST256MAP_FOREACH(desc->intro_points.map, key, const hs_service_intro_point_t *, ip) { - if (!ip->circuit_established) { + if (!hs_circ_service_get_established_intro_circ(ip)) { /* Ignore un-established intro points. They can linger in that list * because their circuit has not opened and they haven't been removed * yet even though we have enough intro circuits. @@ -2370,10 +2370,6 @@ should_remove_intro_point(hs_service_intro_point_t *ip, time_t now) * remove it because it might simply be valid and opened at the previous * scheduled event for the last retry. */

- /* Did we established already? */ - if (ip->circuit_established) { - goto end; - } /* Do we simply have an existing circuit regardless of its state? */ if (hs_circ_service_get_intro_circ(ip)) { goto end; @@ -3326,11 +3322,6 @@ service_handle_intro_established(origin_circuit_t *circ, goto err; }

- /* Flag that we have an established circuit for this intro point. This value - * is what indicates the upload scheduled event if we are ready to build the - * intro point into the descriptor and upload. */ - ip->circuit_established = 1; - log_info(LD_REND, "Successfully received an INTRO_ESTABLISHED cell " "on circuit %u for service %s", TO_CIRCUIT(circ)->n_circ_id, @@ -3725,10 +3716,6 @@ hs_service_intro_circ_has_closed(origin_circuit_t *circ) /* Can't have an intro point object without a descriptor. */ tor_assert(desc);

- /* Circuit disappeared so make sure the intro point is updated. By - * keeping the object in the descriptor, we'll be able to retry. */ - ip->circuit_established = 0; - end: return; } diff --git a/src/feature/hs/hs_service.h b/src/feature/hs/hs_service.h index 7d865f375..193e08546 100644 --- a/src/feature/hs/hs_service.h +++ b/src/feature/hs/hs_service.h @@ -69,9 +69,6 @@ typedef struct hs_service_intro_point_t { * consensus. After MAX_INTRO_POINT_CIRCUIT_RETRIES, we give up on it. */ uint32_t circuit_retries;

- /** Set if this intro point has an established circuit. */ - unsigned int circuit_established : 1; - /** Replay cache recording the encrypted part of an INTRODUCE2 cell that the * circuit associated with this intro point has received. This is used to * prevent replay attacks. */ diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 66194cee3..45c8cb984 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -1013,7 +1013,6 @@ test_intro_established(void *arg) /* Send an empty payload. INTRO_ESTABLISHED cells are basically zeroes. */ ret = hs_service_receive_intro_established(circ, payload, sizeof(payload)); tt_int_op(ret, OP_EQ, 0); - tt_u64_op(ip->circuit_established, OP_EQ, 1); tt_int_op(TO_CIRCUIT(circ)->purpose, OP_EQ, CIRCUIT_PURPOSE_S_INTRO);

done: @@ -1296,18 +1295,11 @@ test_service_event(void *arg) * descriptor map so we can retry it. */ ip = helper_create_service_ip(); service_intro_point_add(service->desc_current->intro_points.map, ip); - ip->circuit_established = 1; /* We'll test that, it MUST be 0 after. */ - run_housekeeping_event(now); - tt_int_op(digest256map_size(service->desc_current->intro_points.map), - OP_EQ, 1); - /* No removal if we have an established circuit after retries. */ - ip->circuit_retries = MAX_INTRO_POINT_CIRCUIT_RETRIES + 1; run_housekeeping_event(now); tt_int_op(digest256map_size(service->desc_current->intro_points.map), OP_EQ, 1); /* Remove the IP object at once for the next test. */ ip->circuit_retries = MAX_INTRO_POINT_CIRCUIT_RETRIES + 1; - ip->circuit_established = 0; run_housekeeping_event(now); tt_int_op(digest256map_size(service->desc_current->intro_points.map), OP_EQ, 0);