commit 1810771799dd0b434ac2b5926297d64e383582e1 Author: David Goulet <dgoulet@torproject.org> Date: Tue Mar 10 10:58:51 2020 -0400 hs-v3: Improve accessor semantic of client cached object Add an inline helper function that indicates if the cached object contains a decrypted descriptor or not. The descriptor object is NULL if tor is unable to decrypt it (lacking client authorization) and some actions need to be done only when we have a decrypted object. This improves code semantic. Fixes #33458 Signed-off-by: David Goulet <dgoulet@torproject.org> --- src/feature/hs/hs_cache.c | 64 ++++++++++++++++++++++++++++++++++++++--------- src/test/test_hs_client.c | 46 ++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 12 deletions(-) diff --git a/src/feature/hs/hs_cache.c b/src/feature/hs/hs_cache.c index 9cf408ca3..44cd2505f 100644 --- a/src/feature/hs/hs_cache.c +++ b/src/feature/hs/hs_cache.c @@ -27,6 +27,21 @@ static int cached_client_descriptor_has_expired(time_t now, const hs_cache_client_descriptor_t *cached_desc); +/** Helper function: Return true iff the cache entry has a decrypted + * descriptor. + * + * A NULL desc object in the entry means that we were not able to decrypt the + * descriptor because we are likely lacking client authorization. It is still + * a valid entry but some operations can't be done without the decrypted + * descriptor thus this function MUST be used to safe guard access to the + * decrypted desc object. */ +static inline bool +entry_has_decrypted_descriptor(const hs_cache_client_descriptor_t *entry) +{ + tor_assert(entry); + return (entry->desc != NULL); +} + /********************** Directory HS cache ******************/ /** Directory descriptor cache. Map indexed by blinded key. */ @@ -341,8 +356,23 @@ static digest256map_t *hs_cache_client_intro_state; static size_t cache_get_client_entry_size(const hs_cache_client_descriptor_t *entry) { - return sizeof(*entry) + - strlen(entry->encoded_desc) + hs_desc_obj_size(entry->desc); + size_t size = 0; + + if (entry == NULL) { + goto end; + } + size += sizeof(*entry); + + if (entry->encoded_desc) { + size += strlen(entry->encoded_desc); + } + + if (entry_has_decrypted_descriptor(entry)) { + size += hs_desc_obj_size(entry->desc); + } + + end: + return size; } /** Remove a given descriptor from our cache. */ @@ -659,14 +689,20 @@ cache_store_as_client(hs_cache_client_descriptor_t *client_desc) * client authorization. */ cache_entry = lookup_v3_desc_as_client(client_desc->key.pubkey); if (cache_entry != NULL) { - /* Signalling an undecrypted descriptor. We'll always replace the one we - * have with the new one just fetched. */ - if (cache_entry->desc == NULL) { + /* If the current or the new cache entry don't have a decrypted descriptor + * (missing client authorization), we always replace the current one with + * the new one. Reason is that we can't inspect the revision counter + * within the plaintext data so we blindly replace. */ + if (!entry_has_decrypted_descriptor(cache_entry) || + !entry_has_decrypted_descriptor(client_desc)) { remove_v3_desc_as_client(cache_entry); cache_client_desc_free(cache_entry); goto store; } + /* From this point on, we know that the decrypted descriptor is in the + * current entry and new object thus safe to access. */ + /* If we have an entry in our cache that has a revision counter greater * than the one we just fetched, discard the one we fetched. */ if (cache_entry->desc->plaintext_data.revision_counter > @@ -740,11 +776,15 @@ cache_clean_v3_as_client(time_t now) MAP_DEL_CURRENT(key); entry_size = cache_get_client_entry_size(entry); bytes_removed += entry_size; + /* We just removed an old descriptor. We need to close all intro circuits - * so we don't have leftovers that can be selected while lacking a - * descriptor. We leave the rendezvous circuits opened because they could - * be in use. */ - hs_client_close_intro_circuits_from_desc(entry->desc); + * if the descriptor is decrypted so we don't have leftovers that can be + * selected while lacking a descriptor. Circuits are selected by intro + * authentication key thus we need the descriptor. We leave the rendezvous + * circuits opened because they could be in use. */ + if (entry_has_decrypted_descriptor(entry)) { + hs_client_close_intro_circuits_from_desc(entry->desc); + } /* Entry is not in the cache anymore, destroy it. */ cache_client_desc_free(entry); /* Update our OOM. We didn't use the remove() function because we are in @@ -793,7 +833,7 @@ hs_cache_lookup_as_client(const ed25519_public_key_t *key) tor_assert(key); cached_desc = lookup_v3_desc_as_client(key->pubkey); - if (cached_desc && cached_desc->desc) { + if (cached_desc && entry_has_decrypted_descriptor(cached_desc)) { return cached_desc->desc; } @@ -866,7 +906,7 @@ hs_cache_remove_as_client(const ed25519_public_key_t *key) /* If we have a decrypted/decoded descriptor, attempt to close its * introduction circuit(s). We shouldn't have circuit(s) without a * descriptor else it will lead to a failure. */ - if (cached_desc->desc) { + if (entry_has_decrypted_descriptor(cached_desc)) { hs_client_close_intro_circuits_from_desc(cached_desc->desc); } /* Remove and free. */ @@ -995,7 +1035,7 @@ hs_cache_client_new_auth_parse(const ed25519_public_key_t *service_pk) } cached_desc = lookup_v3_desc_as_client(service_pk->pubkey); - if (cached_desc == NULL || cached_desc->desc != NULL) { + if (cached_desc == NULL || entry_has_decrypted_descriptor(cached_desc)) { /* No entry for that service or the descriptor is already decoded. */ goto end; } diff --git a/src/test/test_hs_client.c b/src/test/test_hs_client.c index 5f7fe9c40..bff71d264 100644 --- a/src/test/test_hs_client.c +++ b/src/test/test_hs_client.c @@ -965,6 +965,7 @@ test_close_intro_circuits_new_desc(void *arg) (void) arg; hs_init(); + rend_cache_init(); /* This is needed because of the client cache expiration timestamp is based * on having a consensus. See cached_client_descriptor_has_expired(). */ @@ -989,6 +990,51 @@ test_close_intro_circuits_new_desc(void *arg) circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCING; ocirc = TO_ORIGIN_CIRCUIT(circ); + /* Build a descriptor _without_ client authorization and thus not + * decryptable. Make sure the close circuit code path is not triggered. */ + { + char *desc_encoded = NULL; + uint8_t descriptor_cookie[HS_DESC_DESCRIPTOR_COOKIE_LEN]; + curve25519_keypair_t client_kp; + hs_descriptor_t *desc = NULL; + + tt_int_op(0, OP_EQ, curve25519_keypair_generate(&client_kp, 0)); + crypto_rand((char *) descriptor_cookie, sizeof(descriptor_cookie)); + + desc = hs_helper_build_hs_desc_with_client_auth(descriptor_cookie, + &client_kp.pubkey, + &service_kp); + tt_assert(desc); + ret = hs_desc_encode_descriptor(desc, &service_kp, descriptor_cookie, + &desc_encoded); + tt_int_op(ret, OP_EQ, 0); + /* Associate descriptor intro key with the dummy circuit. */ + const hs_desc_intro_point_t *ip = + smartlist_get(desc->encrypted_data.intro_points, 0); + tt_assert(ip); + ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey); + ed25519_pubkey_copy(ô->hs_ident->intro_auth_pk, + &ip->auth_key_cert->signed_key); + hs_descriptor_free(desc); + tt_assert(desc_encoded); + /* Put it in the cache. Should not be decrypted since the client + * authorization creds were not added to the global map. */ + ret = hs_cache_store_as_client(desc_encoded, &service_kp.pubkey); + tor_free(desc_encoded); + tt_int_op(ret, OP_EQ, HS_DESC_DECODE_NEED_CLIENT_AUTH); + + /* Clean cache with a future timestamp. It will trigger the clean up and + * attempt to close the circuit but only if the descriptor is decryptable. + * Cache object should be removed and circuit untouched. */ + hs_cache_clean_as_client(mock_ns.valid_after + (60 * 60 * 24)); + tt_assert(!hs_cache_lookup_as_client(&service_kp.pubkey)); + + /* Make sure the circuit still there. */ + tt_assert(circuit_get_next_intro_circ(NULL, true)); + /* Get rid of the ident, it will be replaced in the next tests. */ + hs_ident_circuit_free(ocirc->hs_ident); + } + /* Build the first descriptor and cache it. */ { char *encoded;