commit bf0065b5c36c6b37fd493e408c220c7757c8eeec Author: Matthew Finkel sysrqb@torproject.org Date: Wed Jun 24 18:38:18 2020 +0000

Bug 40009: Improve tor's client auth stability --- ...ve-accessor-semantic-of-client-cached-obj.patch | 212 +++++++++++++++++++++ projects/tor/build | 4 + projects/tor/config | 1 + 3 files changed, 217 insertions(+)

diff --git a/projects/tor/0001-hs-v3-Improve-accessor-semantic-of-client-cached-obj.patch b/projects/tor/0001-hs-v3-Improve-accessor-semantic-of-client-cached-obj.patch new file mode 100644 index 0000000..1432039 --- /dev/null +++ b/projects/tor/0001-hs-v3-Improve-accessor-semantic-of-client-cached-obj.patch @@ -0,0 +1,212 @@ +From 1810771799dd0b434ac2b5926297d64e383582e1 Mon Sep 17 00:00:00 2001 +From: David Goulet dgoulet@torproject.org +Date: Tue, 10 Mar 2020 10:58:51 -0400 +Subject: [PATCH] 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 9cf408ca3e..44cd2505fd 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 5f7fe9c404..bff71d2645 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(&ocirc->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; +-- +2.20.1 + diff --git a/projects/tor/build b/projects/tor/build index 30ecc7e..012ec98 100644 --- a/projects/tor/build +++ b/projects/tor/build @@ -91,6 +91,10 @@ openssldir=/var/tmp/dist/openssl/openssl [% END %]

cd /var/tmp/build/[% project %]-[% c('version') %] + +# Cherry-pick patch until 0.4.3.6 is released. +patch -p1 < $rootdir/0001-hs-v3-Improve-accessor-semantic-of-client-cached-obj.patch + # add git hash to micro-revision.i for #24995 echo '"[% c("abbrev", { abbrev_length => 16 }) %]"' > micro-revision.i ./autogen.sh diff --git a/projects/tor/config b/projects/tor/config index 912f839..1f57465 100644 --- a/projects/tor/config +++ b/projects/tor/config @@ -68,3 +68,4 @@ input_files: - name: zstd project: zstd enable: '[% c("var/android") %]' + - filename: 0001-hs-v3-Improve-accessor-semantic-of-client-cached-obj.patch