[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [tor-browser-build/maint-9.5] Bug 40009: Improve tor's client auth stability



commit bf0065b5c36c6b37fd493e408c220c7757c8eeec
Author: Matthew Finkel <sysrqb@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
+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@xxxxxxxxxxxxxx>
+---
+ 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



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits