Merge remote-tracking branch 'teor/feature17863'

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".

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;

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. */

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
@@ -3697,9 +3697,12 @@ 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) {
+  /* 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));

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