diff options
| -rw-r--r-- | changes/ticket33458 | 4 | ||||
| -rw-r--r-- | doc/tor.1.txt | 21 | ||||
| -rw-r--r-- | src/feature/hs/hs_cache.c | 64 | ||||
| -rw-r--r-- | src/test/test_hs_client.c | 46 |
4 files changed, 113 insertions, 22 deletions
diff --git a/changes/ticket33458 b/changes/ticket33458 new file mode 100644 index 0000000000..885c6dc505 --- /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. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index e1bd8cb60b..ba9b9bb5e0 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -2147,7 +2147,8 @@ is non-zero): GeoIP data, Tor keeps a per-country count of how many client addresses have contacted it so that it can help the bridge authority guess which countries have blocked access to it. If ExtraInfoStatistics is - enabled, it will be published as part of extra-info document. (Default: 1) + enabled, it will be published as part of the extra-info document. + (Default: 1) //Out of order because it logically belongs after BridgeRelay. [[BridgeDistribution]] **BridgeDistribution** __string__:: @@ -2613,7 +2614,7 @@ types of statistics that Tor relays collect and publish: circuit) and writes them into disk every 24 hours. Onion router operators may use the statistics for performance monitoring. If ExtraInfoStatistics is enabled, it will published as part of - extra-info document. (Default: 0) + the extra-info document. (Default: 0) [[ConnDirectionStatistics]] **ConnDirectionStatistics** **0**|**1**:: Relays only. @@ -2621,7 +2622,7 @@ types of statistics that Tor relays collect and publish: traffic it passes between itself and other relays to disk every 24 hours. Enables relay operators to monitor how much their relay is being used as middle node in the circuit. If ExtraInfoStatistics is - enabled, it will be published as part of extra-info document. + enabled, it will be published as part of the extra-info document. (Default: 0) [[DirReqStatistics]] **DirReqStatistics** **0**|**1**:: @@ -2631,7 +2632,7 @@ types of statistics that Tor relays collect and publish: hours. Enables relay and bridge operators to monitor how much their server is being used by clients to learn about Tor network. If ExtraInfoStatistics is enabled, it will published as part of - extra-info document. (Default: 1) + the extra-info document. (Default: 1) [[EntryStatistics]] **EntryStatistics** **0**|**1**:: Relays only. @@ -2640,7 +2641,7 @@ types of statistics that Tor relays collect and publish: operators to monitor how much inbound traffic that originates from Tor clients passes through their server to go further down the Tor network. If ExtraInfoStatistics is enabled, it will be published - as part of extra-info document. (Default: 0) + as part of the extra-info document. (Default: 0) [[ExitPortStatistics]] **ExitPortStatistics** **0**|**1**:: Exit relays only. @@ -2648,7 +2649,7 @@ types of statistics that Tor relays collect and publish: relayed bytes and opened stream per exit port to disk every 24 hours. Enables exit relay operators to measure and monitor amounts of traffic that leaves Tor network through their exit node. If ExtraInfoStatistics - is enabled, it will be published as part of extra-info document. + is enabled, it will be published as part of the extra-info document. (Default: 0) [[ExtraInfoStatistics]] **ExtraInfoStatistics** **0**|**1**:: @@ -2664,9 +2665,9 @@ types of statistics that Tor relays collect and publish: Relays only. When this option is enabled, a Tor relay writes obfuscated statistics on its role as hidden-service directory, introduction - point, or rendezvous point to disk every 24 hours. If - ExtraInfoStatistics is also enabled, these statistics are further - published to the directory authorities. (Default: 1) + point, or rendezvous point to disk every 24 hours. If ExtraInfoStatistics + is enabled, it will be published as part of the extra-info document. + (Default: 1) [[PaddingStatistics]] **PaddingStatistics** **0**|**1**:: Relays and bridges only. @@ -2675,7 +2676,7 @@ types of statistics that Tor relays collect and publish: These statistics are rounded, and omitted if traffic is low. This information is important for load balancing decisions related to padding. If ExtraInfoStatistics is enabled, it will be published - as a part of extra-info document. (Default: 1) + as a part of the extra-info document. (Default: 1) == DIRECTORY SERVER OPTIONS 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 2e603ec259..3d6422a83c 100644 --- a/src/test/test_hs_client.c +++ b/src/test/test_hs_client.c @@ -966,6 +966,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(). */ @@ -990,6 +991,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; |