commit 7150214baf2385d0e64fd11fe4138932675d444f Author: David Goulet <dgoulet@torproject.org> Date: Mon Sep 11 13:16:23 2017 -0400 hs-v3: Cancel active descriptor directory connections before uploading It is possible that two descriptor upload requests are launched in a very short time frame which can lead to the second request finishing before the first one and where that first one will make the HSDir send back a 400 malformed descriptor leading to a warning. To avoid such, cancel all active directory connections for the specific descriptor we are about to upload. Note that this race is still possible on the HSDir side which triggers a log info to be printed out but that is fine. Fixes #23457 Signed-off-by: David Goulet <dgoulet@torproject.org> --- src/or/hs_client.c | 3 ++- src/or/hs_ident.c | 16 ++++++++++++++++ src/or/hs_ident.h | 8 ++++++++ src/or/hs_service.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/or/hs_client.c b/src/or/hs_client.c index f85ebc847..2f8953ba7 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -156,7 +156,8 @@ directory_launch_v3_desc_fetch(const ed25519_public_key_t *onion_identity_pk, } /* Copy onion pk to a dir_ident so that we attach it to the dir conn */ - ed25519_pubkey_copy(&hs_conn_dir_ident.identity_pk, onion_identity_pk); + hs_ident_dir_conn_init(onion_identity_pk, &blinded_pubkey, + &hs_conn_dir_ident); /* Setup directory request */ directory_request_t *req = diff --git a/src/or/hs_ident.c b/src/or/hs_ident.c index df3928515..b0e4e36a9 100644 --- a/src/or/hs_ident.c +++ b/src/or/hs_ident.c @@ -65,6 +65,22 @@ hs_ident_dir_conn_free(hs_ident_dir_conn_t *ident) tor_free(ident); } +/* Initialized the allocated ident object with identity_pk and blinded_pk. + * None of them can be NULL since a valid directory connection identifier must + * have all fields set. */ +void +hs_ident_dir_conn_init(const ed25519_public_key_t *identity_pk, + const ed25519_public_key_t *blinded_pk, + hs_ident_dir_conn_t *ident) +{ + tor_assert(identity_pk); + tor_assert(blinded_pk); + tor_assert(ident); + + ed25519_pubkey_copy(&ident->identity_pk, identity_pk); + ed25519_pubkey_copy(&ident->blinded_pk, blinded_pk); +} + /* Return a newly allocated edge connection identifier. The given public key * identity_pk is copied into the identifier. */ hs_ident_edge_conn_t * diff --git a/src/or/hs_ident.h b/src/or/hs_ident.h index cfcde781d..101c1cfff 100644 --- a/src/or/hs_ident.h +++ b/src/or/hs_ident.h @@ -96,6 +96,11 @@ typedef struct hs_ident_dir_conn_t { * in the onion address. */ ed25519_public_key_t identity_pk; + /* The blinded public key used to uniquely identify the descriptor that this + * directory connection identifier is for. Only used by the service-side code + * to fine control descriptor uploads. */ + ed25519_public_key_t blinded_pk; + /* XXX: Client authorization. */ } hs_ident_dir_conn_t; @@ -120,6 +125,9 @@ hs_ident_circuit_t *hs_ident_circuit_dup(const hs_ident_circuit_t *src); /* Directory connection identifier API. */ hs_ident_dir_conn_t *hs_ident_dir_conn_dup(const hs_ident_dir_conn_t *src); void hs_ident_dir_conn_free(hs_ident_dir_conn_t *ident); +void hs_ident_dir_conn_init(const ed25519_public_key_t *identity_pk, + const ed25519_public_key_t *blinded_pk, + hs_ident_dir_conn_t *ident); /* Edge connection identifier API. */ hs_ident_edge_conn_t *hs_ident_edge_conn_new( diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 5759aa812..d32a120bc 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -14,6 +14,7 @@ #include "circuitlist.h" #include "circuituse.h" #include "config.h" +#include "connection.h" #include "directory.h" #include "main.h" #include "networkstatus.h" @@ -641,6 +642,41 @@ count_desc_circuit_established(const hs_service_descriptor_t *desc) return count; } +/* For a given service and descriptor of that service, close all active + * directory connections. */ +static void +close_directory_connections(const hs_service_t *service, + const hs_service_descriptor_t *desc) +{ + unsigned int count = 0; + smartlist_t *dir_conns; + + tor_assert(service); + tor_assert(desc); + + /* Close pending HS desc upload connections for the blinded key of 'desc'. */ + dir_conns = connection_list_by_type_purpose(CONN_TYPE_DIR, + DIR_PURPOSE_UPLOAD_HSDESC); + SMARTLIST_FOREACH_BEGIN(dir_conns, connection_t *, conn) { + dir_connection_t *dir_conn = TO_DIR_CONN(conn); + if (ed25519_pubkey_eq(&dir_conn->hs_ident->identity_pk, + &service->keys.identity_pk) && + ed25519_pubkey_eq(&dir_conn->hs_ident->blinded_pk, + &desc->blinded_kp.pubkey)) { + connection_mark_for_close(conn); + count++; + continue; + } + } SMARTLIST_FOREACH_END(conn); + + log_info(LD_REND, "Closed %u active service directory connections for " + "descriptor %s of service %s", + count, safe_str_client(ed25519_fmt(&desc->blinded_kp.pubkey)), + safe_str_client(service->onion_address)); + /* We don't have ownership of the objects in this list. */ + smartlist_free(dir_conns); +} + /* Close all rendezvous circuits for the given service. */ static void close_service_rp_circuits(hs_service_t *service) @@ -2137,7 +2173,9 @@ upload_descriptor_to_hsdir(const hs_service_t *service, } /* Setup the connection identifier. */ - ed25519_pubkey_copy(&ident.identity_pk, &service->keys.identity_pk); + hs_ident_dir_conn_init(&service->keys.identity_pk, &desc->blinded_kp.pubkey, + &ident); + /* This is our resource when uploading which is used to construct the URL * with the version number: "/tor/hs/<version>/publish". */ tor_snprintf(version_str, sizeof(version_str), "%u", @@ -2387,6 +2425,13 @@ upload_descriptor_to_all(const hs_service_t *service, tor_assert(service); tor_assert(desc); + /* We'll first cancel any directory request that are ongoing for this + * descriptor. It is possible that we can trigger multiple uploads in a + * short time frame which can lead to a race where the second upload arrives + * before the first one leading to a 400 malformed descriptor response from + * the directory. Closing all pending requests avoids that. */ + close_directory_connections(service, desc); + /* Get our list of responsible HSDir. */ responsible_dirs = smartlist_new(); /* The parameter 0 means that we aren't a client so tell the function to use