May 2020 - tor-commits - lists.torproject.org

[tor/maint-0.4.3] changes: Add changes file for ticket 33458
by asn＠torproject.org 28 May '20

28 May '20

commit ca356b952ee892363bc7681f0c5bce1874035c2e Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Mar 18 10:16:31 2020 -0400 changes: Add changes file for ticket 33458 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/ticket33458 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/changes/ticket33458 b/changes/ticket33458 new file mode 100644 index 000000000..885c6dc50 --- /dev/null +++ b/changes/ticket33458 @@ -0,0 +1,4 @@ + o Minor bugfix (onion service v3): + - When cleaning the client descriptor cache, an attempt at closing circuits + for a non decrypted descriptor (lacking client authorization) lead to an + assert(). Fixes bug 33458; bugfix on 0.4.2.1-alpha.

1 0

[tor/master] hs-v3: Improve accessor semantic of client cached object
by asn＠torproject.org 28 May '20

28 May '20

commit 1810771799dd0b434ac2b5926297d64e383582e1 Author: David Goulet <dgoulet(a)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(a)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(&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;

1 0

[tor/maint-0.4.3] hs-v3: Improve accessor semantic of client cached object
by asn＠torproject.org 28 May '20

28 May '20

commit 1810771799dd0b434ac2b5926297d64e383582e1 Author: David Goulet <dgoulet(a)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(a)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(&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;

1 0

[tor/master] changes: Add changes file for ticket 33458
by asn＠torproject.org 28 May '20

28 May '20

commit ca356b952ee892363bc7681f0c5bce1874035c2e Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Mar 18 10:16:31 2020 -0400 changes: Add changes file for ticket 33458 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/ticket33458 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/changes/ticket33458 b/changes/ticket33458 new file mode 100644 index 000000000..885c6dc50 --- /dev/null +++ b/changes/ticket33458 @@ -0,0 +1,4 @@ + o Minor bugfix (onion service v3): + - When cleaning the client descriptor cache, an attempt at closing circuits + for a non decrypted descriptor (lacking client authorization) lead to an + assert(). Fixes bug 33458; bugfix on 0.4.2.1-alpha.

1 0

[tor/master] Merge branch 'maint-0.4.3'
by asn＠torproject.org 28 May '20

28 May '20

commit 5d79b43717608b769ffda0a297bc0306bcceaa98 Merge: 8d8a9d7f1 6891d1bbc Author: George Kadianakis <desnacked(a)riseup.net> Date: Thu May 28 12:52:48 2020 +0300 Merge branch 'maint-0.4.3'

1 0

[tor/master] Merge branch 'tor-github/pr/1792' into maint-0.4.3
by asn＠torproject.org 28 May '20

28 May '20

commit 6891d1bbcb935d186150610a7abd20a2a270ce67 Merge: bdb3be011 ca356b952 Author: George Kadianakis <desnacked(a)riseup.net> Date: Thu May 28 12:25:57 2020 +0300 Merge branch 'tor-github/pr/1792' into maint-0.4.3 changes/ticket33458 | 4 +++ src/feature/hs/hs_cache.c | 64 ++++++++++++++++++++++++++++++++++++++--------- src/test/test_hs_client.c | 46 ++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 12 deletions(-)

1 0

[tor/release-0.4.3] hs-v3: Improve accessor semantic of client cached object
by asn＠torproject.org 28 May '20

28 May '20

commit 1810771799dd0b434ac2b5926297d64e383582e1 Author: David Goulet <dgoulet(a)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(a)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(&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;

1 0

[tor/release-0.4.3] changes: Add changes file for ticket 33458
by asn＠torproject.org 28 May '20

28 May '20

commit ca356b952ee892363bc7681f0c5bce1874035c2e Author: David Goulet <dgoulet(a)torproject.org> Date: Wed Mar 18 10:16:31 2020 -0400 changes: Add changes file for ticket 33458 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/ticket33458 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/changes/ticket33458 b/changes/ticket33458 new file mode 100644 index 000000000..885c6dc50 --- /dev/null +++ b/changes/ticket33458 @@ -0,0 +1,4 @@ + o Minor bugfix (onion service v3): + - When cleaning the client descriptor cache, an attempt at closing circuits + for a non decrypted descriptor (lacking client authorization) lead to an + assert(). Fixes bug 33458; bugfix on 0.4.2.1-alpha.

1 0

[tor/release-0.4.3] Merge branch 'tor-github/pr/1792' into maint-0.4.3
by asn＠torproject.org 28 May '20

28 May '20

commit 6891d1bbcb935d186150610a7abd20a2a270ce67 Merge: bdb3be011 ca356b952 Author: George Kadianakis <desnacked(a)riseup.net> Date: Thu May 28 12:25:57 2020 +0300 Merge branch 'tor-github/pr/1792' into maint-0.4.3 changes/ticket33458 | 4 +++ src/feature/hs/hs_cache.c | 64 ++++++++++++++++++++++++++++++++++++++--------- src/test/test_hs_client.c | 46 ++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 12 deletions(-)

1 0

[tor/release-0.4.3] Merge branch 'maint-0.4.3' into release-0.4.3
by asn＠torproject.org 28 May '20

28 May '20

commit a93a9cfe2390bcb28c5b2d7a68d6e6c7e2b74ec4 Merge: 2d331e30c 6891d1bbc Author: George Kadianakis <desnacked(a)riseup.net> Date: Thu May 28 12:52:48 2020 +0300 Merge branch 'maint-0.4.3' into release-0.4.3 changes/ticket33458 | 4 +++ src/feature/hs/hs_cache.c | 64 ++++++++++++++++++++++++++++++++++++++--------- src/test/test_hs_client.c | 46 ++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 12 deletions(-)

1 0