From bb98bc8562c59c0357e8911fa8952ed5ee20206d Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 23 Aug 2018 19:21:26 +1000 Subject: [PATCH] hs: abolish hs_desc_link_specifier_t The previous commits for 23576 confused hs_desc_link_specifier_t and link_specifier_t. Removing hs_desc_link_specifier_t fixes this confusion. Fixes bug 22781; bugfix on 0.3.2.1-alpha. --- changes/bug22781 | 4 + src/feature/hs/hs_cell.c | 9 +- src/feature/hs/hs_client.c | 21 ++-- src/feature/hs/hs_common.c | 39 ++++++++ src/feature/hs/hs_common.h | 2 + src/feature/hs/hs_descriptor.c | 178 +++------------------------------ src/feature/hs/hs_descriptor.h | 29 +----- src/feature/hs/hs_intropoint.c | 4 +- src/feature/hs/hs_service.c | 50 +++++---- src/test/hs_test_helpers.c | 85 +++++++++++----- src/test/test_hs_client.c | 4 + src/test/test_hs_descriptor.c | 111 -------------------- src/test/test_hs_service.c | 20 ++-- 13 files changed, 175 insertions(+), 381 deletions(-) create mode 100644 changes/bug22781 diff --git a/changes/bug22781 b/changes/bug22781 new file mode 100644 index 0000000000..5606dfa5e2 --- /dev/null +++ b/changes/bug22781 @@ -0,0 +1,4 @@ + o Code simplification and refactoring: + - Replace hs_desc_link_specifier_t with link_specifier_t, + and remove all hs_desc_link_specifier_t-specific code. + Fixes bug 22781; bugfix on 0.3.2.1-alpha. diff --git a/src/feature/hs/hs_cell.c b/src/feature/hs/hs_cell.c index 597982b34e..e24520da47 100644 --- a/src/feature/hs/hs_cell.c +++ b/src/feature/hs/hs_cell.c @@ -756,7 +756,14 @@ hs_cell_parse_introduce2(hs_cell_introduce2_data_t *data, idx < trn_cell_introduce_encrypted_get_nspec(enc_cell); idx++) { link_specifier_t *lspec = trn_cell_introduce_encrypted_get_nspecs(enc_cell, idx); - smartlist_add(data->link_specifiers, hs_link_specifier_dup(lspec)); + if (BUG(!lspec)) { + goto done; + } + link_specifier_t *lspec_dup = hs_link_specifier_dup(lspec); + if (BUG(!lspec_dup)) { + goto done; + } + smartlist_add(data->link_specifiers, lspec_dup); } /* Success. */ diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index d4eee50bb7..9d0dee7124 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -528,13 +528,15 @@ find_desc_intro_point_by_legacy_id(const char *legacy_id, SMARTLIST_FOREACH_BEGIN(desc->encrypted_data.intro_points, hs_desc_intro_point_t *, ip) { SMARTLIST_FOREACH_BEGIN(ip->link_specifiers, - const hs_desc_link_specifier_t *, lspec) { + const link_specifier_t *, lspec) { /* Not all tor node have an ed25519 identity key so we still rely on the * legacy identity digest. */ - if (lspec->type != LS_LEGACY_ID) { + if (link_specifier_get_ls_type(lspec) != LS_LEGACY_ID) { continue; } - if (fast_memneq(legacy_id, lspec->u.legacy_id, DIGEST_LEN)) { + if (fast_memneq(legacy_id, + link_specifier_getconstarray_un_legacy_id(lspec), + DIGEST_LEN)) { break; } /* Found it. */ @@ -753,24 +755,13 @@ STATIC extend_info_t * desc_intro_point_to_extend_info(const hs_desc_intro_point_t *ip) { extend_info_t *ei; - smartlist_t *lspecs = smartlist_new(); tor_assert(ip); - /* We first encode the descriptor link specifiers into the binary - * representation which is a trunnel object. */ - SMARTLIST_FOREACH_BEGIN(ip->link_specifiers, - const hs_desc_link_specifier_t *, desc_lspec) { - link_specifier_t *lspec = hs_desc_lspec_to_trunnel(desc_lspec); - smartlist_add(lspecs, lspec); - } SMARTLIST_FOREACH_END(desc_lspec); - /* Explicitly put the direct connection option to 0 because this is client * side and there is no such thing as a non anonymous client. */ - ei = hs_get_extend_info_from_lspecs(lspecs, &ip->onion_key, 0); + ei = hs_get_extend_info_from_lspecs(ip->link_specifiers, &ip->onion_key, 0); - SMARTLIST_FOREACH(lspecs, link_specifier_t *, ls, link_specifier_free(ls)); - smartlist_free(lspecs); return ei; } diff --git a/src/feature/hs/hs_common.c b/src/feature/hs/hs_common.c index 5d8f54230f..a1dc0a6290 100644 --- a/src/feature/hs/hs_common.c +++ b/src/feature/hs/hs_common.c @@ -1843,3 +1843,42 @@ hs_inc_rdv_stream_counter(origin_circuit_t *circ) tor_assert_nonfatal_unreached(); } } + +/* Return a newly allocated link specifier object that is a copy of dst. */ +link_specifier_t * +link_specifier_dup(const link_specifier_t *src) +{ + link_specifier_t *dup = NULL; + uint8_t *buf = NULL; + + if (BUG(!src)) { + goto err; + } + + ssize_t encoded_len_alloc = link_specifier_encoded_len(src); + if (BUG(encoded_len_alloc < 0)) { + goto err; + } + + buf = tor_malloc_zero(encoded_len_alloc); + ssize_t encoded_len_data = link_specifier_encode(buf, + encoded_len_alloc, + src); + if (BUG(encoded_len_data < 0)) { + goto err; + } + + ssize_t parsed_len = link_specifier_parse(&dup, buf, encoded_len_alloc); + if (BUG(parsed_len < 0)) { + goto err; + } + + goto done; + + err: + dup = NULL; + + done: + tor_free(buf); + return dup; +} diff --git a/src/feature/hs/hs_common.h b/src/feature/hs/hs_common.h index a44505930a..544e63e596 100644 --- a/src/feature/hs/hs_common.h +++ b/src/feature/hs/hs_common.h @@ -262,6 +262,8 @@ extend_info_t *hs_get_extend_info_from_lspecs(const smartlist_t *lspecs, const struct curve25519_public_key_t *onion_key, int direct_conn); +link_specifier_t *link_specifier_dup(const link_specifier_t *src); + #ifdef HS_COMMON_PRIVATE STATIC void get_disaster_srv(uint64_t time_period_num, uint8_t *srv_out); diff --git a/src/feature/hs/hs_descriptor.c b/src/feature/hs/hs_descriptor.c index b09d50e010..8f7bdf86ef 100644 --- a/src/feature/hs/hs_descriptor.c +++ b/src/feature/hs/hs_descriptor.c @@ -324,12 +324,11 @@ encode_link_specifiers(const smartlist_t *specs) link_specifier_list_set_n_spec(lslist, smartlist_len(specs)); - SMARTLIST_FOREACH_BEGIN(specs, const hs_desc_link_specifier_t *, + SMARTLIST_FOREACH_BEGIN(specs, const link_specifier_t *, spec) { - link_specifier_t *ls = hs_desc_lspec_to_trunnel(spec); - if (ls) { - link_specifier_list_add_spec(lslist, ls); - } + link_specifier_t *ls = link_specifier_dup(spec); + tor_assert(ls); + link_specifier_list_add_spec(lslist, ls); } SMARTLIST_FOREACH_END(spec); { @@ -1190,52 +1189,22 @@ decode_link_specifiers(const char *encoded) results = smartlist_new(); for (i = 0; i < link_specifier_list_getlen_spec(specs); i++) { - hs_desc_link_specifier_t *hs_spec; link_specifier_t *ls = link_specifier_list_get_spec(specs, i); - tor_assert(ls); - - hs_spec = tor_malloc_zero(sizeof(*hs_spec)); - hs_spec->type = link_specifier_get_ls_type(ls); - switch (hs_spec->type) { - case LS_IPV4: - tor_addr_from_ipv4h(&hs_spec->u.ap.addr, - link_specifier_get_un_ipv4_addr(ls)); - hs_spec->u.ap.port = link_specifier_get_un_ipv4_port(ls); - break; - case LS_IPV6: - tor_addr_from_ipv6_bytes(&hs_spec->u.ap.addr, (const char *) - link_specifier_getarray_un_ipv6_addr(ls)); - hs_spec->u.ap.port = link_specifier_get_un_ipv6_port(ls); - break; - case LS_LEGACY_ID: - /* Both are known at compile time so let's make sure they are the same - * else we can copy memory out of bound. */ - tor_assert(link_specifier_getlen_un_legacy_id(ls) == - sizeof(hs_spec->u.legacy_id)); - memcpy(hs_spec->u.legacy_id, link_specifier_getarray_un_legacy_id(ls), - sizeof(hs_spec->u.legacy_id)); - break; - case LS_ED25519_ID: - /* Both are known at compile time so let's make sure they are the same - * else we can copy memory out of bound. */ - tor_assert(link_specifier_getlen_un_ed25519_id(ls) == - sizeof(hs_spec->u.ed25519_id)); - memcpy(hs_spec->u.ed25519_id, - link_specifier_getconstarray_un_ed25519_id(ls), - sizeof(hs_spec->u.ed25519_id)); - break; - default: - tor_free(hs_spec); + if (BUG(!ls)) { goto err; } - - smartlist_add(results, hs_spec); + link_specifier_t *ls_dup = link_specifier_dup(ls); + if (BUG(!ls_dup)) { + goto err; + } + smartlist_add(results, ls_dup); } goto done; err: if (results) { - SMARTLIST_FOREACH(results, hs_desc_link_specifier_t *, s, tor_free(s)); + SMARTLIST_FOREACH(results, link_specifier_t *, s, + link_specifier_free(s)); smartlist_free(results); results = NULL; } @@ -2878,8 +2847,8 @@ hs_desc_intro_point_free_(hs_desc_intro_point_t *ip) return; } if (ip->link_specifiers) { - SMARTLIST_FOREACH(ip->link_specifiers, hs_desc_link_specifier_t *, - ls, hs_desc_link_specifier_free(ls)); + SMARTLIST_FOREACH(ip->link_specifiers, link_specifier_t *, + ls, link_specifier_free(ls)); smartlist_free(ip->link_specifiers); } tor_cert_free(ip->auth_key_cert); @@ -2972,69 +2941,6 @@ hs_desc_authorized_client_free_(hs_desc_authorized_client_t *client) tor_free(client); } -/* Free the given descriptor link specifier. */ -void -hs_desc_link_specifier_free_(hs_desc_link_specifier_t *ls) -{ - if (ls == NULL) { - return; - } - tor_free(ls); -} - -/* Return a newly allocated descriptor link specifier using the given extend - * info and requested type. Return NULL on error. */ -hs_desc_link_specifier_t * -hs_desc_link_specifier_new(const extend_info_t *info, uint8_t type) -{ - hs_desc_link_specifier_t *ls = NULL; - - tor_assert(info); - - ls = tor_malloc_zero(sizeof(*ls)); - ls->type = type; - switch (ls->type) { - case LS_IPV4: - if (info->addr.family != AF_INET) { - goto err; - } - tor_addr_copy(&ls->u.ap.addr, &info->addr); - ls->u.ap.port = info->port; - break; - case LS_IPV6: - if (info->addr.family != AF_INET6) { - goto err; - } - tor_addr_copy(&ls->u.ap.addr, &info->addr); - ls->u.ap.port = info->port; - break; - case LS_LEGACY_ID: - /* Bug out if the identity digest is not set */ - if (BUG(tor_mem_is_zero(info->identity_digest, - sizeof(info->identity_digest)))) { - goto err; - } - memcpy(ls->u.legacy_id, info->identity_digest, sizeof(ls->u.legacy_id)); - break; - case LS_ED25519_ID: - /* ed25519 keys are optional for intro points */ - if (ed25519_public_key_is_zero(&info->ed_identity)) { - goto err; - } - memcpy(ls->u.ed25519_id, info->ed_identity.pubkey, - sizeof(ls->u.ed25519_id)); - break; - default: - /* Unknown type is code flow error. */ - tor_assert(0); - } - - return ls; - err: - tor_free(ls); - return NULL; -} - /* From the given descriptor, remove and free every introduction point. */ void hs_descriptor_clear_intro_points(hs_descriptor_t *desc) @@ -3050,59 +2956,3 @@ hs_descriptor_clear_intro_points(hs_descriptor_t *desc) smartlist_clear(ips); } } - -/* From a descriptor link specifier object spec, returned a newly allocated - * link specifier object that is the encoded representation of spec. Return - * NULL on error. */ -link_specifier_t * -hs_desc_lspec_to_trunnel(const hs_desc_link_specifier_t *spec) -{ - tor_assert(spec); - - link_specifier_t *ls = link_specifier_new(); - link_specifier_set_ls_type(ls, spec->type); - - switch (spec->type) { - case LS_IPV4: - link_specifier_set_un_ipv4_addr(ls, - tor_addr_to_ipv4h(&spec->u.ap.addr)); - link_specifier_set_un_ipv4_port(ls, spec->u.ap.port); - /* Four bytes IPv4 and two bytes port. */ - link_specifier_set_ls_len(ls, sizeof(spec->u.ap.addr.addr.in_addr) + - sizeof(spec->u.ap.port)); - break; - case LS_IPV6: - { - size_t addr_len = link_specifier_getlen_un_ipv6_addr(ls); - const uint8_t *in6_addr = tor_addr_to_in6_addr8(&spec->u.ap.addr); - uint8_t *ipv6_array = link_specifier_getarray_un_ipv6_addr(ls); - memcpy(ipv6_array, in6_addr, addr_len); - link_specifier_set_un_ipv6_port(ls, spec->u.ap.port); - /* Sixteen bytes IPv6 and two bytes port. */ - link_specifier_set_ls_len(ls, addr_len + sizeof(spec->u.ap.port)); - break; - } - case LS_LEGACY_ID: - { - size_t legacy_id_len = link_specifier_getlen_un_legacy_id(ls); - uint8_t *legacy_id_array = link_specifier_getarray_un_legacy_id(ls); - memcpy(legacy_id_array, spec->u.legacy_id, legacy_id_len); - link_specifier_set_ls_len(ls, legacy_id_len); - break; - } - case LS_ED25519_ID: - { - size_t ed25519_id_len = link_specifier_getlen_un_ed25519_id(ls); - uint8_t *ed25519_id_array = link_specifier_getarray_un_ed25519_id(ls); - memcpy(ed25519_id_array, spec->u.ed25519_id, ed25519_id_len); - link_specifier_set_ls_len(ls, ed25519_id_len); - break; - } - default: - tor_assert_nonfatal_unreached(); - link_specifier_free(ls); - ls = NULL; - } - - return ls; -} diff --git a/src/feature/hs/hs_descriptor.h b/src/feature/hs/hs_descriptor.h index 04a8e16d63..dbe0cb1c94 100644 --- a/src/feature/hs/hs_descriptor.h +++ b/src/feature/hs/hs_descriptor.h @@ -69,28 +69,10 @@ typedef enum { HS_DESC_AUTH_ED25519 = 1 } hs_desc_auth_type_t; -/* Link specifier object that contains information on how to extend to the - * relay that is the address, port and handshake type. */ -typedef struct hs_desc_link_specifier_t { - /* Indicate the type of link specifier. See trunnel ed25519_cert - * specification. */ - uint8_t type; - - /* It must be one of these types, can't be more than one. */ - union { - /* IP address and port of the relay use to extend. */ - tor_addr_port_t ap; - /* Legacy identity. A 20-byte SHA1 identity fingerprint. */ - uint8_t legacy_id[DIGEST_LEN]; - /* ed25519 identity. A 32-byte key. */ - uint8_t ed25519_id[ED25519_PUBKEY_LEN]; - } u; -} hs_desc_link_specifier_t; - /* Introduction point information located in a descriptor. */ typedef struct hs_desc_intro_point_t { /* Link specifier(s) which details how to extend to the relay. This list - * contains hs_desc_link_specifier_t object. It MUST have at least one. */ + * contains link_specifier_t objects. It MUST have at least one. */ smartlist_t *link_specifiers; /* Onion key of the introduction point used to extend to it for the ntor @@ -261,12 +243,6 @@ void hs_desc_encrypted_data_free_(hs_desc_encrypted_data_t *desc); #define hs_desc_encrypted_data_free(desc) \ FREE_AND_NULL(hs_desc_encrypted_data_t, hs_desc_encrypted_data_free_, (desc)) -void hs_desc_link_specifier_free_(hs_desc_link_specifier_t *ls); -#define hs_desc_link_specifier_free(ls) \ - FREE_AND_NULL(hs_desc_link_specifier_t, hs_desc_link_specifier_free_, (ls)) - -hs_desc_link_specifier_t *hs_desc_link_specifier_new( - const extend_info_t *info, uint8_t type); void hs_descriptor_clear_intro_points(hs_descriptor_t *desc); MOCK_DECL(int, @@ -299,9 +275,6 @@ void hs_desc_authorized_client_free_(hs_desc_authorized_client_t *client); FREE_AND_NULL(hs_desc_authorized_client_t, \ hs_desc_authorized_client_free_, (client)) -link_specifier_t *hs_desc_lspec_to_trunnel( - const hs_desc_link_specifier_t *spec); - hs_desc_authorized_client_t *hs_desc_build_fake_authorized_client(void); void hs_desc_build_authorized_client(const uint8_t *subcredential, const curve25519_public_key_t * diff --git a/src/feature/hs/hs_intropoint.c b/src/feature/hs/hs_intropoint.c index b28a5c2b80..c9cd3a0419 100644 --- a/src/feature/hs/hs_intropoint.c +++ b/src/feature/hs/hs_intropoint.c @@ -601,8 +601,8 @@ hs_intropoint_clear(hs_intropoint_t *ip) return; } tor_cert_free(ip->auth_key_cert); - SMARTLIST_FOREACH(ip->link_specifiers, hs_desc_link_specifier_t *, ls, - hs_desc_link_specifier_free(ls)); + SMARTLIST_FOREACH(ip->link_specifiers, link_specifier_t *, ls, + link_specifier_free(ls)); smartlist_free(ip->link_specifiers); memset(ip, 0, sizeof(hs_intropoint_t)); } diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 6e4da8856a..8cd863eef4 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -280,9 +280,10 @@ describe_intro_point(const hs_service_intro_point_t *ip) const char *legacy_id = NULL; SMARTLIST_FOREACH_BEGIN(ip->base.link_specifiers, - const hs_desc_link_specifier_t *, lspec) { - if (lspec->type == LS_LEGACY_ID) { - legacy_id = (const char *) lspec->u.legacy_id; + const link_specifier_t *, lspec) { + if (link_specifier_get_ls_type(lspec) == LS_LEGACY_ID) { + legacy_id = (const char *) + link_specifier_getconstarray_un_legacy_id(lspec); break; } } SMARTLIST_FOREACH_END(lspec); @@ -623,16 +624,16 @@ get_objects_from_ident(const hs_ident_circuit_t *ident, * encountered in the link specifier list. Return NULL if it can't be found. * * The caller does NOT have ownership of the object, the intro point does. */ -static hs_desc_link_specifier_t * +static link_specifier_t * get_link_spec_by_type(const hs_service_intro_point_t *ip, uint8_t type) { - hs_desc_link_specifier_t *lnk_spec = NULL; + link_specifier_t *lnk_spec = NULL; tor_assert(ip); SMARTLIST_FOREACH_BEGIN(ip->base.link_specifiers, - hs_desc_link_specifier_t *, ls) { - if (ls->type == type) { + link_specifier_t *, ls) { + if (link_specifier_get_ls_type(ls) == type) { lnk_spec = ls; goto end; } @@ -648,7 +649,7 @@ get_link_spec_by_type(const hs_service_intro_point_t *ip, uint8_t type) STATIC const node_t * get_node_from_intro_point(const hs_service_intro_point_t *ip) { - const hs_desc_link_specifier_t *ls; + const link_specifier_t *ls; tor_assert(ip); @@ -657,7 +658,8 @@ get_node_from_intro_point(const hs_service_intro_point_t *ip) return NULL; } /* XXX In the future, we want to only use the ed25519 ID (#22173). */ - return node_get_by_id((const char *) ls->u.legacy_id); + return node_get_by_id( + (const char *) link_specifier_getconstarray_un_legacy_id(ls)); } /* Given a service intro point, return the extend_info_t for it. This can @@ -1523,7 +1525,7 @@ remember_failing_intro_point(const hs_service_intro_point_t *ip, hs_service_descriptor_t *desc, time_t now) { time_t *time_of_failure, *prev_ptr; - const hs_desc_link_specifier_t *legacy_ls; + const link_specifier_t *legacy_ls; tor_assert(ip); tor_assert(desc); @@ -1532,22 +1534,13 @@ remember_failing_intro_point(const hs_service_intro_point_t *ip, *time_of_failure = now; legacy_ls = get_link_spec_by_type(ip, LS_LEGACY_ID); tor_assert(legacy_ls); - prev_ptr = digestmap_set(desc->intro_points.failed_id, - (const char *) legacy_ls->u.legacy_id, - time_of_failure); + prev_ptr = digestmap_set( + desc->intro_points.failed_id, + (const char *) link_specifier_getconstarray_un_legacy_id(legacy_ls), + time_of_failure); tor_free(prev_ptr); } -/* Copy the descriptor link specifier object from src to dst. */ -static void -link_specifier_copy(hs_desc_link_specifier_t *dst, - const hs_desc_link_specifier_t *src) -{ - tor_assert(dst); - tor_assert(src); - memcpy(dst, src, sizeof(hs_desc_link_specifier_t)); -} - /* Using a given descriptor signing keypair signing_kp, a service intro point * object ip and the time now, setup the content of an already allocated * descriptor intro desc_ip. @@ -1582,9 +1575,14 @@ setup_desc_intro_point(const ed25519_keypair_t *signing_kp, /* Copy link specifier(s). */ SMARTLIST_FOREACH_BEGIN(ip->base.link_specifiers, - const hs_desc_link_specifier_t *, ls) { - hs_desc_link_specifier_t *copy = tor_malloc_zero(sizeof(*copy)); - link_specifier_copy(copy, ls); + const link_specifier_t *, ls) { + if (BUG(!ls)) { + goto done; + } + link_specifier_t *copy = link_specifier_dup(ls); + if (BUG(!copy)) { + goto done; + } smartlist_add(desc_ip->link_specifiers, copy); } SMARTLIST_FOREACH_END(ls); diff --git a/src/test/hs_test_helpers.c b/src/test/hs_test_helpers.c index f2ae8398df..1c47c7d7d4 100644 --- a/src/test/hs_test_helpers.c +++ b/src/test/hs_test_helpers.c @@ -21,22 +21,33 @@ hs_helper_build_intro_point(const ed25519_keypair_t *signing_kp, time_t now, /* For a usable intro point we need at least two link specifiers: One legacy * keyid and one ipv4 */ { - hs_desc_link_specifier_t *ls_legacy = tor_malloc_zero(sizeof(*ls_legacy)); - hs_desc_link_specifier_t *ls_v4 = tor_malloc_zero(sizeof(*ls_v4)); - ls_legacy->type = LS_LEGACY_ID; - memcpy(ls_legacy->u.legacy_id, "0299F268FCA9D55CD157976D39AE92B4B455B3A8", - DIGEST_LEN); - ls_v4->u.ap.port = 9001; - int family = tor_addr_parse(&ls_v4->u.ap.addr, addr); + tor_addr_t a; + tor_addr_make_unspec(&a); + link_specifier_t *ls_legacy = link_specifier_new(); + /* TODO: this name is wrong: it is sometimes an IPv6 address */ + link_specifier_t *ls_v4 = link_specifier_new(); + link_specifier_set_ls_type(ls_legacy, LS_LEGACY_ID); + memcpy(link_specifier_getarray_un_legacy_id(ls_legacy), + /* TODO: this code is wrong: it copies hex into binary */ + "0299F268FCA9D55CD157976D39AE92B4B455B3A8", + link_specifier_getlen_un_legacy_id(ls_legacy)); + int family = tor_addr_parse(&a, addr); switch (family) { case AF_INET: - ls_v4->type = LS_IPV4; + link_specifier_set_ls_type(ls_v4, LS_IPV4); + link_specifier_set_un_ipv4_addr(ls_v4, tor_addr_to_ipv4h(&a)); + link_specifier_set_un_ipv4_port(ls_v4, 9001); break; case AF_INET6: - ls_v4->type = LS_IPV6; + link_specifier_set_ls_type(ls_v4, LS_IPV6); + memcpy(link_specifier_getarray_un_ipv6_addr(ls_v4), + tor_addr_to_in6_addr8(&a), + link_specifier_getlen_un_ipv6_addr(ls_v4)); + link_specifier_set_un_ipv6_port(ls_v4, 9001); break; default: /* Stop the test, not suppose to have an error. */ + /* TODO: just tt_fail(), because this code is confusing */ tt_int_op(family, OP_EQ, AF_INET); } smartlist_add(ip->link_specifiers, ls_legacy); @@ -153,8 +164,11 @@ hs_helper_build_hs_desc_impl(unsigned int no_ip, /* Add four intro points. */ smartlist_add(desc->encrypted_data.intro_points, hs_helper_build_intro_point(signing_kp, now, "1.2.3.4", 0)); +/* IPv6-only introduction points are not supported yet, see #23588 */ +#if 0 smartlist_add(desc->encrypted_data.intro_points, hs_helper_build_intro_point(signing_kp, now, "[2600::1]", 0)); +#endif smartlist_add(desc->encrypted_data.intro_points, hs_helper_build_intro_point(signing_kp, now, "3.2.1.4", 1)); smartlist_add(desc->encrypted_data.intro_points, @@ -202,7 +216,6 @@ void hs_helper_desc_equal(const hs_descriptor_t *desc1, const hs_descriptor_t *desc2) { - char *addr1 = NULL, *addr2 = NULL; /* Plaintext data section. */ tt_int_op(desc1->plaintext_data.version, OP_EQ, desc2->plaintext_data.version); @@ -291,35 +304,57 @@ hs_helper_desc_equal(const hs_descriptor_t *desc1, tt_int_op(smartlist_len(ip1->link_specifiers), ==, smartlist_len(ip2->link_specifiers)); for (int j = 0; j < smartlist_len(ip1->link_specifiers); j++) { - hs_desc_link_specifier_t *ls1 = smartlist_get(ip1->link_specifiers, j), - *ls2 = smartlist_get(ip2->link_specifiers, j); - tt_int_op(ls1->type, ==, ls2->type); - switch (ls1->type) { + link_specifier_t *ls1 = smartlist_get(ip1->link_specifiers, j), + *ls2 = smartlist_get(ip2->link_specifiers, j); + tt_int_op(link_specifier_get_ls_type(ls1), ==, + link_specifier_get_ls_type(ls2)); + switch (link_specifier_get_ls_type(ls1)) { case LS_IPV4: + { + uint32_t addr1 = link_specifier_get_un_ipv4_addr(ls1); + uint32_t addr2 = link_specifier_get_un_ipv4_addr(ls2); + tt_int_op(addr1, OP_EQ, addr2); + uint16_t port1 = link_specifier_get_un_ipv4_port(ls1); + uint16_t port2 = link_specifier_get_un_ipv4_port(ls2); + tt_int_op(port1, ==, port2); + } + break; case LS_IPV6: { - addr1 = tor_addr_to_str_dup(&ls1->u.ap.addr); - addr2 = tor_addr_to_str_dup(&ls2->u.ap.addr); - tt_str_op(addr1, OP_EQ, addr2); - tor_free(addr1); - tor_free(addr2); - tt_int_op(ls1->u.ap.port, ==, ls2->u.ap.port); + const uint8_t *addr1 = + link_specifier_getconstarray_un_ipv6_addr(ls1); + const uint8_t *addr2 = + link_specifier_getconstarray_un_ipv6_addr(ls2); + tt_int_op(link_specifier_getlen_un_ipv6_addr(ls1), OP_EQ, + link_specifier_getlen_un_ipv6_addr(ls2)); + tt_mem_op(addr1, OP_EQ, addr2, + link_specifier_getlen_un_ipv6_addr(ls1)); + uint16_t port1 = link_specifier_get_un_ipv6_port(ls1); + uint16_t port2 = link_specifier_get_un_ipv6_port(ls2); + tt_int_op(port1, ==, port2); } break; case LS_LEGACY_ID: - tt_mem_op(ls1->u.legacy_id, OP_EQ, ls2->u.legacy_id, - sizeof(ls1->u.legacy_id)); + { + const uint8_t *id1 = + link_specifier_getconstarray_un_legacy_id(ls1); + const uint8_t *id2 = + link_specifier_getconstarray_un_legacy_id(ls2); + tt_int_op(link_specifier_getlen_un_legacy_id(ls1), OP_EQ, + link_specifier_getlen_un_legacy_id(ls2)); + tt_mem_op(id1, OP_EQ, id2, + link_specifier_getlen_un_legacy_id(ls1)); + } break; default: /* Unknown type, caught it and print its value. */ - tt_int_op(ls1->type, OP_EQ, -1); + tt_int_op(link_specifier_get_ls_type(ls1), OP_EQ, -1); } } } } done: - tor_free(addr1); - tor_free(addr2); + ; } diff --git a/src/test/test_hs_client.c b/src/test/test_hs_client.c index 2f2bb45581..8362b6cbda 100644 --- a/src/test/test_hs_client.c +++ b/src/test/test_hs_client.c @@ -403,6 +403,9 @@ test_client_pick_intro(void *arg) /* 2) Mark all intro points except _the chosen one_ as failed. Then query the * desc and get a random intro: check that we got _the chosen one_. */ { + /* Tell hs_get_extend_info_from_lspecs() to skip the private address check. + */ + get_options_mutable()->ExtendAllowPrivateAddresses = 1; /* Pick the chosen intro point and get its ei */ hs_desc_intro_point_t *chosen_intro_point = smartlist_get(desc->encrypted_data.intro_points, 0); @@ -476,6 +479,7 @@ test_client_pick_intro(void *arg) SMARTLIST_FOREACH_BEGIN(desc->encrypted_data.intro_points, hs_desc_intro_point_t *, ip) { extend_info_t *intro_ei = desc_intro_point_to_extend_info(ip); + tt_assert(intro_ei); if (intro_ei) { const char *ptr; char ip_addr[TOR_ADDR_BUF_LEN]; diff --git a/src/test/test_hs_descriptor.c b/src/test/test_hs_descriptor.c index de584ed47a..09c6c3e700 100644 --- a/src/test/test_hs_descriptor.c +++ b/src/test/test_hs_descriptor.c @@ -178,115 +178,6 @@ test_descriptor_padding(void *arg) return; } -static void -test_link_specifier(void *arg) -{ - ssize_t ret; - hs_desc_link_specifier_t spec; - smartlist_t *link_specifiers = smartlist_new(); - char buf[256]; - char *b64 = NULL; - link_specifier_t *ls = NULL; - - (void) arg; - - /* Always this port. */ - spec.u.ap.port = 42; - smartlist_add(link_specifiers, &spec); - - /* Test IPv4 for starter. */ - { - uint32_t ipv4; - - spec.type = LS_IPV4; - ret = tor_addr_parse(&spec.u.ap.addr, "1.2.3.4"); - tt_int_op(ret, OP_EQ, AF_INET); - b64 = encode_link_specifiers(link_specifiers); - tt_assert(b64); - - /* Decode it and validate the format. */ - ret = base64_decode(buf, sizeof(buf), b64, strlen(b64)); - tt_int_op(ret, OP_GT, 0); - /* First byte is the number of link specifier. */ - tt_int_op(get_uint8(buf), OP_EQ, 1); - ret = link_specifier_parse(&ls, (uint8_t *) buf + 1, ret - 1); - tt_int_op(ret, OP_EQ, 8); - /* Should be 2 bytes for port and 4 bytes for IPv4. */ - tt_int_op(link_specifier_get_ls_len(ls), OP_EQ, 6); - ipv4 = link_specifier_get_un_ipv4_addr(ls); - tt_int_op(tor_addr_to_ipv4h(&spec.u.ap.addr), OP_EQ, ipv4); - tt_int_op(link_specifier_get_un_ipv4_port(ls), OP_EQ, spec.u.ap.port); - - link_specifier_free(ls); - ls = NULL; - tor_free(b64); - } - - /* Test IPv6. */ - { - uint8_t ipv6[16]; - - spec.type = LS_IPV6; - ret = tor_addr_parse(&spec.u.ap.addr, "[1:2:3:4::]"); - tt_int_op(ret, OP_EQ, AF_INET6); - b64 = encode_link_specifiers(link_specifiers); - tt_assert(b64); - - /* Decode it and validate the format. */ - ret = base64_decode(buf, sizeof(buf), b64, strlen(b64)); - tt_int_op(ret, OP_GT, 0); - /* First byte is the number of link specifier. */ - tt_int_op(get_uint8(buf), OP_EQ, 1); - ret = link_specifier_parse(&ls, (uint8_t *) buf + 1, ret - 1); - tt_int_op(ret, OP_EQ, 20); - /* Should be 2 bytes for port and 16 bytes for IPv6. */ - tt_int_op(link_specifier_get_ls_len(ls), OP_EQ, 18); - for (unsigned int i = 0; i < sizeof(ipv6); i++) { - ipv6[i] = link_specifier_get_un_ipv6_addr(ls, i); - } - tt_mem_op(tor_addr_to_in6_addr8(&spec.u.ap.addr), OP_EQ, ipv6, - sizeof(ipv6)); - tt_int_op(link_specifier_get_un_ipv6_port(ls), OP_EQ, spec.u.ap.port); - - link_specifier_free(ls); - ls = NULL; - tor_free(b64); - } - - /* Test legacy. */ - { - uint8_t *id; - - spec.type = LS_LEGACY_ID; - memset(spec.u.legacy_id, 'Y', sizeof(spec.u.legacy_id)); - b64 = encode_link_specifiers(link_specifiers); - tt_assert(b64); - - /* Decode it and validate the format. */ - ret = base64_decode(buf, sizeof(buf), b64, strlen(b64)); - tt_int_op(ret, OP_GT, 0); - /* First byte is the number of link specifier. */ - tt_int_op(get_uint8(buf), OP_EQ, 1); - ret = link_specifier_parse(&ls, (uint8_t *) buf + 1, ret - 1); - /* 20 bytes digest + 1 byte type + 1 byte len. */ - tt_int_op(ret, OP_EQ, 22); - tt_int_op(link_specifier_getlen_un_legacy_id(ls), OP_EQ, DIGEST_LEN); - /* Digest length is 20 bytes. */ - tt_int_op(link_specifier_get_ls_len(ls), OP_EQ, DIGEST_LEN); - id = link_specifier_getarray_un_legacy_id(ls); - tt_mem_op(spec.u.legacy_id, OP_EQ, id, DIGEST_LEN); - - link_specifier_free(ls); - ls = NULL; - tor_free(b64); - } - - done: - link_specifier_free(ls); - tor_free(b64); - smartlist_free(link_specifiers); -} - static void test_encode_descriptor(void *arg) { @@ -932,8 +823,6 @@ struct testcase_t hs_descriptor[] = { /* Encoding tests. */ { "cert_encoding", test_cert_encoding, TT_FORK, NULL, NULL }, - { "link_specifier", test_link_specifier, TT_FORK, - NULL, NULL }, { "encode_descriptor", test_encode_descriptor, TT_FORK, NULL, NULL }, { "descriptor_padding", test_descriptor_padding, TT_FORK, diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index e8bdcd86e2..57132e6197 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -328,17 +328,18 @@ helper_create_service_with_clients(int num_clients) static hs_service_intro_point_t * helper_create_service_ip(void) { - hs_desc_link_specifier_t *ls; + link_specifier_t *ls; hs_service_intro_point_t *ip = service_intro_point_new(NULL); tor_assert(ip); /* Add a first unused link specifier. */ - ls = tor_malloc_zero(sizeof(*ls)); - ls->type = LS_IPV4; + ls = link_specifier_new(); + link_specifier_set_ls_type(ls, LS_IPV4); smartlist_add(ip->base.link_specifiers, ls); /* Add a second link specifier used by a test. */ - ls = tor_malloc_zero(sizeof(*ls)); - ls->type = LS_LEGACY_ID; - memset(ls->u.legacy_id, 'A', sizeof(ls->u.legacy_id)); + ls = link_specifier_new(); + link_specifier_set_ls_type(ls, LS_LEGACY_ID); + memset(link_specifier_getarray_un_legacy_id(ls), 'A', + link_specifier_getlen_un_legacy_id(ls)); smartlist_add(ip->base.link_specifiers, ls); return ip; @@ -811,10 +812,11 @@ test_helper_functions(void *arg) const node_t *node = get_node_from_intro_point(ip); tt_ptr_op(node, OP_EQ, &mock_node); SMARTLIST_FOREACH_BEGIN(ip->base.link_specifiers, - hs_desc_link_specifier_t *, ls) { - if (ls->type == LS_LEGACY_ID) { + link_specifier_t *, ls) { + if (link_specifier_get_ls_type(ls) == LS_LEGACY_ID) { /* Change legacy id in link specifier which is not the mock node. */ - memset(ls->u.legacy_id, 'B', sizeof(ls->u.legacy_id)); + memset(link_specifier_getarray_un_legacy_id(ls), 'B', + link_specifier_getlen_un_legacy_id(ls)); } } SMARTLIST_FOREACH_END(ls); node = get_node_from_intro_point(ip);