diff --git a/changes/bug16069-exit-policy-rule6 b/changes/bug16069-exit-policy-rule6 new file mode 100644 index 0000000000..5b0560a2cf --- /dev/null +++ b/changes/bug16069-exit-policy-rule6 @@ -0,0 +1,16 @@ + o Minor bug fixes (torrc exit policies): + - When parsing torrc ExitPolicies, we now warn if: + * an IPv4 address is used on an accept6 or reject6 line. The line is + ignored, but the rest of the policy items in the list are used. + (accept/reject continue to allow both IPv4 and IPv6 addresses in + torrcs.) + * a "private" address alias is used on an accept6 or reject6 line. + The line filters both IPv4 and IPv6 private addresses, disregarding + the 6 in accept6/reject6. + - When parsing torrc ExitPolicies, we now issue an info-level message: + * when expanding an accept/reject * line to include both IPv4 and IPv6 + wildcard addresses. + - In each instance, usage advice is provided to avoid the message. + Partial fix for ticket 16069. Patch by "teor". + Patch on 2eb7eafc9d78 and a96c0affcb4c (25 Oct 2012), + released in 0.2.4.7-alpha. diff --git a/src/common/address.c b/src/common/address.c index dd336257ef..597f6990d3 100644 --- a/src/common/address.c +++ b/src/common/address.c @@ -690,6 +690,10 @@ tor_addr_parse_mask_ports(const char *s, if (flags & TAPMP_EXTENDED_STAR) { family = AF_UNSPEC; tor_addr_make_unspec(addr_out); + log_info(LD_GENERAL, + "'%s' expands into rules which apply to all IPv4 and IPv6 " + "addresses. (Use accept/reject *4:* for IPv4 or " + "accept[6]/reject[6] *6:* for IPv6.)", s); } else { family = AF_INET; tor_addr_from_ipv4h(addr_out, 0); diff --git a/src/or/policies.c b/src/or/policies.c index 07a3e18597..e1d7da1f5e 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -167,6 +167,7 @@ parse_addr_policy(config_line_t *cfg, smartlist_t **dest, smartlist_t *result; smartlist_t *entries; addr_policy_t *item; + int malformed_list; int r = 0; if (!cfg) @@ -179,12 +180,22 @@ parse_addr_policy(config_line_t *cfg, smartlist_t **dest, SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); SMARTLIST_FOREACH_BEGIN(entries, const char *, ent) { log_debug(LD_CONFIG,"Adding new entry '%s'",ent); - item = router_parse_addr_policy_item_from_string(ent, assume_action); + malformed_list = 0; + item = router_parse_addr_policy_item_from_string(ent, assume_action, + &malformed_list); if (item) { smartlist_add(result, item); - } else { - log_warn(LD_CONFIG,"Malformed policy '%s'.", ent); + } else if (malformed_list) { + /* the error is so severe the entire list should be discarded */ + log_warn(LD_CONFIG, "Malformed policy '%s'. Discarding entire policy " + "list.", ent); r = -1; + } else { + /* the error is minor: don't add the item, but keep processing the + * rest of the policies in the list */ + log_debug(LD_CONFIG, "Ignored policy '%s' due to non-fatal error. " + "The remainder of the policy list will be used.", + ent); } } SMARTLIST_FOREACH_END(ent); SMARTLIST_FOREACH(entries, char *, ent, tor_free(ent)); @@ -568,6 +579,8 @@ cmp_single_addr_policy(addr_policy_t *a, addr_policy_t *b) return r; if ((r=((int)a->is_private - (int)b->is_private))) return r; + /* refcnt and is_canonical are irrelevant to equality, + * they are hash table implementation details */ if ((r=tor_addr_compare(&a->addr, &b->addr, CMP_EXACT))) return r; if ((r=((int)a->maskbits - (int)b->maskbits))) diff --git a/src/or/routerparse.c b/src/or/routerparse.c index d0b2cba19f..2f7e50e60a 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -3666,12 +3666,20 @@ networkstatus_parse_detached_signatures(const char *s, const char *eos) * assume_action is nonnegative, then insert its action (ADDR_POLICY_ACCEPT or * ADDR_POLICY_REJECT) for items that specify no action. * + * Returns NULL on policy errors. + * + * If there is a policy error, malformed_list is set to true if the entire + * policy list should be discarded. Otherwise, it is set to false, and only + * this item should be ignored - the rest of the policy list can continue to + * be processed and used. + * * The addr_policy_t returned by this function can have its address set to * AF_UNSPEC for '*'. Use policy_expand_unspec() to turn this into a pair * of AF_INET and AF_INET6 items. */ MOCK_IMPL(addr_policy_t *, -router_parse_addr_policy_item_from_string,(const char *s, int assume_action)) +router_parse_addr_policy_item_from_string,(const char *s, int assume_action, + int *malformed_list)) { directory_token_t *tok = NULL; const char *cp, *eos; @@ -3688,6 +3696,8 @@ router_parse_addr_policy_item_from_string,(const char *s, int assume_action)) addr_policy_t *r; memarea_t *area = NULL; + tor_assert(malformed_list); + s = eat_whitespace(s); if ((*s == '*' || TOR_ISDIGIT(*s)) && assume_action >= 0) { if (tor_snprintf(line, sizeof(line), "%s %s", @@ -3714,9 +3724,32 @@ router_parse_addr_policy_item_from_string,(const char *s, int assume_action)) goto err; } + /* Use the extended interpretation of accept/reject *, + * expanding it into an IPv4 wildcard and an IPv6 wildcard. + * Also permit *4 and *6 for IPv4 and IPv6 only wildcards. */ r = router_parse_addr_policy(tok, TAPMP_EXTENDED_STAR); + if (!r) { + goto err; + } + + /* Ensure that accept6/reject6 fields are followed by IPv6 addresses. + * AF_UNSPEC addresses are only permitted on the accept/reject field type. + * Unlike descriptors, torrcs exit policy accept/reject can be followed by + * either an IPv4 or IPv6 address. */ + if ((tok->tp == K_ACCEPT6 || tok->tp == K_REJECT6) && + tor_addr_family(&r->addr) != AF_INET6) { + /* This is a non-fatal error, just ignore this one entry. */ + *malformed_list = 0; + log_warn(LD_DIR, "IPv4 address '%s' with accept6/reject6 field type in " + "exit policy. Ignoring, but continuing to parse rules. (Use " + "accept/reject with IPv4 addresses.)", + tok->n_args == 1 ? tok->args[0] : ""); + return NULL; + } + goto done; err: + *malformed_list = 1; r = NULL; done: token_clear(tok); @@ -3733,19 +3766,26 @@ static int router_add_exit_policy(routerinfo_t *router, directory_token_t *tok) { addr_policy_t *newe; + /* Use the standard interpretation of accept/reject *, an IPv4 wildcard. */ newe = router_parse_addr_policy(tok, 0); if (!newe) return -1; if (! router->exit_policy) router->exit_policy = smartlist_new(); + /* Ensure that in descriptors, accept/reject fields are followed by + * IPv4 addresses, and accept6/reject6 fields are followed by + * IPv6 addresses. Unlike torrcs, descriptor exit policies do not permit + * accept/reject followed by IPv6. */ if (((tok->tp == K_ACCEPT6 || tok->tp == K_REJECT6) && tor_addr_family(&newe->addr) == AF_INET) || ((tok->tp == K_ACCEPT || tok->tp == K_REJECT) && tor_addr_family(&newe->addr) == AF_INET6)) { + /* There's nothing the user can do about other relays' descriptors, + * so we don't provide usage advice here. */ log_warn(LD_DIR, "Mismatch between field type and address type in exit " - "policy"); + "policy '%s'. Ignoring.", tok->n_args == 1 ? tok->args[0] : ""); addr_policy_free(newe); return -1; } @@ -3821,6 +3861,13 @@ router_parse_addr_policy_private(directory_token_t *tok) result.prt_min = port_min; result.prt_max = port_max; + if (tok->tp == K_ACCEPT6 || tok->tp == K_REJECT6) { + log_warn(LD_GENERAL, + "'%s' expands into rules which apply to all private IPv4 and " + "IPv6 addresses. (Use accept/reject private:* for IPv4 and " + "IPv6.)", tok->n_args == 1 ? tok->args[0] : ""); + } + return addr_policy_get_canonical_entry(&result); } diff --git a/src/or/routerparse.h b/src/or/routerparse.h index 85e4b7d88e..99fd52866c 100644 --- a/src/or/routerparse.h +++ b/src/or/routerparse.h @@ -41,7 +41,7 @@ extrainfo_t *extrainfo_parse_entry_from_string(const char *s, const char *end, int cache_copy, struct digest_ri_map_t *routermap, int *can_dl_again_out); MOCK_DECL(addr_policy_t *, router_parse_addr_policy_item_from_string, - (const char *s, int assume_action)); + (const char *s, int assume_action, int *malformed_list)); version_status_t tor_version_is_obsolete(const char *myversion, const char *versionlist); int tor_version_as_new_as(const char *platform, const char *cutoff); diff --git a/src/or/routerset.c b/src/or/routerset.c index 9fe5dffdeb..3be55d3404 100644 --- a/src/or/routerset.c +++ b/src/or/routerset.c @@ -85,10 +85,13 @@ routerset_parse(routerset_t *target, const char *s, const char *description) int added_countries = 0; char *countryname; smartlist_t *list = smartlist_new(); + int malformed_list; smartlist_split_string(list, s, ",", SPLIT_SKIP_SPACE | SPLIT_IGNORE_BLANK, 0); SMARTLIST_FOREACH_BEGIN(list, char *, nick) { addr_policy_t *p; + /* if it doesn't pass our validation, assume it's malformed */ + malformed_list = 1; if (is_legal_hexdigest(nick)) { char d[DIGEST_LEN]; if (*nick == '$') @@ -106,15 +109,21 @@ routerset_parse(routerset_t *target, const char *s, const char *description) added_countries = 1; } else if ((strchr(nick,'.') || strchr(nick, '*')) && (p = router_parse_addr_policy_item_from_string( - nick, ADDR_POLICY_REJECT))) { + nick, ADDR_POLICY_REJECT, + &malformed_list))) { log_debug(LD_CONFIG, "Adding address %s to %s", nick, description); smartlist_add(target->policies, p); - } else { - log_warn(LD_CONFIG, "Entry '%s' in %s is malformed.", nick, - description); + } else if (malformed_list) { + log_warn(LD_CONFIG, "Entry '%s' in %s is malformed. Discarding entire" + " list.", nick, description); r = -1; tor_free(nick); SMARTLIST_DEL_CURRENT(list, nick); + } else { + log_notice(LD_CONFIG, "Entry '%s' in %s is ignored. Using the" + " remainder of the list.", nick, description); + tor_free(nick); + SMARTLIST_DEL_CURRENT(list, nick); } } SMARTLIST_FOREACH_END(nick); policy_expand_unspec(&target->policies); diff --git a/src/test/test_policy.c b/src/test/test_policy.c index 33f90c7da5..5e91468e8c 100644 --- a/src/test/test_policy.c +++ b/src/test/test_policy.c @@ -84,11 +84,13 @@ test_policies_general(void *arg) smartlist_t *sm = NULL; char *policy_str = NULL; short_policy_t *short_parsed = NULL; + int malformed_list = -1; (void)arg; policy = smartlist_new(); - p = router_parse_addr_policy_item_from_string("reject 192.168.0.0/16:*",-1); + p = router_parse_addr_policy_item_from_string("reject 192.168.0.0/16:*", -1, + &malformed_list); tt_assert(p != NULL); tt_int_op(ADDR_POLICY_REJECT,OP_EQ, p->policy_type); tor_addr_from_ipv4h(&tar, 0xc0a80000u); @@ -117,60 +119,76 @@ test_policies_general(void *arg) tt_assert(policy2); policy3 = smartlist_new(); - p = router_parse_addr_policy_item_from_string("reject *:*",-1); + p = router_parse_addr_policy_item_from_string("reject *:*", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy3, p); - p = router_parse_addr_policy_item_from_string("accept *:*",-1); + p = router_parse_addr_policy_item_from_string("accept *:*", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy3, p); policy4 = smartlist_new(); - p = router_parse_addr_policy_item_from_string("accept *:443",-1); + p = router_parse_addr_policy_item_from_string("accept *:443", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy4, p); - p = router_parse_addr_policy_item_from_string("accept *:443",-1); + p = router_parse_addr_policy_item_from_string("accept *:443", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy4, p); policy5 = smartlist_new(); - p = router_parse_addr_policy_item_from_string("reject 0.0.0.0/8:*",-1); + p = router_parse_addr_policy_item_from_string("reject 0.0.0.0/8:*", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy5, p); - p = router_parse_addr_policy_item_from_string("reject 169.254.0.0/16:*",-1); + p = router_parse_addr_policy_item_from_string("reject 169.254.0.0/16:*", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy5, p); - p = router_parse_addr_policy_item_from_string("reject 127.0.0.0/8:*",-1); + p = router_parse_addr_policy_item_from_string("reject 127.0.0.0/8:*", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy5, p); - p = router_parse_addr_policy_item_from_string("reject 192.168.0.0/16:*",-1); + p = router_parse_addr_policy_item_from_string("reject 192.168.0.0/16:*", + -1, &malformed_list); tt_assert(p != NULL); smartlist_add(policy5, p); - p = router_parse_addr_policy_item_from_string("reject 10.0.0.0/8:*",-1); + p = router_parse_addr_policy_item_from_string("reject 10.0.0.0/8:*", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy5, p); - p = router_parse_addr_policy_item_from_string("reject 172.16.0.0/12:*",-1); + p = router_parse_addr_policy_item_from_string("reject 172.16.0.0/12:*", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy5, p); - p = router_parse_addr_policy_item_from_string("reject 80.190.250.90:*",-1); + p = router_parse_addr_policy_item_from_string("reject 80.190.250.90:*", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy5, p); - p = router_parse_addr_policy_item_from_string("reject *:1-65534",-1); + p = router_parse_addr_policy_item_from_string("reject *:1-65534", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy5, p); - p = router_parse_addr_policy_item_from_string("reject *:65535",-1); + p = router_parse_addr_policy_item_from_string("reject *:65535", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy5, p); - p = router_parse_addr_policy_item_from_string("accept *:1-65535",-1); + p = router_parse_addr_policy_item_from_string("accept *:1-65535", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy5, p); policy6 = smartlist_new(); - p = router_parse_addr_policy_item_from_string("accept 43.3.0.0/9:*",-1); + p = router_parse_addr_policy_item_from_string("accept 43.3.0.0/9:*", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy6, p); policy7 = smartlist_new(); - p = router_parse_addr_policy_item_from_string("accept 0.0.0.0/8:*",-1); + p = router_parse_addr_policy_item_from_string("accept 0.0.0.0/8:*", -1, + &malformed_list); tt_assert(p != NULL); smartlist_add(policy7, p); @@ -297,6 +315,68 @@ test_policies_general(void *arg) TT_BAD_SHORT_POLICY("accept 1-,3"); TT_BAD_SHORT_POLICY("accept 1-,3"); + /* Make sure that IPv4 addresses are ignored in accept6/reject6 lines. */ + p = router_parse_addr_policy_item_from_string("accept6 1.2.3.4:*", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(!malformed_list); + + p = router_parse_addr_policy_item_from_string("reject6 2.4.6.0/24:*", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(!malformed_list); + + p = router_parse_addr_policy_item_from_string("accept6 *4:*", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(!malformed_list); + + /* Make sure malformed policies are detected as such. */ + p = router_parse_addr_policy_item_from_string("bad_token *4:*", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(malformed_list); + + p = router_parse_addr_policy_item_from_string("accept6 **:*", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(malformed_list); + + p = router_parse_addr_policy_item_from_string("accept */15:*", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(malformed_list); + + p = router_parse_addr_policy_item_from_string("reject6 */:*", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(malformed_list); + + p = router_parse_addr_policy_item_from_string("accept 127.0.0.1/33:*", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(malformed_list); + + p = router_parse_addr_policy_item_from_string("accept6 [::1]/129:*", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(malformed_list); + + p = router_parse_addr_policy_item_from_string("reject 8.8.8.8/-1:*", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(malformed_list); + + p = router_parse_addr_policy_item_from_string("reject 8.8.4.4:10-5", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(malformed_list); + + p = router_parse_addr_policy_item_from_string("reject 1.2.3.4:-1", -1, + &malformed_list); + tt_assert(p == NULL); + tt_assert(malformed_list); + /* Test a too-long policy. */ { int i; @@ -360,6 +440,7 @@ test_dump_exit_policy_to_string(void *arg) { char *ep; addr_policy_t *policy_entry; + int malformed_list = -1; routerinfo_t *ri = tor_malloc_zero(sizeof(routerinfo_t)); @@ -376,7 +457,8 @@ test_dump_exit_policy_to_string(void *arg) ri->exit_policy = smartlist_new(); ri->policy_is_reject_star = 0; - policy_entry = router_parse_addr_policy_item_from_string("accept *:*",-1); + policy_entry = router_parse_addr_policy_item_from_string("accept *:*", -1, + &malformed_list); smartlist_add(ri->exit_policy,policy_entry); @@ -386,7 +468,8 @@ test_dump_exit_policy_to_string(void *arg) tor_free(ep); - policy_entry = router_parse_addr_policy_item_from_string("reject *:25",-1); + policy_entry = router_parse_addr_policy_item_from_string("reject *:25", -1, + &malformed_list); smartlist_add(ri->exit_policy,policy_entry); @@ -397,7 +480,8 @@ test_dump_exit_policy_to_string(void *arg) tor_free(ep); policy_entry = - router_parse_addr_policy_item_from_string("reject 8.8.8.8:*",-1); + router_parse_addr_policy_item_from_string("reject 8.8.8.8:*", -1, + &malformed_list); smartlist_add(ri->exit_policy,policy_entry); @@ -407,7 +491,8 @@ test_dump_exit_policy_to_string(void *arg) tor_free(ep); policy_entry = - router_parse_addr_policy_item_from_string("reject6 [FC00::]/7:*",-1); + router_parse_addr_policy_item_from_string("reject6 [FC00::]/7:*", -1, + &malformed_list); smartlist_add(ri->exit_policy,policy_entry); @@ -418,7 +503,8 @@ test_dump_exit_policy_to_string(void *arg) tor_free(ep); policy_entry = - router_parse_addr_policy_item_from_string("accept6 [c000::]/3:*",-1); + router_parse_addr_policy_item_from_string("accept6 [c000::]/3:*", -1, + &malformed_list); smartlist_add(ri->exit_policy,policy_entry); diff --git a/src/test/test_routerset.c b/src/test/test_routerset.c index 9bd0c125c3..90dfb28c6b 100644 --- a/src/test/test_routerset.c +++ b/src/test/test_routerset.c @@ -430,7 +430,7 @@ NS(test_main)(void *arg) */ NS_DECL(addr_policy_t *, router_parse_addr_policy_item_from_string, - (const char *s, int assume_action)); + (const char *s, int assume_action, int *malformed_list)); addr_policy_t *NS(mock_addr_policy); @@ -457,10 +457,13 @@ NS(test_main)(void *arg) } addr_policy_t * -NS(router_parse_addr_policy_item_from_string)(const char *s, int assume_action) +NS(router_parse_addr_policy_item_from_string)(const char *s, + int assume_action, + int *malformed_list) { (void)s; (void)assume_action; + (void)malformed_list; CALLED(router_parse_addr_policy_item_from_string)++; return NS(mock_addr_policy);