From 46fea1dfeeae1b575d427a9c3c6b4a8d1ab7f2b9 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 19 Sep 2019 15:24:01 +1000 Subject: [PATCH 1/4] test: dirserv_router_has_valid_address() with zero-family addresses Sometimes tor doesn't initialise an address, so its family is zero. Failing test for 31793. Future commits will fix the code. --- src/test/test_address.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/test/test_address.c b/src/test/test_address.c index ef6daa06b4..c186cda73b 100644 --- a/src/test/test_address.c +++ b/src/test/test_address.c @@ -1271,10 +1271,44 @@ test_address_dirserv_router_addr_private(void *ignored) (void)ignored; /* A stub routerinfo structure, with only its address fields set. */ routerinfo_t *ri = NULL; + CHECK_RI_ADDR("1.0.0.1", 0); CHECK_RI_ADDR("10.0.0.1", -1); + CHECK_RI_ADDR6("2600::1", 0); CHECK_RI_ADDR6("fe80::1", -1); + + /* Null addresses */ + /* IPv4 null fails, regardless of IPv6 */ + CHECK_RI_ADDR("0.0.0.0", -1); + + { + ri = tor_malloc_zero(sizeof(routerinfo_t)); + ri->addr = 0; /* 0.0.0.0 */ + tor_addr_parse(&ri->ipv6_addr, "::"); + tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, -1); + tor_free(ri); + } + + /* IPv6 null succeeds, because IPv4 is not null */ + CHECK_RI_ADDR6("::", 0); + + /* Byte-zeroed null addresses */ + /* IPv4 null fails, regardless of IPv6 */ + { + ri = tor_malloc_zero(sizeof(routerinfo_t)); + tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, -1); + tor_free(ri); + } + + /* IPv6 null succeeds, because IPv4 is not internal */ + { + ri = tor_malloc_zero(sizeof(routerinfo_t)); + ri->addr = 16777217; /* 1.0.0.1 */ + tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, 0); + tor_free(ri); + } + done: tor_free(ri); } From 03c2b1be8d1e749b4f468cfe522da9e8375a83ad Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 19 Sep 2019 15:27:39 +1000 Subject: [PATCH 2/4] dirauth: reorder the checks in dirserv_router_has_valid_address() To avoid a bug warning. Fixes 31793. Bug not in any release, no changes file required. --- src/feature/dirauth/process_descs.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/feature/dirauth/process_descs.c b/src/feature/dirauth/process_descs.c index e1a02179b0..1c026372b0 100644 --- a/src/feature/dirauth/process_descs.c +++ b/src/feature/dirauth/process_descs.c @@ -432,20 +432,22 @@ STATIC int dirserv_router_has_valid_address(routerinfo_t *ri) { tor_addr_t addr; + if (get_options()->DirAllowPrivateAddresses) return 0; /* whatever it is, we're fine with it */ - tor_addr_from_ipv4h(&addr, ri->addr); - if (tor_addr_is_internal(&addr, 0) || tor_addr_is_null(&addr)) { + tor_addr_from_ipv4h(&addr, ri->addr); + if (tor_addr_is_null(&addr) || tor_addr_is_internal(&addr, 0)) { log_info(LD_DIRSERV, "Router %s published internal IPv4 address. Refusing.", router_describe(ri)); return -1; /* it's a private IP, we should reject it */ } + /* We only check internal v6 on non-null addresses because we do not require * IPv6 and null IPv6 is normal. */ - if (tor_addr_is_internal(&ri->ipv6_addr, 0) && - !tor_addr_is_null(&ri->ipv6_addr)) { + if (!tor_addr_is_null(&ri->ipv6_addr) && + tor_addr_is_internal(&ri->ipv6_addr, 0)) { log_info(LD_DIRSERV, "Router %s published internal IPv6 address. Refusing.", router_describe(ri)); From c2ceede37623935a63c05187d89ba7b6d86df7e8 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 19 Sep 2019 15:39:36 +1000 Subject: [PATCH 3/4] test: refactor dirserv_router_has_valid_address() tests Cleanup after 31793. --- src/test/test_address.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/src/test/test_address.c b/src/test/test_address.c index c186cda73b..ab95113dcd 100644 --- a/src/test/test_address.c +++ b/src/test/test_address.c @@ -1245,25 +1245,29 @@ test_address_tor_node_in_same_network_family(void *ignored) helper_free_mock_node(node_b); } -#define CHECK_RI_ADDR(addr_str, rv) STMT_BEGIN \ +/* Test dirserv_router_has_valid_address() on a stub routerinfo, with only its + * address fields set. Use IPv4 ipv4_addr_str and IPv6 ipv6_addr_str. + * Fail if it does not return rv. */ +#define TEST_ROUTER_VALID_ADDRESS_HELPER(ipv4_addr_str, ipv6_addr_str, rv) \ + STMT_BEGIN \ ri = tor_malloc_zero(sizeof(routerinfo_t)); \ tor_addr_t addr; \ - tor_addr_parse(&addr, (addr_str)); \ + tor_addr_parse(&addr, (ipv4_addr_str)); \ ri->addr = tor_addr_to_ipv4h(&addr); \ - tor_addr_make_null(&ri->ipv6_addr, AF_INET6); \ - tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, (rv)); \ + tor_addr_parse(&ri->ipv6_addr, (ipv6_addr_str)); \ + tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, (rv)); \ tor_free(ri); \ STMT_END -/* XXX: Here, we use a non-internal IPv4 as dirserv_router_has_valid_address() - * will check internal/null IPv4 first. */ -#define CHECK_RI_ADDR6(addr_str, rv) STMT_BEGIN \ - ri = tor_malloc_zero(sizeof(routerinfo_t)); \ - ri->addr = 16777217; /* 1.0.0.1 */ \ - tor_addr_parse(&ri->ipv6_addr, (addr_str)); \ - tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, (rv)); \ - tor_free(ri); \ - STMT_END +/* Like TEST_ROUTER_VALID_ADDRESS_HELPER(), but always passes a null + * IPv6 address. */ +#define CHECK_RI_ADDR(ipv4_addr_str, rv) \ + TEST_ROUTER_VALID_ADDRESS_HELPER(ipv4_addr_str, "::", rv) + +/* Like TEST_ROUTER_VALID_ADDRESS_HELPER(), but always passes a non-internal + * IPv4 address, so that the IPv6 check is reached. */ +#define CHECK_RI_ADDR6(ipv6_addr_str, rv) \ + TEST_ROUTER_VALID_ADDRESS_HELPER("1.0.0.1", ipv6_addr_str, rv) static void test_address_dirserv_router_addr_private(void *ignored) @@ -1281,14 +1285,7 @@ test_address_dirserv_router_addr_private(void *ignored) /* Null addresses */ /* IPv4 null fails, regardless of IPv6 */ CHECK_RI_ADDR("0.0.0.0", -1); - - { - ri = tor_malloc_zero(sizeof(routerinfo_t)); - ri->addr = 0; /* 0.0.0.0 */ - tor_addr_parse(&ri->ipv6_addr, "::"); - tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, -1); - tor_free(ri); - } + TEST_ROUTER_VALID_ADDRESS_HELPER("0.0.0.0", "::", -1); /* IPv6 null succeeds, because IPv4 is not null */ CHECK_RI_ADDR6("::", 0); From 69a1f9c8a7a9c8221029ce83f19d0e8d3e93f917 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 19 Sep 2019 16:17:17 +1000 Subject: [PATCH 4/4] test: test dirserv_router_has_valid_address() with DirAllowPrivateAddresses Part of 31793. --- src/test/test_address.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/src/test/test_address.c b/src/test/test_address.c index ab95113dcd..32fb4aa232 100644 --- a/src/test/test_address.c +++ b/src/test/test_address.c @@ -24,6 +24,7 @@ #endif /* defined(HAVE_IFCONF_TO_SMARTLIST) */ #include "core/or/or.h" +#include "app/config/config.h" #include "feature/dirauth/process_descs.h" #include "feature/nodelist/routerinfo_st.h" #include "feature/nodelist/node_st.h" @@ -1245,6 +1246,14 @@ test_address_tor_node_in_same_network_family(void *ignored) helper_free_mock_node(node_b); } +static or_options_t mock_options; + +static const or_options_t * +mock_get_options(void) +{ + return &mock_options; +} + /* Test dirserv_router_has_valid_address() on a stub routerinfo, with only its * address fields set. Use IPv4 ipv4_addr_str and IPv6 ipv6_addr_str. * Fail if it does not return rv. */ @@ -1270,22 +1279,32 @@ test_address_tor_node_in_same_network_family(void *ignored) TEST_ROUTER_VALID_ADDRESS_HELPER("1.0.0.1", ipv6_addr_str, rv) static void -test_address_dirserv_router_addr_private(void *ignored) +test_address_dirserv_router_addr_private(void *opt_dir_allow_private) { - (void)ignored; /* A stub routerinfo structure, with only its address fields set. */ routerinfo_t *ri = NULL; + /* The expected return value for private addresses. + * Modified if DirAllowPrivateAddresses is 1. */ + int private_rv = -1; + + memset(&mock_options, 0, sizeof(or_options_t)); + MOCK(get_options, mock_get_options); + + if (opt_dir_allow_private) { + mock_options.DirAllowPrivateAddresses = 1; + private_rv = 0; + } CHECK_RI_ADDR("1.0.0.1", 0); - CHECK_RI_ADDR("10.0.0.1", -1); + CHECK_RI_ADDR("10.0.0.1", private_rv); CHECK_RI_ADDR6("2600::1", 0); - CHECK_RI_ADDR6("fe80::1", -1); + CHECK_RI_ADDR6("fe80::1", private_rv); /* Null addresses */ /* IPv4 null fails, regardless of IPv6 */ - CHECK_RI_ADDR("0.0.0.0", -1); - TEST_ROUTER_VALID_ADDRESS_HELPER("0.0.0.0", "::", -1); + CHECK_RI_ADDR("0.0.0.0", private_rv); + TEST_ROUTER_VALID_ADDRESS_HELPER("0.0.0.0", "::", private_rv); /* IPv6 null succeeds, because IPv4 is not null */ CHECK_RI_ADDR6("::", 0); @@ -1294,7 +1313,7 @@ test_address_dirserv_router_addr_private(void *ignored) /* IPv4 null fails, regardless of IPv6 */ { ri = tor_malloc_zero(sizeof(routerinfo_t)); - tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, -1); + tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, private_rv); tor_free(ri); } @@ -1308,10 +1327,14 @@ test_address_dirserv_router_addr_private(void *ignored) done: tor_free(ri); + UNMOCK(get_options); } #define ADDRESS_TEST(name, flags) \ { #name, test_address_ ## name, flags, NULL, NULL } +#define ADDRESS_TEST_STR_ARG(name, flags, str_arg) \ + { #name "/" str_arg, test_address_ ## name, flags, &passthrough_setup, \ + (void *)(str_arg) } struct testcase_t address_tests[] = { ADDRESS_TEST(udp_socket_trick_whitebox, TT_FORK), @@ -1344,5 +1367,6 @@ struct testcase_t address_tests[] = { ADDRESS_TEST(tor_addr_in_same_network_family, 0), ADDRESS_TEST(tor_node_in_same_network_family, 0), ADDRESS_TEST(dirserv_router_addr_private, 0), + ADDRESS_TEST_STR_ARG(dirserv_router_addr_private, 0, "allow_private"), END_OF_TESTCASES };