Commit 6891d1bb authored by George Kadianakis's avatar George Kadianakis
Browse files

Merge branch 'tor-github/pr/1792' into maint-0.4.3

parents bdb3be01 ca356b95
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.
......@@ -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;
}
......
......@@ -969,6 +969,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(). */
......@@ -993,6 +994,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;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment