r13834@catbus: nickm | 2007-07-19 15:40:42 -0400

 Another patch from croup: drop support for address masks that do not correspond to bit prefixes.  Nobody has used this for a while, and we have given warnings for a long time.


svn:r10881

ChangeLog

@@ -1,4 +1,9 @@
 Changes in version 0.2.0.3-alpha - 2007-??-??
+  o Removed features:
+    - Stop allowing address masks that do not correspond to bit prefixes.
+      We have warned about these for a really long time; now it's time
+      to reject them. (Patch from croup.)
+
   o Minor features:
     - Create listener connections before we setuid to the configured User and
       Group.  This way, you can choose port values under 1024, start Tor as

src/common/util.c

@@ -2000,6 +2000,32 @@ addr_mask_get_bits(uint32_t mask)
   return -1;
 }
 
+/** Compare two addresses <b>a1</b> and <b>a2</b> for equality under a
+ *  etmask of <b>mbits</b> bits.  Return -1, 0, or 1.
+ *
+ * XXXX020Temporary function to allow masks as bitcounts everywhere.  This
+ * will be replaced with an IPv6-aware version as soon as 32-bit addresses are
+ * no longer passed around.
+ */
+int
+addr_mask_cmp_bits(uint32_t a1, uint32_t a2, maskbits_t bits)
+{
+  if (bits > 32)
+    bits = 32;
+  else if (bits == 0)
+    return 0;
+
+  a1 >>= (32-bits);
+  a2 >>= (32-bits);
+
+  if (a1 < a2)
+    return -1;
+  else if (a1 > a2)
+    return 1;
+  else
+    return 0;
+}
+
 /** Parse a string <b>s</b> in the format of (*|port(-maxport)?)?, setting the
  * various *out pointers as appropriate.  Return 0 on success, -1 on failure.
  */
@@ -2058,7 +2084,7 @@ parse_port_range(const char *port, uint16_t *port_min_out,
  */
 int
 parse_addr_and_port_range(const char *s, uint32_t *addr_out,
-                          uint32_t *mask_out, uint16_t *port_min_out,
+                          maskbits_t *maskbits_out, uint16_t *port_min_out,
                           uint16_t *port_max_out)
 {
   char *address;
@@ -2068,7 +2094,7 @@ parse_addr_and_port_range(const char *s, uint32_t *addr_out,
 
   tor_assert(s);
   tor_assert(addr_out);
-  tor_assert(mask_out);
+  tor_assert(maskbits_out);
   tor_assert(port_min_out);
   tor_assert(port_max_out);
 
@@ -2098,9 +2124,9 @@ parse_addr_and_port_range(const char *s, uint32_t *addr_out,
 
   if (!mask) {
     if (strcmp(address,"*")==0)
-      *mask_out = 0;
+      *maskbits_out = 0;
     else
-      *mask_out = 0xFFFFFFFFu;
+      *maskbits_out = 32;
   } else {
     endptr = NULL;
     bits = (int) strtol(mask, &endptr, 10);
@@ -2111,9 +2137,16 @@ parse_addr_and_port_range(const char *s, uint32_t *addr_out,
                  "Bad number of mask bits on address range; rejecting.");
         goto err;
       }
-      *mask_out = ~((1u<<(32-bits))-1);
+      *maskbits_out = bits;
     } else if (tor_inet_aton(mask, &in) != 0) {
-      *mask_out = ntohl(in.s_addr);
+      bits = addr_mask_get_bits(ntohl(in.s_addr));
+      if (bits < 0) {
+        log_warn(LD_GENERAL,
+                 "Mask %s on address range isn't a prefix; dropping",
+                 escaped(mask));
+        goto err;
+      }
+      *maskbits_out = bits;
     } else {
       log_warn(LD_GENERAL,
                "Malformed mask %s on address range; rejecting.",
@@ -2351,7 +2384,7 @@ tor_addr_is_null(const tor_addr_t *addr)
 {
   tor_assert(addr);
 
-  switch(IN_FAMILY(addr)) {
+  switch (IN_FAMILY(addr)) {
     case AF_INET6:
       if (!IN6_ADDR(addr)->s6_addr32[0] && !IN6_ADDR(addr)->s6_addr32[1] &&
           !IN6_ADDR(addr)->s6_addr32[2] && !IN6_ADDR(addr)->s6_addr32[3])
@@ -2457,38 +2490,32 @@ tor_addr_compare_masked(const tor_addr_t *addr1, const tor_addr_t *addr2,
     return 0;
 
   if (v_family[0] == AF_INET) { /* Real or mapped IPv4 */
-#if 0
     if (mbits >= 32) {
       masked_a = ip4a;
       masked_b = ip4b;
+    } else if (mbits == 0) {
+      return 0;
     } else {
-      masked_a = ip4a & (0xfffffffful << (32-mbits));
+      masked_a = ip4a >> (32-mbits);
-      masked_b = ip4b & (0xfffffffful << (32-mbits));
+      masked_b = ip4b >> (32-mbits);
     }
-#endif
-    if (mbits > 32)
-      mbits = 32;
-    masked_a = ip4a >> (32-mbits);
-    masked_b = ip4b >> (32-mbits);
     if (masked_a < masked_b)
       return -1;
     else if (masked_a > masked_b)
       return 1;
     return 0;
   } else if (v_family[0] == AF_INET6) { /* Real IPv6 */
-    maskbits_t lmbits;
     const uint32_t *a1 = IN6_ADDR(addr1)->s6_addr32;
     const uint32_t *a2 = IN6_ADDR(addr2)->s6_addr32;
     for (idx = 0; idx < 4; ++idx) {
-      if (!mbits)
+      uint32_t masked_a = ntohl(a1[idx]);
+      uint32_t masked_b = ntohl(a2[idx]);
+      if (!mbits) {
         return 0; /* Mask covers both addresses from here on */
-      else if (mbits > 32)
+      } else if (mbits < 32) {
-        lmbits = 32;
+        masked_a >>= (32-mbits);
-      else
+        masked_b >>= (32-mbits);
-        lmbits = mbits;
+      }
-
-      masked_a = ntohl(a1[idx]) >> (32-lmbits);
-      masked_b = ntohl(a2[idx]) >> (32-lmbits);
 
       if (masked_a > masked_b)
         return 1;
@@ -2499,25 +2526,6 @@ tor_addr_compare_masked(const tor_addr_t *addr1, const tor_addr_t *addr2,
         return 0;
       mbits -= 32;
     }
-#if 0
-    for (idx = 0; idx < 4; ++idx) {
-      if (mbits <= 32*idx) /* Mask covers both addresses from here on */
-        return 0;
-      if (mbits >= 32*(idx+1)) { /* Mask doesn't affect these 32 bits */
-        lmbits = 32;
-      } else {
-        lmbits = mbits % 32;
-      }
-      masked_a = ntohl(IN6_ADDR(addr1).s6_addr32[idx]) &
-                 (0xfffffffful << (32-lmbits));
-      masked_b = ntohl(IN6_ADDR(addr2).s6_addr32[idx]) &
-                 (0xfffffffful << (32-lmbits));
-      if (masked_a > masked_b)
-        return 1;
-      if (masked_a < masked_b)
-        return -1;
-    }
-#endif
     return 0;
   }
 

src/common/util.h

@@ -251,9 +251,10 @@ int parse_addr_port(int severity, const char *addrport, char **address,
 int parse_port_range(const char *port, uint16_t *port_min_out,
                      uint16_t *port_max_out);
 int parse_addr_and_port_range(const char *s, uint32_t *addr_out,
-                              uint32_t *mask_out, uint16_t *port_min_out,
+                              maskbits_t *maskbits_out, uint16_t *port_min_out,
                               uint16_t *port_max_out);
 int addr_mask_get_bits(uint32_t mask);
+int addr_mask_cmp_bits(uint32_t a1, uint32_t a2, maskbits_t bits);
 int tor_inet_ntoa(const struct in_addr *in, char *buf, size_t buf_len);
 char *tor_dup_addr(uint32_t addr) ATTR_MALLOC;
 int get_interface_address(int severity, uint32_t *addr);

src/or/config.c

@@ -3562,8 +3562,8 @@ parse_redirect_line(smartlist_t *result, config_line_t *line, char **msg)
     *msg = tor_strdup("Wrong number of elements in RedirectExit line");
     goto err;
   }
-  if (parse_addr_and_port_range(smartlist_get(elements,0),&r->addr,&r->mask,
+  if (parse_addr_and_port_range(smartlist_get(elements,0),&r->addr,
-                                &r->port_min,&r->port_max)) {
+                                &r->maskbits,&r->port_min,&r->port_max)) {
     *msg = tor_strdup("Error parsing source address in RedirectExit line");
     goto err;
   }

src/or/connection_edge.c

@@ -917,8 +917,7 @@ client_dns_set_reverse_addressmap(const char *address, const char *v,
  * These options are configured by parse_virtual_addr_network().
  */
 static uint32_t virtual_addr_network = 0x7fc00000u;
-static uint32_t virtual_addr_netmask = 0xffc00000u;
+static maskbits_t virtual_addr_netmask_bits = 10;
-static int virtual_addr_netmask_bits = 10;
 static uint32_t next_virtual_addr    = 0x7fc00000u;
 
 /** Read a netmask of the form 127.192.0.0/10 from "val", and check whether
@@ -930,11 +929,11 @@ int
 parse_virtual_addr_network(const char *val, int validate_only,
                            char **msg)
 {
-  uint32_t addr, mask;
+  uint32_t addr;
   uint16_t port_min, port_max;
-  int bits;
+  maskbits_t bits;
 
-  if (parse_addr_and_port_range(val, &addr, &mask, &port_min, &port_max)) {
+  if (parse_addr_and_port_range(val, &addr, &bits, &port_min, &port_max)) {
     if (msg) *msg = tor_strdup("Error parsing VirtualAddressNetwork");
     return -1;
   }
@@ -944,13 +943,6 @@ parse_virtual_addr_network(const char *val, int validate_only,
     return -1;
   }
 
-  bits = addr_mask_get_bits(mask);
-  if (bits < 0) {
-    if (msg) *msg = tor_strdup("VirtualAddressNetwork must have a mask that "
-                               "can be expressed as a prefix");
-    return -1;
-  }
-
   if (bits > 16) {
     if (msg) *msg = tor_strdup("VirtualAddressNetwork expects a /16 "
                                "network or larger");
@@ -960,11 +952,10 @@ parse_virtual_addr_network(const char *val, int validate_only,
   if (validate_only)
     return 0;
 
-  virtual_addr_network = addr & mask;
+  virtual_addr_network = addr & (0xfffffffful << (32-bits));
-  virtual_addr_netmask = mask;
   virtual_addr_netmask_bits = bits;
 
-  if ((next_virtual_addr & mask) != addr)
+  if (addr_mask_cmp_bits(next_virtual_addr, addr, bits))
     next_virtual_addr = addr;
 
   return 0;
@@ -983,7 +974,8 @@ address_is_in_virtual_range(const char *address)
     return 1;
   } else if (tor_inet_aton(address, &in)) {
     uint32_t addr = ntohl(in.s_addr);
-    if ((addr & virtual_addr_netmask) == virtual_addr_network)
+    if (!addr_mask_cmp_bits(addr, virtual_addr_network,
+                            virtual_addr_netmask_bits))
       return 1;
   }
   return 0;
@@ -1029,7 +1021,8 @@ addressmap_get_virtual_address(int type)
         log_warn(LD_CONFIG, "Ran out of virtual addresses!");
         return NULL;
       }
-      if ((next_virtual_addr & virtual_addr_netmask) != virtual_addr_network)
+      if (!addr_mask_cmp_bits(next_virtual_addr, virtual_addr_network,
+                              virtual_addr_netmask_bits))
         next_virtual_addr = virtual_addr_network;
     }
     return tor_strdup(buf);
@@ -2452,7 +2445,7 @@ connection_exit_connect(edge_connection_t *edge_conn)
   if (redirect_exit_list) {
     SMARTLIST_FOREACH(redirect_exit_list, exit_redirect_t *, r,
     {
-      if ((addr&r->mask)==(r->addr&r->mask) &&
+      if (!addr_mask_cmp_bits(addr, r->addr, r->maskbits) &&
           (r->port_min <= port) && (port <= r->port_max)) {
         struct in_addr in;
         if (r->is_redirect) {

src/or/or.h

@@ -1028,8 +1028,9 @@ typedef struct addr_policy_t {
 
   /* XXXX020 make this ipv6-capable */
   uint32_t addr; /**< Base address to accept or reject. */
-  uint32_t msk; /**< Accept/reject all addresses <b>a</b> such that
+  maskbits_t maskbits; /**< Accept/reject all addresses <b>a</b> such that the
-                 * a &amp; msk == <b>addr</b> &amp; msk . */
+                 * first <b>maskbits</b> bits of <b>a</b> match
+                 * <b>addr</b>. */
   uint16_t prt_min; /**< Lowest port number to accept/reject. */
   uint16_t prt_max; /**< Highest port number to accept/reject. */
 
@@ -1739,9 +1740,9 @@ typedef struct exit_redirect_t {
   /* XXXX020 make this whole mess ipv6-capable.  (Does anybody use it? */
 
   uint32_t addr;
-  uint32_t mask;
   uint16_t port_min;
   uint16_t port_max;
+  maskbits_t maskbits;
 
   uint32_t addr_dest;
   uint16_t port_dest;

src/or/policies.c

@@ -63,10 +63,6 @@ parse_addr_policy(config_line_t *cfg, addr_policy_t **dest,
       log_debug(LD_CONFIG,"Adding new entry '%s'",ent);
       *nextp = router_parse_addr_policy_from_string(ent, assume_action);
       if (*nextp) {
-        if (addr_mask_get_bits((*nextp)->msk)<0) {
-          log_warn(LD_CONFIG, "Address policy element '%s' can't be expressed "
-                   "as a bit prefix.", ent);
-        }
         /* Advance nextp to the end of the policy. */
         while (*nextp)
           nextp = &((*nextp)->next);
@@ -308,7 +304,7 @@ cmp_single_addr_policy(addr_policy_t *a, addr_policy_t *b)
     return r;
   if ((r=((int)a->addr - (int)b->addr)))
     return r;
-  if ((r=((int)a->msk - (int)b->msk)))
+  if ((r=((int)a->maskbits - (int)b->maskbits)))
     return r;
   if ((r=((int)a->prt_min - (int)b->prt_min)))
     return r;
@@ -371,7 +367,7 @@ compare_addr_to_addr_policy(uint32_t addr, uint16_t port,
       if ((port >= tmpe->prt_min && port <= tmpe->prt_max) ||
            (!port && tmpe->prt_min<=1 && tmpe->prt_max>=65535)) {
         /* The port definitely matches. */
-        if (tmpe->msk == 0) {
+        if (tmpe->maskbits == 0) {
           match = 1;
         } else {
           maybe = 1;
@@ -382,7 +378,7 @@ compare_addr_to_addr_policy(uint32_t addr, uint16_t port,
       }
     } else {
       /* Address is known */
-      if ((addr & tmpe->msk) == (tmpe->addr & tmpe->msk)) {
+      if (!addr_mask_cmp_bits(addr, tmpe->addr, tmpe->maskbits)) {
         if (port >= tmpe->prt_min && port <= tmpe->prt_max) {
           /* Exact match for the policy */
           match = 1;
@@ -420,11 +416,11 @@ addr_policy_covers(addr_policy_t *a, addr_policy_t *b)
 {
   /* We can ignore accept/reject, since "accept *:80, reject *:80" reduces
    * to "accept *:80". */
-  if (a->msk & ~b->msk) {
+  if (a->maskbits > b->maskbits) {
-    /* There's a wildcard bit in b->msk that's not a wildcard in a. */
+    /* a has more fixed bits than b; it can't possibly cover b. */
     return 0;
   }
-  if ((a->addr & a->msk) != (b->addr & a->msk)) {
+  if (addr_mask_cmp_bits(a->addr, b->addr, a->maskbits)) {
     /* There's a fixed bit in a that's set differently in b. */
     return 0;
   }
@@ -438,11 +434,16 @@ addr_policy_covers(addr_policy_t *a, addr_policy_t *b)
 static int
 addr_policy_intersects(addr_policy_t *a, addr_policy_t *b)
 {
+  maskbits_t minbits;
   /* All the bits we care about are those that are set in both
    * netmasks.  If they are equal in a and b's networkaddresses
    * then the networks intersect.  If there is a difference,
    * then they do not. */
-  if (((a->addr ^ b->addr) & a->msk & b->msk) != 0)
+  if (a->maskbits < b->maskbits)
+    minbits = a->maskbits;
+  else
+    minbits = b->maskbits;
+  if (addr_mask_cmp_bits(a->addr, b->addr, minbits))
     return 0;
   if (a->prt_max < b->prt_min || b->prt_max < a->prt_min)
     return 0;
@@ -470,7 +471,7 @@ exit_policy_remove_redundancies(addr_policy_t **dest)
 
   /* Step one: find a *:* entry and cut off everything after it. */
   for (ap=*dest; ap; ap=ap->next) {
-    if (ap->msk == 0 && ap->prt_min <= 1 && ap->prt_max >= 65535) {
+    if (ap->maskbits == 0 && ap->prt_min <= 1 && ap->prt_max >= 65535) {
       /* This is a catch-all line -- later lines are unreachable. */
       if (ap->next) {
         addr_policy_free(ap->next);
@@ -581,7 +582,7 @@ exit_policy_is_general_exit(addr_policy_t *policy)
     for ( ; p; p = p->next) {
       if (p->prt_min > ports[i] || p->prt_max < ports[i])
         continue; /* Doesn't cover our port. */
-      if ((p->msk & 0x00fffffful) != 0)
+      if (p->maskbits > 8)
         continue; /* Narrower than a /8. */
       if ((p->addr & 0xff000000ul) == 0x7f000000ul)
         continue; /* 127.x */
@@ -605,7 +606,7 @@ policy_is_reject_star(addr_policy_t *p)
       return 0;
     else if (p->policy_type == ADDR_POLICY_REJECT &&
              p->prt_min <= 1 && p->prt_max == 65535 &&
-             p->msk == 0)
+             p->maskbits == 0)
       return 1;
   }
   return 1;
@@ -626,24 +627,15 @@ policy_write_item(char *buf, size_t buflen, addr_policy_t *policy)
   /* write accept/reject 1.2.3.4 */
   result = tor_snprintf(buf, buflen, "%s %s",
             policy->policy_type == ADDR_POLICY_ACCEPT ? "accept" : "reject",
-            policy->msk == 0 ? "*" : addrbuf);
+            policy->maskbits == 0 ? "*" : addrbuf);
   if (result < 0)
     return -1;
   written += strlen(buf);
-  /* If the mask is 0xffffffff, we don't need to give it.  If the mask is 0,
+  /* If the maskbits is 32 we don't need to give it.  If the mask is 0,
    * we already wrote "*". */
-  if (policy->msk != 0xFFFFFFFFu && policy->msk != 0) {
+  if (policy->maskbits < 32 && policy->maskbits > 0) {
-    int n_bits = addr_mask_get_bits(policy->msk);
+    if (tor_snprintf(buf+written, buflen-written, "/%d", policy->maskbits)<0)
-    if (n_bits >= 0) {
+      return -1;
-      if (tor_snprintf(buf+written, buflen-written, "/%d", n_bits)<0)
-        return -1;
-    } else {
-      /* Write "/255.255.0.0" */
-      in.s_addr = htonl(policy->msk);
-      tor_inet_ntoa(&in, addrbuf, sizeof(addrbuf));
-      if (tor_snprintf(buf+written, buflen-written, "/%s", addrbuf)<0)
-        return -1;
-    }
     written += strlen(buf+written);
   }
   if (policy->prt_min <= 1 && policy->prt_max == 65535) {

src/or/routerparse.c

@@ -2092,7 +2092,7 @@ router_parse_addr_policy_from_string(const char *s, int assume_action)
 {
   directory_token_t *tok = NULL;
   const char *cp;
-  char *tmp;
+  char *tmp = NULL;
   addr_policy_t *r;
   size_t len, idx;
   const char *eos;
@@ -2175,7 +2175,7 @@ router_parse_addr_policy(directory_token_t *tok)
   newe->policy_type = (tok->tp == K_REJECT) ? ADDR_POLICY_REJECT
     : ADDR_POLICY_ACCEPT;
 
-  if (parse_addr_and_port_range(arg, &newe->addr, &newe->msk,
+  if (parse_addr_and_port_range(arg, &newe->addr, &newe->maskbits,
                                 &newe->prt_min, &newe->prt_max))
     goto policy_read_failed;
 
@@ -2229,7 +2229,7 @@ router_parse_addr_policy_private(directory_token_t *tok)
                  tok->tp == K_REJECT ? "reject" : "accept",
                  private_nets[net], arg);
     if (parse_addr_and_port_range((*nextp)->string + 7,
-                                  &(*nextp)->addr, &(*nextp)->msk,
+                                  &(*nextp)->addr, &(*nextp)->maskbits,
                                   &(*nextp)->prt_min, &(*nextp)->prt_max)) {
       log_warn(LD_BUG, "Couldn't parse an address range we generated!");
       return NULL;
@@ -2253,7 +2253,7 @@ assert_addr_policy_ok(addr_policy_t *t)
     tor_assert(t2);
     tor_assert(t2->policy_type == t->policy_type);
     tor_assert(t2->addr == t->addr);
-    tor_assert(t2->msk == t->msk);
+    tor_assert(t2->maskbits == t->maskbits);
     tor_assert(t2->prt_min == t->prt_min);
     tor_assert(t2->prt_max == t->prt_max);
     tor_assert(!strcmp(t2->string, t->string));

src/or/test.c

@@ -2095,12 +2095,12 @@ test_dir_format(void)
   ex1.policy_type = ADDR_POLICY_ACCEPT;
   ex1.string = NULL;
   ex1.addr = 0;
-  ex1.msk = 0;
+  ex1.maskbits = 0;
   ex1.prt_min = ex1.prt_max = 80;
   ex1.next = &ex2;
   ex2.policy_type = ADDR_POLICY_REJECT;
   ex2.addr = 18 << 24;
-  ex2.msk = 0xff000000u;
+  ex2.maskbits = 8;
   ex2.prt_min = ex2.prt_max = 24;
   ex2.next = NULL;
   r2.address = tor_strdup("1.1.1.1");
@@ -2719,7 +2719,7 @@ test_policies(void)
   test_eq(ADDR_POLICY_REJECT, policy->policy_type);
   tor_addr_from_ipv4(&tar, 0xc0a80000u);
   test_assert(policy->addr == 0xc0a80000u);
-  test_eq(0xffff0000u, policy->msk);
+  test_eq(16, policy->maskbits);
   test_eq(1, policy->prt_min);
   test_eq(65535, policy->prt_max);
   test_streq("reject 192.168.0.0/16:*", policy->string);