Ignore accept6/reject6 IPv4, warn about unexpected rule outcomes

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.

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.

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);

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)))

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);
 }
 

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);

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);

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);
 

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);