From 4a240552c4ea837e174f37e04a19fb9f01e03d6d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 19 Jul 2007 19:40:45 +0000 Subject: 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 | 5 +++ src/common/util.c | 94 ++++++++++++++++++++++++++---------------------- src/common/util.h | 3 +- src/or/config.c | 4 +-- src/or/connection_edge.c | 29 ++++++--------- src/or/or.h | 7 ++-- src/or/policies.c | 48 +++++++++++-------------- src/or/routerparse.c | 8 ++--- src/or/test.c | 6 ++-- 9 files changed, 102 insertions(+), 102 deletions(-) diff --git a/ChangeLog b/ChangeLog index 96728c490..e6e2a5336 100644 --- a/ChangeLog +++ b/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 diff --git a/src/common/util.c b/src/common/util.c index 230acabe5..099d87bd2 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2000,6 +2000,32 @@ addr_mask_get_bits(uint32_t mask) return -1; } +/** Compare two addresses a1 and a2 for equality under a + * etmask of mbits 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 s 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_b = ip4b & (0xfffffffful << (32-mbits)); + masked_a = ip4a >> (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) - lmbits = 32; - else - lmbits = mbits; - - masked_a = ntohl(a1[idx]) >> (32-lmbits); - masked_b = ntohl(a2[idx]) >> (32-lmbits); + } else if (mbits < 32) { + masked_a >>= (32-mbits); + masked_b >>= (32-mbits); + } 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; } diff --git a/src/common/util.h b/src/common/util.h index 0165661fb..3cd19c06c 100644 --- a/src/common/util.h +++ b/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); diff --git a/src/or/config.c b/src/or/config.c index bebc8caca..4663e5b3d 100644 --- a/src/or/config.c +++ b/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, - &r->port_min,&r->port_max)) { + if (parse_addr_and_port_range(smartlist_get(elements,0),&r->addr, + &r->maskbits,&r->port_min,&r->port_max)) { *msg = tor_strdup("Error parsing source address in RedirectExit line"); goto err; } diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 49151b99a..95f2536ef 100644 --- a/src/or/connection_edge.c +++ b/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 int virtual_addr_netmask_bits = 10; +static maskbits_t 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_netmask = mask; + virtual_addr_network = addr & (0xfffffffful << (32-bits)); 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) { diff --git a/src/or/or.h b/src/or/or.h index 664e31464..8df54314e 100644 --- a/src/or/or.h +++ b/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 a such that - * a & msk == addr & msk . */ + maskbits_t maskbits; /**< Accept/reject all addresses a such that the + * first maskbits bits of a match + * addr. */ 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; diff --git a/src/or/policies.c b/src/or/policies.c index 5c1004025..62b98a476 100644 --- a/src/or/policies.c +++ b/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) { - /* There's a wildcard bit in b->msk that's not a wildcard in a. */ + if (a->maskbits > b->maskbits) { + /* 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) { - int n_bits = addr_mask_get_bits(policy->msk); - if (n_bits >= 0) { - 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; - } + if (policy->maskbits < 32 && policy->maskbits > 0) { + if (tor_snprintf(buf+written, buflen-written, "/%d", policy->maskbits)<0) + return -1; written += strlen(buf+written); } if (policy->prt_min <= 1 && policy->prt_max == 65535) { diff --git a/src/or/routerparse.c b/src/or/routerparse.c index ecbc5fb46..3967f7fd9 100644 --- a/src/or/routerparse.c +++ b/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)); diff --git a/src/or/test.c b/src/or/test.c index 85726e09f..7ba546ff1 100644 --- a/src/or/test.c +++ b/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); -- cgit v1.2.3