From d9a7d4779887dbd2cba082c2a5daa535fe0d36ce Mon Sep 17 00:00:00 2001 From: Neel Chauhan Date: Fri, 19 Jul 2019 12:56:02 -0400 Subject: [PATCH 1/5] Check for private IPv6 addresses in dirserv_router_has_valid_address() --- src/feature/dirauth/process_descs.c | 16 +++++++++++++--- src/feature/dirauth/process_descs.h | 4 ++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/feature/dirauth/process_descs.c b/src/feature/dirauth/process_descs.c index a68d155651..e1a02179b0 100644 --- a/src/feature/dirauth/process_descs.c +++ b/src/feature/dirauth/process_descs.c @@ -428,7 +428,7 @@ dirserv_free_fingerprint_list(void) /** Return -1 if ri has a private or otherwise bad address, * unless we're configured to not care. Return 0 if all ok. */ -static int +STATIC int dirserv_router_has_valid_address(routerinfo_t *ri) { tor_addr_t addr; @@ -436,12 +436,22 @@ dirserv_router_has_valid_address(routerinfo_t *ri) return 0; /* whatever it is, we're fine with it */ tor_addr_from_ipv4h(&addr, ri->addr); - if (tor_addr_is_internal(&addr, 0)) { + if (tor_addr_is_internal(&addr, 0) || tor_addr_is_null(&addr)) { log_info(LD_DIRSERV, - "Router %s published internal IP address. Refusing.", + "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)) { + log_info(LD_DIRSERV, + "Router %s published internal IPv6 address. Refusing.", + router_describe(ri)); + return -1; /* it's a private IP, we should reject it */ + } + return 0; } diff --git a/src/feature/dirauth/process_descs.h b/src/feature/dirauth/process_descs.h index 001c866eba..1d4085b091 100644 --- a/src/feature/dirauth/process_descs.h +++ b/src/feature/dirauth/process_descs.h @@ -36,4 +36,8 @@ void dirserv_set_node_flags_from_authoritative_status(node_t *node, int dirserv_would_reject_router(const routerstatus_t *rs); +#ifdef TOR_UNIT_TESTS +STATIC int dirserv_router_has_valid_address(routerinfo_t *ri); +#endif /* defined(TOR_UNIT_TESTS) */ + #endif /* !defined(TOR_RECV_UPLOADS_H) */ From cc5af6dbd5d27cc80fba9df65eea1fff5833a08d Mon Sep 17 00:00:00 2001 From: Neel Chauhan Date: Fri, 19 Jul 2019 13:11:45 -0400 Subject: [PATCH 2/5] Add changes file for Bug #31088 --- changes/bug31088 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/bug31088 diff --git a/changes/bug31088 b/changes/bug31088 new file mode 100644 index 0000000000..c258d1bada --- /dev/null +++ b/changes/bug31088 @@ -0,0 +1,5 @@ + o Minor bugfixes (ipv6): + - We check for private IPv6 address alongside their IPv4 equivalents when + authorities check descriptors. Previously, we only checked for private + IPv4 addresses. Fixes bug 31088; bugfix on 0.2.3.21-rc. Patch by Neel + Chauhan. From c8e69e63f9d39776bd926f1a07628c03c7582280 Mon Sep 17 00:00:00 2001 From: Neel Chauhan Date: Tue, 13 Aug 2019 20:36:05 -0400 Subject: [PATCH 3/5] Add test for dirserv_router_has_valid_address() --- 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 bf9ca047dc..eb0500be8f 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 "feature/dirauth/process_descs.h" #include "feature/nodelist/routerinfo_st.h" #include "feature/nodelist/node_st.h" #include "feature/nodelist/nodelist.h" @@ -1244,6 +1245,38 @@ 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 \ + routerinfo_t *ri = tor_malloc_zero(sizeof(routerinfo_t)); \ + tor_addr_t addr; \ + tor_addr_parse(&addr, 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_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 \ + routerinfo_t *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 + +static void +test_address_dirserv_router_addr_private(void *ignored) +{ + (void)ignored; + 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); + done: + ; +} + #define ADDRESS_TEST(name, flags) \ { #name, test_address_ ## name, flags, NULL, NULL } @@ -1277,5 +1310,6 @@ struct testcase_t address_tests[] = { ADDRESS_TEST(tor_addr_eq_ipv4h, 0), 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), END_OF_TESTCASES }; From 5eca338107a4d578e1cd0204e21fd7556c0aa926 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 26 Aug 2019 18:27:55 -0400 Subject: [PATCH 4/5] In tests, make sure that "ri" is freed on all paths. In Tor's tests, the tt_*() macros can call "goto done" on failure. When that happens, we need to make sure that all of our allocated memory still gets freed, or else Coverity will complain. --- src/test/test_address.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/test/test_address.c b/src/test/test_address.c index eb0500be8f..164cc3150d 100644 --- a/src/test/test_address.c +++ b/src/test/test_address.c @@ -1246,7 +1246,7 @@ test_address_tor_node_in_same_network_family(void *ignored) } #define CHECK_RI_ADDR(addr_str, rv) STMT_BEGIN \ - routerinfo_t *ri = tor_malloc_zero(sizeof(routerinfo_t)); \ + ri = tor_malloc_zero(sizeof(routerinfo_t)); \ tor_addr_t addr; \ tor_addr_parse(&addr, addr_str); \ ri->addr = tor_addr_to_ipv4h(&addr); \ @@ -1258,7 +1258,7 @@ test_address_tor_node_in_same_network_family(void *ignored) /* 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 \ - routerinfo_t *ri = tor_malloc_zero(sizeof(routerinfo_t)); \ + 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); \ @@ -1269,12 +1269,14 @@ static void 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); done: - ; + tor_free(ri); } #define ADDRESS_TEST(name, flags) \ From 8af92b6577595181eddc1dea25cf44d20bedb8a0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 26 Aug 2019 18:30:02 -0400 Subject: [PATCH 5/5] test_address: parenthesize macro arguments. It's good style to always add parentheses when using macro arguments, in case somebody someday provides an argument that contains an operator you don't expect, or causes the expression to parse differently. --- src/test/test_address.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/test_address.c b/src/test/test_address.c index 164cc3150d..ef6daa06b4 100644 --- a/src/test/test_address.c +++ b/src/test/test_address.c @@ -1248,10 +1248,10 @@ test_address_tor_node_in_same_network_family(void *ignored) #define CHECK_RI_ADDR(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, (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); \ + tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, (rv)); \ tor_free(ri); \ STMT_END @@ -1260,8 +1260,8 @@ test_address_tor_node_in_same_network_family(void *ignored) #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_addr_parse(&ri->ipv6_addr, (addr_str)); \ + tt_int_op(dirserv_router_has_valid_address(ri), OP_EQ, (rv)); \ tor_free(ri); \ STMT_END