From e54e71fb6bb0f76bf73b540580abd0a5c5c8539d Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 16 Dec 2015 08:48:32 +1100 Subject: [PATCH 1/4] Limit IPv6 mask bits to 128 --- src/common/address.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/common/address.c b/src/common/address.c index 627764153b..69a80986ed 100644 --- a/src/common/address.c +++ b/src/common/address.c @@ -1039,6 +1039,8 @@ tor_addr_compare_masked(const tor_addr_t *addr1, const tor_addr_t *addr2, return r; } case AF_INET6: { + if (mbits > 128) + mbits = 128; const uint8_t *a1 = tor_addr_to_in6_addr8(addr1); const uint8_t *a2 = tor_addr_to_in6_addr8(addr2); const int bytes = mbits >> 3; From cd0a5db5e9ffbe873fba1a0f4965cdd008824247 Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 16 Dec 2015 08:50:13 +1100 Subject: [PATCH 2/4] Initialise malformed_list to 0 each time we parse a policy --- src/or/routerparse.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/or/routerparse.c b/src/or/routerparse.c index f6619cb902..8170bbb057 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -3697,6 +3697,7 @@ router_parse_addr_policy_item_from_string,(const char *s, int assume_action, memarea_t *area = NULL; tor_assert(malformed_list); + *malformed_list = 0; s = eat_whitespace(s); if ((*s == '*' || TOR_ISDIGIT(*s)) && assume_action >= 0) { From ce92335214f4490f0e14487a99415c26777be2a8 Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 16 Dec 2015 08:49:06 +1100 Subject: [PATCH 3/4] Add policy assume_action support for IPv6 addresses These IPv6 addresses must be quoted, because : is the port separator, and "acce" is a valid hex block. Add unit tests for assumed actions in IPv6 policies. --- src/or/routerparse.c | 8 ++-- src/test/test_policy.c | 87 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 8170bbb057..f5a003ea70 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -3684,8 +3684,8 @@ router_parse_addr_policy_item_from_string,(const char *s, int assume_action, directory_token_t *tok = NULL; const char *cp, *eos; /* Longest possible policy is - * "accept6 ffff:ffff:..255/128:10000-65535", - * which contains a max-length IPv6 address, plus 24 characters. + * "accept6 [ffff:ffff:..255]/128:10000-65535", + * which contains a max-length IPv6 address, plus 26 characters. * But note that there can be an arbitrary amount of space between the * accept and the address:mask/port element. * We don't need to multiply TOR_ADDR_BUF_LEN by 2, as there is only one @@ -3700,7 +3700,9 @@ router_parse_addr_policy_item_from_string,(const char *s, int assume_action, *malformed_list = 0; s = eat_whitespace(s); - if ((*s == '*' || TOR_ISDIGIT(*s)) && assume_action >= 0) { + /* We can only do assume_action on []-quoted IPv6, as "a" (accept) + * and ":" (port separator) are ambiguous */ + if ((*s == '*' || *s == '[' || TOR_ISDIGIT(*s)) && assume_action >= 0) { if (tor_snprintf(line, sizeof(line), "%s %s", assume_action == ADDR_POLICY_ACCEPT?"accept":"reject", s)<0) { log_warn(LD_DIR, "Policy %s is too long.", escaped(s)); diff --git a/src/test/test_policy.c b/src/test/test_policy.c index b7d89a1445..4f5565e575 100644 --- a/src/test/test_policy.c +++ b/src/test/test_policy.c @@ -270,6 +270,93 @@ test_policies_general(void *arg) addr_policy_list_free(policy); policy = NULL; + /* make sure assume_action works */ + malformed_list = 0; + p = router_parse_addr_policy_item_from_string("127.0.0.1", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(p); + addr_policy_free(p); + tt_assert(!malformed_list); + + p = router_parse_addr_policy_item_from_string("127.0.0.1:*", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(p); + addr_policy_free(p); + tt_assert(!malformed_list); + + p = router_parse_addr_policy_item_from_string("[::]", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(p); + addr_policy_free(p); + tt_assert(!malformed_list); + + p = router_parse_addr_policy_item_from_string("[::]:*", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(p); + addr_policy_free(p); + tt_assert(!malformed_list); + + p = router_parse_addr_policy_item_from_string("[face::b]", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(p); + addr_policy_free(p); + tt_assert(!malformed_list); + + p = router_parse_addr_policy_item_from_string("[b::aaaa]", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(p); + addr_policy_free(p); + tt_assert(!malformed_list); + + p = router_parse_addr_policy_item_from_string("*", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(p); + addr_policy_free(p); + tt_assert(!malformed_list); + + p = router_parse_addr_policy_item_from_string("*4", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(p); + addr_policy_free(p); + tt_assert(!malformed_list); + + p = router_parse_addr_policy_item_from_string("*6", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(p); + addr_policy_free(p); + tt_assert(!malformed_list); + + /* These are all ambiguous IPv6 addresses, it's good that we reject them */ + p = router_parse_addr_policy_item_from_string("acce::abcd", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(!p); + tt_assert(malformed_list); + malformed_list = 0; + + p = router_parse_addr_policy_item_from_string("7:1234", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(!p); + tt_assert(malformed_list); + malformed_list = 0; + + p = router_parse_addr_policy_item_from_string("::", + ADDR_POLICY_ACCEPT, + &malformed_list); + tt_assert(!p); + tt_assert(malformed_list); + malformed_list = 0; + /* make sure compacting logic works. */ policy = NULL; line.key = (char*)"foo"; From e2e09a2dbeae508ef8539e48eff1babb6aa0346e Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 16 Dec 2015 08:47:47 +1100 Subject: [PATCH 4/4] Warn when comparing against an AF_UNSPEC address in a policy It produces unexpected results, and it's most likely a bug. --- changes/feature17863 | 6 ++++++ src/or/policies.c | 12 ++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 changes/feature17863 diff --git a/changes/feature17863 b/changes/feature17863 new file mode 100644 index 0000000000..86c4e2ce0c --- /dev/null +++ b/changes/feature17863 @@ -0,0 +1,6 @@ + o Minor feature (IPv6): + - Add address policy assume_action support for IPv6 addresses. + - Limit IPv6 mask bits to 128. + - Warn when comparing against an AF_UNSPEC address in a policy, + it's almost always a bug. + Closes ticket 17863; patch by "teor". diff --git a/src/or/policies.c b/src/or/policies.c index 32a7ec2da4..c9bce1b234 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -696,6 +696,10 @@ compare_known_tor_addr_to_addr_policy(const tor_addr_t *addr, uint16_t port, /* We know the address and port, and we know the policy, so we can just * compute an exact match. */ SMARTLIST_FOREACH_BEGIN(policy, addr_policy_t *, tmpe) { + if (tmpe->addr.family == AF_UNSPEC) { + log_warn(LD_BUG, "Policy contains an AF_UNSPEC address, which only " + "matches other AF_UNSPEC addresses."); + } /* Address is known */ if (!tor_addr_compare_masked(addr, &tmpe->addr, tmpe->maskbits, CMP_EXACT)) { @@ -723,6 +727,10 @@ compare_known_tor_addr_to_addr_policy_noport(const tor_addr_t *addr, int maybe_accept = 0, maybe_reject = 0; SMARTLIST_FOREACH_BEGIN(policy, addr_policy_t *, tmpe) { + if (tmpe->addr.family == AF_UNSPEC) { + log_warn(LD_BUG, "Policy contains an AF_UNSPEC address, which only " + "matches other AF_UNSPEC addresses."); + } if (!tor_addr_compare_masked(addr, &tmpe->addr, tmpe->maskbits, CMP_EXACT)) { if (tmpe->prt_min <= 1 && tmpe->prt_max >= 65535) { @@ -762,6 +770,10 @@ compare_unknown_tor_addr_to_addr_policy(uint16_t port, int maybe_accept = 0, maybe_reject = 0; SMARTLIST_FOREACH_BEGIN(policy, addr_policy_t *, tmpe) { + if (tmpe->addr.family == AF_UNSPEC) { + log_warn(LD_BUG, "Policy contains an AF_UNSPEC address, which only " + "matches other AF_UNSPEC addresses."); + } if (tmpe->prt_min <= port && port <= tmpe->prt_max) { if (tmpe->maskbits == 0) { /* Definitely matches, since it covers all addresses. */