tor-commits December 2017

tor-commits@lists.torproject.org

14 participants
2422 discussions

[tor/maint-0.3.2] hs-v3: Don't lookup an intro point while cleaning it up
by nickm＠torproject.org 12 Dec '17

12 Dec '17

commit b6fd78ea301bd089df5facf8997e31d889c8b760 Author: George Kadianakis <desnacked(a)riseup.net> Date: Tue Dec 12 16:12:30 2017 +0200 hs-v3: Don't lookup an intro point while cleaning it up Commit e80893e51b0c0320838cbed8c46fd5b0fe608bef made tor call hs_service_intro_circ_has_closed() when we mark for close a circuit. When we cleanup intro points, we iterate over the descriptor's map of intro points and we can possibly mark for close a circuit. This was problematic because we would MAP_DEL_CURRENT() the intro point then free it and finally mark for close the circuit which would lookup the intro point that we just free in the map we are iterating over. This can't be done and leads to a use-after-free because the intro point will be returned successfully due to the fact that we are still in the loop iterating. In other words, MAP_DEL_CURRENT() followed by a digest256map_get() of the same object should never be done in the same loop. Fixes #24595 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/hs_service.c | 50 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 99965ddc3..dd836bf84 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -1815,6 +1815,15 @@ intro_point_should_expire(const hs_service_intro_point_t *ip, static void cleanup_intro_points(hs_service_t *service, time_t now) { + /* List of intro points to close. We can't mark the intro circuits for close + * in the modify loop because doing so calls + * hs_service_intro_circ_has_closed() which does a digest256map_get() on the + * intro points map (that we are iterating over). This can't be done in a + * single iteration after a MAP_DEL_CURRENT, the object will still be + * returned leading to a use-after-free. So, we close the circuits and free + * the intro points after the loop if any. */ + smartlist_t *ips_to_free = smartlist_new(); + tor_assert(service); /* For both descriptors, cleanup the intro points. */ @@ -1824,7 +1833,6 @@ cleanup_intro_points(hs_service_t *service, time_t now) DIGEST256MAP_FOREACH_MODIFY(desc->intro_points.map, key, hs_service_intro_point_t *, ip) { const node_t *node = get_node_from_intro_point(ip); - origin_circuit_t *ocirc = hs_circ_service_get_intro_circ(ip); int has_expired = intro_point_should_expire(ip, now); /* We cleanup an intro point if it has expired or if we do not know the @@ -1845,26 +1853,34 @@ cleanup_intro_points(hs_service_t *service, time_t now) remember_failing_intro_point(ip, desc, approx_time()); } - /* Remove intro point from descriptor map. We'll add it to the failed - * map if we retried it too many times. */ + /* Remove intro point from descriptor map and add it to the list of + * ips to free for which we'll also try to close the intro circuit. */ MAP_DEL_CURRENT(key); - service_intro_point_free(ip); - - /* XXX: Legacy code does NOT do that, it keeps the circuit open until - * a new descriptor is uploaded and then closed all expiring intro - * point circuit. Here, we close immediately and because we just - * discarded the intro point, a new one will be selected, a new - * descriptor created and uploaded. There is no difference to an - * attacker between the timing of a new consensus and intro point - * rotation (possibly?). */ - if (ocirc && !TO_CIRCUIT(ocirc)->marked_for_close) { - /* After this, no new cells will be handled on the circuit. */ - circuit_mark_for_close(TO_CIRCUIT(ocirc), END_CIRC_REASON_FINISHED); - } - continue; + smartlist_add(ips_to_free, ip); } } DIGEST256MAP_FOREACH_END; } FOR_EACH_DESCRIPTOR_END; + + /* Go over the intro points to free and close their circuit if any. */ + SMARTLIST_FOREACH_BEGIN(ips_to_free, hs_service_intro_point_t *, ip) { + /* See if we need to close the intro point circuit as well */ + + /* XXX: Legacy code does NOT close circuits like this: it keeps the circuit + * open until a new descriptor is uploaded and then closed all expiring + * intro point circuit. Here, we close immediately and because we just + * discarded the intro point, a new one will be selected, a new descriptor + * created and uploaded. There is no difference to an attacker between the + * timing of a new consensus and intro point rotation (possibly?). */ + origin_circuit_t *ocirc = hs_circ_service_get_intro_circ(ip); + if (ocirc && !TO_CIRCUIT(ocirc)->marked_for_close) { + circuit_mark_for_close(TO_CIRCUIT(ocirc), END_CIRC_REASON_FINISHED); + } + + /* Cleanup the intro point */ + service_intro_point_free(ip); + } SMARTLIST_FOREACH_END(ip); + + smartlist_free(ips_to_free); } /* Set the next rotation time of the descriptors for the given service for the

1 0

[tor/master] hs-v3: Don't lookup an intro point while cleaning it up
by nickm＠torproject.org 12 Dec '17

12 Dec '17

1 0

[tor/maint-0.3.2] Merge remote-tracking branch 'dgoulet/bug24595_032_02' into maint-0.3.2
by nickm＠torproject.org 12 Dec '17

12 Dec '17

commit cf5ab5934e80c6c04825d9db83329627fe6b9729 Merge: 4c54021ba b6fd78ea3 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Dec 12 09:32:24 2017 -0500 Merge remote-tracking branch 'dgoulet/bug24595_032_02' into maint-0.3.2 src/or/hs_service.c | 50 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 17 deletions(-)

1 0

[tor/master] Merge remote-tracking branch 'dgoulet/bug24595_032_02' into maint-0.3.2
by nickm＠torproject.org 12 Dec '17

12 Dec '17

1 0

[tor/master] Merge branch 'maint-0.3.2'
by nickm＠torproject.org 12 Dec '17

12 Dec '17

commit 13455c0f1a191824d383339c9efcc3b31ecd8e6f Merge: e622f208a cf5ab5934 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Dec 12 09:32:29 2017 -0500 Merge branch 'maint-0.3.2' src/or/hs_service.c | 50 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 17 deletions(-)

1 0

[tor/release-0.3.2] hs-v3: Don't lookup an intro point while cleaning it up
by nickm＠torproject.org 12 Dec '17

12 Dec '17

1 0

[tor/master] fixup! Stop waiting for microdescs if the consensus supports IPv6 ORPorts
by nickm＠torproject.org 12 Dec '17

12 Dec '17

commit 94ad47e332940aee3e1a04beeebac03b3803e085 Author: teor <teor2345(a)gmail.com> Date: Mon Dec 11 00:53:49 2017 +1100 fixup! Stop waiting for microdescs if the consensus supports IPv6 ORPorts --- changes/bug23827 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/bug23827 b/changes/bug23827 index 7c626e1b3..75279abb8 100644 --- a/changes/bug23827 +++ b/changes/bug23827 @@ -5,4 +5,4 @@ - Make IPv6-only clients wait for microdescs for relays, even if we were previously using descriptors (or were using them as a bridge) and have a cached descriptor for them. - Implements #23827. + Implements 23827.

1 0

[tor/master] Add networkstatus_consensus_has_ipv6() and unit tests
by nickm＠torproject.org 12 Dec '17

12 Dec '17

commit 28d4355a6ebe69c84d0ae31d7a0763f0a17f0151 Author: teor <teor2345(a)gmail.com> Date: Sat Dec 2 23:01:07 2017 +1100 Add networkstatus_consensus_has_ipv6() and unit tests networkstatus_consensus_has_ipv6() tells us whether the consensus method of our current consensus supports IPv6 ORPorts in the consensus. Part of #23827. --- src/or/networkstatus.c | 26 ++++++++++++++ src/or/networkstatus.h | 1 + src/test/test_dir.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 2843d0626..61cca1b9b 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1507,6 +1507,32 @@ networkstatus_consensus_is_already_downloading(const char *resource) return answer; } +/* Does the current, reasonably live consensus have IPv6 addresses? + * Returns 1 if there is a reasonably live consensus and its consensus method + * includes IPv6 addresses in the consensus. + * Otherwise, if there is no consensus, or the method does not include IPv6 + * addresses, returns 0. */ +int +networkstatus_consensus_has_ipv6(const or_options_t* options) +{ + const networkstatus_t *cons = networkstatus_get_reasonably_live_consensus( + approx_time(), + usable_consensus_flavor()); + + /* If we have no consensus, we have no IPv6 in it */ + if (!cons) { + return 0; + } + + /* Different flavours of consensus gained IPv6 at different times */ + if (we_use_microdescriptors_for_circuits(options)) { + return + cons->consensus_method >= MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS; + } else { + return cons->consensus_method >= MIN_METHOD_FOR_A_LINES; + } +} + /** Given two router status entries for the same router identity, return 1 if * if the contents have changed between them. Otherwise, return 0. */ static int diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index 791e32adc..1851a55e8 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -93,6 +93,7 @@ int networkstatus_consensus_can_use_multiple_directories( MOCK_DECL(int, networkstatus_consensus_can_use_extra_fallbacks,( const or_options_t *options)); int networkstatus_consensus_is_already_downloading(const char *resource); +int networkstatus_consensus_has_ipv6(const or_options_t* options); #define NSSET_FROM_CACHE 1 #define NSSET_WAS_WAITING_FOR_CERTS 2 diff --git a/src/test/test_dir.c b/src/test/test_dir.c index ee7aea5b8..0da4e7e15 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -6174,6 +6174,101 @@ test_dir_platform_str(void *arg) ; } +static networkstatus_t *mock_networkstatus; + +static networkstatus_t * +mock_networkstatus_get_latest_consensus_by_flavor(consensus_flavor_t f) +{ + (void)f; + return mock_networkstatus; +} + +static void +test_dir_networkstatus_consensus_has_ipv6(void *arg) +{ + (void)arg; + + int has_ipv6 = 0; + + /* Init options and networkstatus */ + or_options_t our_options; + mock_options = &our_options; + reset_options(mock_options, &mock_get_options_calls); + MOCK(get_options, mock_get_options); + + networkstatus_t our_networkstatus; + mock_networkstatus = &our_networkstatus; + memset(mock_networkstatus, 0, sizeof(*mock_networkstatus)); + MOCK(networkstatus_get_latest_consensus_by_flavor, + mock_networkstatus_get_latest_consensus_by_flavor); + + /* A live consensus */ + mock_networkstatus->valid_after = time(NULL) - 3600; + mock_networkstatus->valid_until = time(NULL) + 3600; + + /* Test the bounds for A lines in the NS consensus */ + mock_options->UseMicrodescriptors = 0; + + mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(has_ipv6); + + mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES + 1; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(has_ipv6); + + mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES + 20; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(has_ipv6); + + mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES - 1; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(!has_ipv6); + + /* Test the bounds for A lines in the microdesc consensus */ + mock_options->UseMicrodescriptors = 1; + + mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(has_ipv6); + + mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS + 1; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(has_ipv6); + + mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS + 20; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(has_ipv6); + + mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS - 1; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(!has_ipv6); + + /* Test the edge cases */ + mock_options->UseMicrodescriptors = 1; + mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS; + + /* Reasonably live */ + mock_networkstatus->valid_until = time(NULL) - 60; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(has_ipv6); + + /* Not reasonably live */ + mock_networkstatus->valid_after = time(NULL) - 24*60*60 - 3600; + mock_networkstatus->valid_until = time(NULL) - 24*60*60 - 60; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(!has_ipv6); + + /* NULL consensus */ + mock_networkstatus = NULL; + has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); + tt_assert(!has_ipv6); + + done: + UNMOCK(get_options); + UNMOCK(networkstatus_get_latest_consensus_by_flavor); +} + #define DIR_LEGACY(name) \ { #name, test_dir_ ## name , TT_FORK, NULL, NULL } @@ -6241,6 +6336,7 @@ struct testcase_t dir_tests[] = { DIR(assumed_flags, 0), DIR(networkstatus_compute_bw_weights_v10, 0), DIR(platform_str, 0), + DIR(networkstatus_consensus_has_ipv6, 0), END_OF_TESTCASES };

1 0

[tor/master] Merge remote-tracking branch 'teor/bug23827-v2'
by nickm＠torproject.org 12 Dec '17

12 Dec '17

commit e622f208a316cf3388003c1cdd1961eaaa8c370d Merge: 9c604e2bb 94ad47e33 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Dec 12 09:27:03 2017 -0500 Merge remote-tracking branch 'teor/bug23827-v2' changes/bug23827 | 8 ++++ src/or/networkstatus.c | 26 +++++++++++++ src/or/networkstatus.h | 1 + src/or/policies.c | 31 +++++++++++---- src/test/test_dir.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 160 insertions(+), 7 deletions(-)

1 0

[tor/master] fixup! Add networkstatus_consensus_has_ipv6() and unit tests
by nickm＠torproject.org 12 Dec '17

12 Dec '17

commit bbc781885dd64142c687c21339d809782444e0c7 Author: teor <teor2345(a)gmail.com> Date: Mon Dec 11 00:51:40 2017 +1100 fixup! Add networkstatus_consensus_has_ipv6() and unit tests This is what happens when you don't run: make check-spaces make check-changes --- changes/bug23827 | 2 +- src/test/test_dir.c | 15 ++++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/changes/bug23827 b/changes/bug23827 index 6954ef891..7c626e1b3 100644 --- a/changes/bug23827 +++ b/changes/bug23827 @@ -1,7 +1,7 @@ o Minor feature (IPv6): - When a consensus has IPv6 ORPorts, make IPv6-only clients use them, rather than waiting to download microdescriptors. - Implements #23827. + Implements 23827. - Make IPv6-only clients wait for microdescs for relays, even if we were previously using descriptors (or were using them as a bridge) and have a cached descriptor for them. diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 0da4e7e15..c85f7f065 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -6228,25 +6228,30 @@ test_dir_networkstatus_consensus_has_ipv6(void *arg) /* Test the bounds for A lines in the microdesc consensus */ mock_options->UseMicrodescriptors = 1; - mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS; + mock_networkstatus->consensus_method = + MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS; has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); tt_assert(has_ipv6); - mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS + 1; + mock_networkstatus->consensus_method = + MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS + 1; has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); tt_assert(has_ipv6); - mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS + 20; + mock_networkstatus->consensus_method = + MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS + 20; has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); tt_assert(has_ipv6); - mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS - 1; + mock_networkstatus->consensus_method = + MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS - 1; has_ipv6 = networkstatus_consensus_has_ipv6(get_options()); tt_assert(!has_ipv6); /* Test the edge cases */ mock_options->UseMicrodescriptors = 1; - mock_networkstatus->consensus_method = MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS; + mock_networkstatus->consensus_method = + MIN_METHOD_FOR_A_LINES_IN_MICRODESC_CONSENSUS; /* Reasonably live */ mock_networkstatus->valid_until = time(NULL) - 60;

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

tor-commits December 2017