tor-commits October 2018

tor-commits@lists.torproject.org

17 participants
2350 discussions

[tor/master] Merge branch 'tor-github/pr/438' into maint-0.3.5
by dgoulet＠torproject.org 30 Oct '18

30 Oct '18

commit 488969fe9c4279477b1ff6da34ccf44ed19da8c7 Merge: 95559279e a61473114 Author: David Goulet <dgoulet(a)torproject.org> Date: Tue Oct 30 11:43:54 2018 -0400 Merge branch 'tor-github/pr/438' into maint-0.3.5 changes/ticket27838 | 4 ++ src/feature/hs/hs_service.c | 134 ++++++++++++++++++++++++++++---------------- src/feature/hs/hs_service.h | 72 +++++++++++++++--------- src/test/test_hs_service.c | 4 +- 4 files changed, 135 insertions(+), 79 deletions(-)

1 0

[tor/master] hs-v3: Create desc signing key cert before uploading
by dgoulet＠torproject.org 30 Oct '18

30 Oct '18

commit 81c466c34abdcaae9ed78958da50148ccf9939f8 Author: David Goulet <dgoulet(a)torproject.org> Date: Mon Oct 22 16:18:44 2018 -0400 hs-v3: Create desc signing key cert before uploading Before this commit, we would create the descriptor signing key certificate when first building the descriptor. In some extreme cases, it lead to the expiry of the certificate which triggers a BUG() when encoding the descriptor before uploading. Ticket #27838 details a possible scenario in which this can happen. It is an edge case where tor losts internet connectivity, notices it and closes all circuits. When it came back up, the HS subsystem noticed that it had no introduction circuits, created them and tried to upload the descriptor. However, in the meantime, if tor did lack a live consensus because it is currently seeking to download one, we would consider that we don't need to rotate the descriptors leading to using the expired signing key certificate. That being said, this commit does a bit more to make this process cleaner. There are a series of things that we need to "refresh" before uploading a descriptor: signing key cert, intro points and revision counter. A refresh function is added to deal with all mutable descriptor fields. It in turn simplified a bit the code surrounding the creation of the plaintext data. We keep creating the cert when building the descriptor in order to accomodate the unit tests. However, it is replaced every single time the descriptor is uploaded. Fixes #27838 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/ticket27838 | 4 ++ src/feature/hs/hs_service.c | 111 +++++++++++++++++++++++++++++--------------- 2 files changed, 78 insertions(+), 37 deletions(-) diff --git a/changes/ticket27838 b/changes/ticket27838 new file mode 100644 index 000000000..1699730d7 --- /dev/null +++ b/changes/ticket27838 @@ -0,0 +1,4 @@ + o Minor bugfixes (hidden service v3): + - Build the service descriptor signing key certificate before uploading so + we always have a fresh one leaving no chances for it to expire service + side. Fixes bug 27838; bugfix on 0.3.2.1-alpha. diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 78654bfb2..850eb5cc7 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -1696,6 +1696,32 @@ build_desc_intro_points(const hs_service_t *service, } DIGEST256MAP_FOREACH_END; } +/* Build the descriptor signing key certificate. */ +static void +build_desc_signing_key_cert(hs_service_descriptor_t *desc, time_t now) +{ + hs_desc_plaintext_data_t *plaintext; + + tor_assert(desc); + tor_assert(desc->desc); + + /* Ease our life a bit. */ + plaintext = &desc->desc->plaintext_data; + + /* Get rid of what we have right now. */ + tor_cert_free(plaintext->signing_key_cert); + + /* Fresh certificate for the signing key. */ + plaintext->signing_key_cert = + tor_cert_create(&desc->blinded_kp, CERT_TYPE_SIGNING_HS_DESC, + &desc->signing_kp.pubkey, now, HS_DESC_CERT_LIFETIME, + CERT_FLAG_INCLUDE_SIGNING_KEY); + /* If the cert creation fails, the descriptor encoding will fail and thus + * ultimately won't be uploaded. We'll get a stack trace to help us learn + * where the call came from and the tor_cert_create() will log the error. */ + tor_assert_nonfatal(plaintext->signing_key_cert); +} + /* Populate the descriptor encrypted section from the given service object. * This will generate a valid list of introduction points that can be used * after for circuit creation. Return 0 on success else -1 on error. */ @@ -1811,17 +1837,15 @@ build_service_desc_superencrypted(const hs_service_t *service, /* Populate the descriptor plaintext section from the given service object. * The caller must make sure that the keys in the descriptors are valid that - * is are non-zero. Return 0 on success else -1 on error. */ -static int + * is are non-zero. This can't fail. */ +static void build_service_desc_plaintext(const hs_service_t *service, - hs_service_descriptor_t *desc, time_t now) + hs_service_descriptor_t *desc) { - int ret = -1; hs_desc_plaintext_data_t *plaintext; tor_assert(service); tor_assert(desc); - /* XXX: Use a "assert_desc_ok()" ? */ tor_assert(!tor_mem_is_zero((char *) &desc->blinded_kp, sizeof(desc->blinded_kp))); tor_assert(!tor_mem_is_zero((char *) &desc->signing_kp, @@ -1835,24 +1859,13 @@ build_service_desc_plaintext(const hs_service_t *service, plaintext->version = service->config.version; plaintext->lifetime_sec = HS_DESC_DEFAULT_LIFETIME; - plaintext->signing_key_cert = - tor_cert_create(&desc->blinded_kp, CERT_TYPE_SIGNING_HS_DESC, - &desc->signing_kp.pubkey, now, HS_DESC_CERT_LIFETIME, - CERT_FLAG_INCLUDE_SIGNING_KEY); - if (plaintext->signing_key_cert == NULL) { - log_warn(LD_REND, "Unable to create descriptor signing certificate for " - "service %s", - safe_str_client(service->onion_address)); - goto end; - } /* Copy public key material to go in the descriptor. */ ed25519_pubkey_copy(&plaintext->signing_pubkey, &desc->signing_kp.pubkey); ed25519_pubkey_copy(&plaintext->blinded_pubkey, &desc->blinded_kp.pubkey); - /* Success. */ - ret = 0; - end: - return ret; + /* Create the signing key certificate. This will be updated before each + * upload but we create it here so we don't complexify our unit tests. */ + build_desc_signing_key_cert(desc, approx_time()); } /** Compute the descriptor's OPE cipher for encrypting revision counters. */ @@ -1944,8 +1957,7 @@ build_service_desc_keys(const hs_service_t *service, * * This can error if we are unable to create keys or certificate. */ static void -build_service_descriptor(hs_service_t *service, time_t now, - uint64_t time_period_num, +build_service_descriptor(hs_service_t *service, uint64_t time_period_num, hs_service_descriptor_t **desc_out) { char *encoded_desc; @@ -1964,9 +1976,8 @@ build_service_descriptor(hs_service_t *service, time_t now, goto err; } /* Setup plaintext descriptor content. */ - if (build_service_desc_plaintext(service, desc, now) < 0) { - goto err; - } + build_service_desc_plaintext(service, desc); + /* Setup superencrypted descriptor content. */ if (build_service_desc_superencrypted(service, desc) < 0) { goto err; @@ -2039,10 +2050,8 @@ build_descriptors_for_new_service(hs_service_t *service, time_t now) } /* Build descriptors. */ - build_service_descriptor(service, now, current_desc_tp, - &service->desc_current); - build_service_descriptor(service, now, next_desc_tp, - &service->desc_next); + build_service_descriptor(service, current_desc_tp, &service->desc_current); + build_service_descriptor(service, next_desc_tp, &service->desc_next); log_info(LD_REND, "Hidden service %s has just started. Both descriptors " "built. Now scheduled for upload.", safe_str_client(service->onion_address)); @@ -2072,7 +2081,7 @@ build_all_descriptors(time_t now) } if (service->desc_next == NULL) { - build_service_descriptor(service, now, hs_get_next_time_period_num(0), + build_service_descriptor(service, hs_get_next_time_period_num(0), &service->desc_next); log_info(LD_REND, "Hidden service %s next descriptor successfully " "built. Now scheduled for upload.", @@ -3085,6 +3094,37 @@ should_service_upload_descriptor(const hs_service_t *service, return 0; } +/* Refresh the given service descriptor meaning this will update every mutable + * field that needs to be updated before we upload. + * + * This should ONLY be called before uploading a descriptor. It assumes that + * the descriptor has been built (desc->desc) and that all intro point + * circuits have been established. */ +static void +refresh_service_descriptor(const hs_service_t *service, + hs_service_descriptor_t *desc, time_t now) +{ + /* There are few fields that we consider "mutable" in the descriptor meaning + * we need to update them regurlarly over the lifetime fo the descriptor. + * The rest are set once and should not be modified. + * + * - Signing key certificate. + * - Revision counter. + * - Introduction points which includes many thing. See + * hs_desc_intro_point_t. and the setup_desc_intro_point() function. + */ + + /* Create the signing key certificate. */ + build_desc_signing_key_cert(desc, now); + + /* Build the intro points descriptor section. The refresh step is just + * before we upload so all circuits have been properly established. */ + build_desc_intro_points(service, desc, now); + + /* Set the desc revision counter right before uploading */ + set_descriptor_revision_counter(desc, now, service->desc_current == desc); +} + /* Scheduled event run from the main loop. Try to upload the descriptor for * each service. */ STATIC void @@ -3120,15 +3160,12 @@ run_upload_descriptor_event(time_t now) service->config.num_intro_points, (desc->missing_intro_points) ? " (couldn't pick more)" : ""); - /* At this point, we have to upload the descriptor so start by building - * the intro points descriptor section which we are now sure to be - * accurate because all circuits have been established. */ - build_desc_intro_points(service, desc, now); - - /* Set the desc revision counter right before uploading */ - set_descriptor_revision_counter(desc, approx_time(), - service->desc_current == desc); + /* We are about to upload so we need to do one last step which is to + * update the service's descriptor mutable fields in order to upload a + * coherent descriptor. */ + refresh_service_descriptor(service, desc, now); + /* Proceed with the upload, the descriptor is ready to be encoded. */ upload_descriptor_to_all(service, desc); } FOR_EACH_DESCRIPTOR_END; } FOR_EACH_SERVICE_END;

1 0

[tor/master] Documentation: Document which descriptor elements are (im)mutable.
by dgoulet＠torproject.org 30 Oct '18

30 Oct '18

commit df78fb24511e890a1a2bf00127db604919bbdc0e Author: George Kadianakis <desnacked(a)riseup.net> Date: Tue Oct 23 16:34:43 2018 +0300 Documentation: Document which descriptor elements are (im)mutable. --- src/feature/hs/hs_service.h | 58 +++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/src/feature/hs/hs_service.h b/src/feature/hs/hs_service.h index 863d96bb4..9804a4571 100644 --- a/src/feature/hs/hs_service.h +++ b/src/feature/hs/hs_service.h @@ -99,48 +99,64 @@ typedef struct hs_service_intropoints_t { digestmap_t *failed_id; } hs_service_intropoints_t; -/* Representation of a service descriptor. */ +/* Representation of a service descriptor. + * + * Some elements of the descriptor are mutable whereas others are immutable: + + * Immutable elements are initialized once when the descriptor is built (when + * service descriptors gets rotated). This means that these elements are + * initialized once and then they don't change for the lifetime of the + * descriptor. See build_service_descriptor(). + * + * Mutable elements are initialized when we build the descriptor but they are + * also altered during the lifetime of the descriptor. They could be + * _refreshed_ everytime we upload the descriptor (which happens multiple times + * over the lifetime of the descriptor), or through periodic events. We do this + * for elements like the descriptor revision counter and various + * certificates. See refresh_service_descriptor() and + * update_service_descriptor_intro_points(). + */ typedef struct hs_service_descriptor_t { - /* Decoded descriptor. This object is used for encoding when the service - * publishes the descriptor. */ + /* Mutable: Decoded descriptor. This object is used for encoding when the + * service publishes the descriptor. */ hs_descriptor_t *desc; - /* Client authorization ephemeral keypair. */ + /* Immutable: Client authorization ephemeral keypair. */ curve25519_keypair_t auth_ephemeral_kp; - /* Descriptor cookie used to encrypt the descriptor, when the client - * authorization is enabled */ + /* Immutable: Descriptor cookie used to encrypt the descriptor, when the + * client authorization is enabled */ uint8_t descriptor_cookie[HS_DESC_DESCRIPTOR_COOKIE_LEN]; - /* Descriptor signing keypair. */ + /* Immutable: Descriptor signing keypair. */ ed25519_keypair_t signing_kp; - /* Blinded keypair derived from the master identity public key. */ + /* Immutable: Blinded keypair derived from the master identity public key. */ ed25519_keypair_t blinded_kp; - /* When is the next time when we should upload the descriptor. */ + /* Mutable: When is the next time when we should upload the descriptor. */ time_t next_upload_time; - /* Introduction points assign to this descriptor which contains - * hs_service_intropoints_t object indexed by authentication key (the RSA - * key if the node is legacy). */ + /* Mutable: Introduction points assign to this descriptor which contains + * hs_service_intropoints_t object indexed by authentication key (the RSA key + * if the node is legacy). */ hs_service_intropoints_t intro_points; - /* The time period number this descriptor has been created for. */ + /* Immutable: The time period number this descriptor has been created for. */ uint64_t time_period_num; - /* True iff we have missing intro points for this descriptor because we - * couldn't pick any nodes. */ + /* Mutable: True iff we have missing intro points for this descriptor because + * we couldn't pick any nodes. */ unsigned int missing_intro_points : 1; - /** List of the responsible HSDirs (their b64ed identity digest) last time we - * uploaded this descriptor. If the set of responsible HSDirs is different - * from this list, this means we received new dirinfo and we need to - * reupload our descriptor. */ + /** Mutable: List of the responsible HSDirs (their b64ed identity digest) + * last time we uploaded this descriptor. If the set of responsible HSDirs + * is different from this list, this means we received new dirinfo and we + * need to reupload our descriptor. */ smartlist_t *previous_hsdirs; - /** The OPE cipher for encrypting revision counters for this descriptor. - * Tied to the descriptor blinded key. */ + /** Immutable: The OPE cipher for encrypting revision counters for this + * descriptor. Tied to the descriptor blinded key. */ struct crypto_ope_t *ope_cipher; } hs_service_descriptor_t;

1 0

[tor/release-0.3.5] hs-v3: Create desc signing key cert before uploading
by dgoulet＠torproject.org 30 Oct '18

30 Oct '18

1 0

[tor/master] Documentation: Move the hs_service_descriptor_t elements around.
by dgoulet＠torproject.org 30 Oct '18

30 Oct '18

commit a614731144be9478617411bb3268a4fccec506d9 Author: George Kadianakis <desnacked(a)riseup.net> Date: Tue Oct 23 16:43:08 2018 +0300 Documentation: Move the hs_service_descriptor_t elements around. Move the elements around to concentrate mutable and immutable elements together. This commit changes no code, check with --color-moved. --- src/feature/hs/hs_service.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/feature/hs/hs_service.h b/src/feature/hs/hs_service.h index 9804a4571..3f0adecbf 100644 --- a/src/feature/hs/hs_service.h +++ b/src/feature/hs/hs_service.h @@ -117,10 +117,6 @@ typedef struct hs_service_intropoints_t { * update_service_descriptor_intro_points(). */ typedef struct hs_service_descriptor_t { - /* Mutable: Decoded descriptor. This object is used for encoding when the - * service publishes the descriptor. */ - hs_descriptor_t *desc; - /* Immutable: Client authorization ephemeral keypair. */ curve25519_keypair_t auth_ephemeral_kp; @@ -134,6 +130,17 @@ typedef struct hs_service_descriptor_t { /* Immutable: Blinded keypair derived from the master identity public key. */ ed25519_keypair_t blinded_kp; + /* Immutable: The time period number this descriptor has been created for. */ + uint64_t time_period_num; + + /** Immutable: The OPE cipher for encrypting revision counters for this + * descriptor. Tied to the descriptor blinded key. */ + struct crypto_ope_t *ope_cipher; + + /* Mutable: Decoded descriptor. This object is used for encoding when the + * service publishes the descriptor. */ + hs_descriptor_t *desc; + /* Mutable: When is the next time when we should upload the descriptor. */ time_t next_upload_time; @@ -142,9 +149,6 @@ typedef struct hs_service_descriptor_t { * if the node is legacy). */ hs_service_intropoints_t intro_points; - /* Immutable: The time period number this descriptor has been created for. */ - uint64_t time_period_num; - /* Mutable: True iff we have missing intro points for this descriptor because * we couldn't pick any nodes. */ unsigned int missing_intro_points : 1; @@ -154,10 +158,6 @@ typedef struct hs_service_descriptor_t { * is different from this list, this means we received new dirinfo and we * need to reupload our descriptor. */ smartlist_t *previous_hsdirs; - - /** Immutable: The OPE cipher for encrypting revision counters for this - * descriptor. Tied to the descriptor blinded key. */ - struct crypto_ope_t *ope_cipher; } hs_service_descriptor_t; /* Service key material. */

1 0

[tor/release-0.3.5] Documentation: Move the hs_service_descriptor_t elements around.
by dgoulet＠torproject.org 30 Oct '18

30 Oct '18

1 0

[tor/release-0.3.5] Func rename: Make it clear that update_all_descriptors() does intro points.
by dgoulet＠torproject.org 30 Oct '18

30 Oct '18

commit 29c194e0222a517a55a16fdc2a80a965f406cab8 Author: George Kadianakis <desnacked(a)riseup.net> Date: Tue Oct 23 16:35:02 2018 +0300 Func rename: Make it clear that update_all_descriptors() does intro points. With the new refresh_service_descriptor() function we had both refresh_service_descriptor() and update_service_descriptor() which is basically the same thing. This commit renames update_service_descriptor() to update_service_descriptor_intro_points() to make it clear it's not a generic refresh and it's only about intro points. Commit changes no code. --- src/feature/hs/hs_service.c | 23 +++++++++++------------ src/feature/hs/hs_service.h | 2 +- src/test/test_hs_service.c | 4 ++-- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 850eb5cc7..58d28e984 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -2293,12 +2293,9 @@ service_desc_schedule_upload(hs_service_descriptor_t *desc, } } -/* Update the given descriptor from the given service. The possible update - * actions includes: - * - Picking missing intro points if needed. - */ +/* Pick missing intro points for this descriptor if needed. */ static void -update_service_descriptor(hs_service_t *service, +update_service_descriptor_intro_points(hs_service_t *service, hs_service_descriptor_t *desc, time_t now) { unsigned int num_intro_points; @@ -2337,15 +2334,17 @@ update_service_descriptor(hs_service_t *service, } } -/* Update descriptors for each service if needed. */ +/* Update descriptor intro points for each service if needed. We do this as + * part of the periodic event because we need to establish intro point circuits + * before we publish descriptors. */ STATIC void -update_all_descriptors(time_t now) +update_all_descriptors_intro_points(time_t now) { FOR_EACH_SERVICE_BEGIN(service) { /* We'll try to update each descriptor that is if certain conditions apply * in order for the descriptor to be updated. */ FOR_EACH_DESCRIPTOR_BEGIN(service, desc) { - update_service_descriptor(service, desc, now); + update_service_descriptor_intro_points(service, desc, now); } FOR_EACH_DESCRIPTOR_END; } FOR_EACH_SERVICE_END; } @@ -2630,10 +2629,10 @@ run_build_descriptor_event(time_t now) * been rotated or we just started up. */ build_all_descriptors(now); - /* Finally, we'll check if we should update the descriptors. Missing - * introduction points will be picked in this function which is useful for - * newly built descriptors. */ - update_all_descriptors(now); + /* Finally, we'll check if we should update the descriptors' intro + * points. Missing introduction points will be picked in this function which + * is useful for newly built descriptors. */ + update_all_descriptors_intro_points(now); } /* For the given service, launch any intro point circuits that could be diff --git a/src/feature/hs/hs_service.h b/src/feature/hs/hs_service.h index 6fb15b9d3..863d96bb4 100644 --- a/src/feature/hs/hs_service.h +++ b/src/feature/hs/hs_service.h @@ -387,7 +387,7 @@ STATIC int intro_point_should_expire(const hs_service_intro_point_t *ip, STATIC void run_housekeeping_event(time_t now); STATIC void rotate_all_descriptors(time_t now); STATIC void build_all_descriptors(time_t now); -STATIC void update_all_descriptors(time_t now); +STATIC void update_all_descriptors_intro_points(time_t now); STATIC void run_upload_descriptor_event(time_t now); STATIC void service_descriptor_free_(hs_service_descriptor_t *desc); diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index bfe50eb3c..ee2d71aa7 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -1457,7 +1457,7 @@ test_build_update_descriptors(void *arg) /* Time to test the update of those descriptors. At first, we have no node * in the routerlist so this will find NO suitable node for the IPs. */ setup_full_capture_of_logs(LOG_INFO); - update_all_descriptors(now); + update_all_descriptors_intro_points(now); expect_log_msg_containing("Unable to find a suitable node to be an " "introduction point for service"); teardown_capture_of_logs(); @@ -1508,7 +1508,7 @@ test_build_update_descriptors(void *arg) /* We expect to pick only one intro point from the node above. */ setup_full_capture_of_logs(LOG_INFO); - update_all_descriptors(now); + update_all_descriptors_intro_points(now); tor_free(node->ri->onion_curve25519_pkey); /* Avoid memleak. */ tor_free(node->ri->cache_info.signing_key_cert); tor_free(node->ri->onion_pkey);

1 0

[tor/master] Merge branch 'maint-0.3.5'
by dgoulet＠torproject.org 30 Oct '18

30 Oct '18

commit aa1ae1343a2e57dc5a7bcd045566a10175acdd10 Merge: 124c43704 488969fe9 Author: David Goulet <dgoulet(a)torproject.org> Date: Tue Oct 30 11:44:14 2018 -0400 Merge branch 'maint-0.3.5' changes/ticket27838 | 4 ++ src/feature/hs/hs_service.c | 134 ++++++++++++++++++++++++++++---------------- src/feature/hs/hs_service.h | 72 +++++++++++++++--------- src/test/test_hs_service.c | 4 +- 4 files changed, 135 insertions(+), 79 deletions(-)

1 0

[tor/release-0.3.5] Documentation: Document which descriptor elements are (im)mutable.
by dgoulet＠torproject.org 30 Oct '18

30 Oct '18

1 0

[tor/release-0.3.5] Merge branch 'tor-github/pr/438' into maint-0.3.5
by dgoulet＠torproject.org 30 Oct '18

30 Oct '18

1 0

← Newer
1
...
9
10
11
12
13
14
15
...
235
Older →

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

tor-commits October 2018