[PATCH] IPv6 ACLs

[Bob Ham]

Hi there,

The attached patch implements IPv6 support for access control.  It also
includes a small update to restrict zeroconf advertisements to IPv4 if
RFC2553 support isn't enabled.

I've tested the patch both with RFC2553 support enabled and without.
There seem to be no regressions.

A more extensive zeroconf update will follow to restrict the addresses
it advertises to ones that are actually listened on.  It seems there may
be issues listening on IPv6 addresses as well.

Regards,

Bob Ham

--
Bob Ham <rah@...>

for (;;) { ++pancakes; }

[distcc-v6-acl.patch]

---

Index: src/access.c
===================================================================
--- src/access.c (revision 676)
+++ src/access.c (working copy)
@@ -32,11 +32,13 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <limits.h>
 
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
+#include <netdb.h>
 
 #include "types.h"
 #include "distcc.h"
@@ -47,79 +49,317 @@
 /**
  * @file
  *
- * Simple IP-based access-control system
+ * IP-based access-control system
  */
 
-static const in_addr_t allones = 0xffffffffUL;
+static const uint32_t allones = 0xffffffffUL;
 
+/**
+ * Split a "HOST/BITS" mask specification into HOST and BITS.
+ **/
+static void split_mask(const char *spec,
+                       char **host,
+                       const char **bits)
+{
+    int value_len = strcspn(spec, "/");
+    
+    if (host != NULL) {
+        char *host_str;
+        
+        host_str = malloc(value_len + 1);
+        strncpy(host_str, spec, value_len);
+        host_str[value_len] = '\0';
+        
+        *host = host_str;
+    }
+    
+    if (bits != NULL) {
+        if (spec[value_len] && spec[value_len + 1])
+            *bits = spec + value_len + 1;
+        else
+            *bits = NULL;
+    }
+}
 
+
 /**
+ * Set a dcc_address_t to a full mask.
+ **/
+static void set_allones(dcc_address_t *addr)
+{
+#ifndef ENABLE_RFC2553
+    *addr = *(dcc_address_t *)&allones;
+#else
+    memset(&addr->addr, ~0, sizeof(struct in6_addr));
+#endif
+}
+
+
+/**
+ * Set a 32-bit address, @p addr, to a mask of @p mask_size bits.
+ **/
+static inline void set_mask_32(uint32_t *addr, int mask_bits) {
+    *addr = htonl(~(allones >> mask_bits));
+}
+
+
+#ifdef ENABLE_RFC2553
+/**
+ * Set a v6 address, @p addr, to a mask of @p mask_size bits.
+ **/
+static void set_mask_inet6(struct in6_addr *mask, int mask_bits) {
+    uint32_t *iptr = (uint32_t *)mask;
+    int allones_count = mask_bits / 32;
+    int i;
+    
+    for(i = 0; i < allones_count; ++i, ++iptr)
+        *iptr = allones;
+    
+    set_mask_32(iptr, mask_bits % 32);
+    ++iptr;
+    
+    for(i = 4 - allones_count - 1; i > 0; --i, ++iptr)
+        *iptr = 0;
+}
+#endif /* ENABLE_RFC2553 */
+
+
+/**
+ * Set a dcc_address_t to a mask of @p mask_bits bits.  The family field of
+ * the address should be set appropriately if RFC2553 is enabled.
+ **/
+static void set_mask(dcc_address_t *mask, int mask_bits)
+{
+#ifndef ENABLE_RFC2553
+    set_mask_32((uint32_t *)mask, mask_bits);
+#else
+    if (mask->family == AF_INET)
+        set_mask_32((uint32_t *)&mask->addr.inet, mask_bits);
+    else
+        set_mask_inet6(&mask->addr.inet6, mask_bits);
+#endif
+}
+
+/**
  * Interpret a "HOST/BITS" mask specification.  Return @p value and @p mask.
  **/
 int dcc_parse_mask(const char *spec,
-                   in_addr_t *value,
-                   in_addr_t *mask)
+                   dcc_address_t *value,
+                   dcc_address_t *mask)
 {
-    int value_len;
-    struct in_addr ia;
-    int mask_bits;
     char *value_str;
     int matched;
     const char *mask_str;
-
-    value_len = strcspn(spec, "/");
-
-    /* get bit before slash */
-    value_str = strdup(spec);
-    value_str[value_len] = '\0';
+#ifndef ENABLE_RFC2553
+    struct in_addr ia;
+#else
+    struct addrinfo hints;
+    struct addrinfo *res;
+#endif
+    
+    split_mask(spec, &value_str, &mask_str);
+    
+    /* extract and parse value part */
+#ifndef ENABLE_RFC2553
     matched = inet_pton(AF_INET, value_str, &ia);
-
-    /* extract and parse value part */
     if (!matched) {
         rs_log_error("can't parse internet address \"%s\"", value_str);
         free(value_str);
         return EXIT_BAD_ARGUMENTS;
     }
+    *value = ia;
+    
+#else /* ENABLE_RFC2553 */
+    
+    memset(&hints, 0, sizeof(hints));
+    hints.ai_flags = AI_NUMERICHOST;
+    hints.ai_family = AF_UNSPEC;
+    hints.ai_socktype = SOCK_STREAM;
+    
+    matched = getaddrinfo(value_str, NULL, &hints, &res);
+    if (matched != 0) {
+        rs_log_error("can't parse internet address \"%s\": %s",
+                     value_str, gai_strerror(matched));
+        free(value_str);
+        return EXIT_BAD_ARGUMENTS;
+    }
+    
+    value->family = res->ai_family;
+    if (res->ai_family == AF_INET) {
+        value->addr.inet = ((struct sockaddr_in *)res->ai_addr)->sin_addr;
+    } else if (res->ai_family == AF_INET6) {
+        value->addr.inet6 = ((struct sockaddr_in6 *)res->ai_addr)->sin6_addr;
+    } else {
+        rs_log_error("unsupported address family %d for address \"%s\"",
+                     res->ai_family, value_str);
+        freeaddrinfo(res);
+        free(value_str);
+        return EXIT_BAD_ARGUMENTS;
+    }
+    
+    freeaddrinfo(res);
+#endif /* !ENABLE_RFC2553 */
     free(value_str);
-    *value = ia.s_addr;
+    
+    
+#ifdef ENABLE_RFC2553
+    mask->family = value->family;
+#endif
+    if (mask_str != NULL) {
+        int mask_bits;
+#ifndef ENABLE_RFC2553
+        static const int mask_max_bits = 32;
+#else
+        static const int mask_max_bits = 128;
+#endif
 
-    mask_str = &spec[value_len + 1];
-    if (spec[value_len] && *mask_str) {
         /* find mask length as a number of bits */
         mask_bits = atoi(mask_str);
-        if (mask_bits < 0 || mask_bits > 32) {
+        if (mask_bits < 0 || mask_bits > mask_max_bits) {
             rs_log_error("invalid mask \"%s\"", mask_str);
             return EXIT_BAD_ARGUMENTS;
         }
 
         /* Make a network-endian mask with the top mask_bits set.  */
-        if (mask_bits == 32)
-            *mask = allones;
-        else
-            *mask = htonl(~(allones >> mask_bits));
+        if (mask_bits == mask_max_bits) {
+            set_allones(mask);
+        } else {
+            set_mask(mask, mask_bits);
+        }
     } else {
-        *mask = allones;
+        set_allones(mask);
     }
+    
     return 0;
 }
 
 
 /**
- * Check whether a client ought to be allowed.
+ * Compare the masked components of two 32-bit addresses.
  *
- * @returns 0 for allowed, or EXIT_ACCESS_DENIED.
+ * @returns 1 if they match, or 0.
  **/
-int dcc_check_address(in_addr_t client,
-                      in_addr_t value,
-                      in_addr_t mask)
+static inline int compare_address_32(const uint32_t *client,
+                                     const uint32_t *value,
+                                     const uint32_t *mask)
 {
-    if ((client & mask) == (value & mask)) {
+    if ((*client & *mask) == (*value & *mask))
+        return 1;
+    else
+        return 0;
+}
+
+/**
+ * Check whether two 32-bit addresses match.
+ *
+ * @returns 0 if they match, or EXIT_ACCESS_DENIED.
+ **/
+static int check_address_inet(const struct in_addr *client,
+                              const struct in_addr *value,
+                              const struct in_addr *mask)
+{
+    if (compare_address_32((uint32_t *)client, (uint32_t *)value, (uint32_t *)mask)) {
         rs_trace("match client %#lx, value %#lx, mask %#lx",
-                 (long) client, (long) value, (long) mask);
+                 (long) client->s_addr,
+                 (long) value->s_addr,
+                 (long) mask->s_addr);
         return 0;
     } else {
         rs_trace("deny client %#lx, value %#lx, mask %#lx",
-                 (long) client, (long) value, (long) mask);
+                 (long) client->s_addr,
+                 (long) value->s_addr,
+                 (long) mask->s_addr);
         return EXIT_ACCESS_DENIED;
     }
 }
+
+#ifdef ENABLE_RFC2553
+/**
+ * Compare the masked components of two v6 addresses
+ *
+ * @returns 1 if they match, or 0.
+ **/
+static int compare_address_inet6(const struct in6_addr *client,
+                                 const struct in6_addr *value,
+                                 const struct in6_addr *mask)
+{
+    const uint32_t *cp = (const uint32_t *)client;
+    const uint32_t *vp = (const uint32_t *)value;
+    const uint32_t *mp = (const uint32_t *)mask;
+    int i;
+    
+    for (i = 0; i < 4; ++i)
+        if (!compare_address_32(cp + i, vp + i, mp + i))
+            return 0;
+    
+    return 1;
+}
+
+
+/**
+ * Check whether two v6 addresses match.
+ *
+ * @returns 0 if they match, or EXIT_ACCESS_DENIED.
+ **/
+static int check_address_inet6(const struct in6_addr *client,
+                               const struct in6_addr *value,
+                               const struct in6_addr *mask)
+{
+    if (compare_address_inet6(client, value, mask)) {
+        rs_trace("match v6 client");
+        //rs_trace("match client %#lx, value %#lx, mask %#lx",
+        //(long) *client, (long) *value, (long) *mask);
+        return 0;
+    } else {
+        rs_trace("deny v6 client");
+        //rs_trace("deny client %#lx, value %#lx, mask %#lx",
+        //(long) *client, (long) *value, (long) *mask);
+        return EXIT_ACCESS_DENIED;
+    }
+}
+#endif /* ENABLE_RFC2553 */
+
+
+/**
+ * Check whether a client ought to be allowed.
+ *
+ * @returns 0 for allowed, or EXIT_ACCESS_DENIED.
+ **/
+int dcc_check_address(const struct sockaddr *client,
+                      const dcc_address_t *value,
+                      const dcc_address_t *mask)
+{
+    if (client->sa_family == AF_INET) {
+        /* The double-cast here avoids warnings from -Wcast-align. */
+        const struct sockaddr_in *sockaddr = (const struct sockaddr_in *) (void *) client;
+        const struct in_addr *cli_addr = &sockaddr->sin_addr;
+
+#ifndef ENABLE_RFC2553
+        return check_address_inet(cli_addr, value, mask);
+#else
+        if (value->family == AF_INET)
+            return check_address_inet(cli_addr, &value->addr.inet, &mask->addr.inet);
+        else
+            return EXIT_ACCESS_DENIED;
+        
+        
+    } else if (client->sa_family == AF_INET6) {
+        const struct sockaddr_in6 *sa6 = (const struct sockaddr_in6 *)client;
+        const struct in6_addr *a6 = &sa6->sin6_addr;
+        
+        if (value->family == AF_INET) {
+            if (IN6_IS_ADDR_V4MAPPED(a6) || IN6_IS_ADDR_V4COMPAT(a6)) {
+                const struct in_addr *a4 = (const struct in_addr *) &a6->s6_addr[12];
+                return check_address_inet(a4, &value->addr.inet, &mask->addr.inet);
+            } else
+                return EXIT_ACCESS_DENIED;
+        } else
+            return check_address_inet6(a6, &value->addr.inet6, &mask->addr.inet6);
+        
+#endif
+    } else {
+        rs_log_notice("access denied from unsupported address family %d",
+                      client->sa_family);
+        return EXIT_ACCESS_DENIED;
+    }
+}
Index: src/access.h
===================================================================
--- src/access.h (revision 676)
+++ src/access.h (working copy)
@@ -20,16 +20,31 @@
  * USA.
  */
 
+#include <config.h>
+
 /* access.c */
+
+#ifndef ENABLE_RFC2553
+typedef struct in_addr dcc_address_t;
+#else
+typedef struct dcc_address {
+    sa_family_t family;
+    union {
+        struct in_addr inet;
+        struct in6_addr inet6;
+    } addr;
+} dcc_address_t;
+#endif
+
 int dcc_parse_mask(const char *mask_spec,
-                   in_addr_t *value,
-                   in_addr_t *mask);
+                   dcc_address_t *value,
+                   dcc_address_t *mask);
 
-int dcc_check_address(in_addr_t client,
-                      in_addr_t value,
-                      in_addr_t mask);
+int dcc_check_address(const struct sockaddr *client,
+                      const dcc_address_t *value,
+                      const dcc_address_t *mask);
 
 struct dcc_allow_list {
-    in_addr_t           addr, mask;
+    dcc_address_t addr, mask;
     struct dcc_allow_list *next;
 };
Index: src/srvnet.c
===================================================================
--- src/srvnet.c (revision 676)
+++ src/srvnet.c (working copy)
@@ -252,35 +252,9 @@
     }
 
     for (l = allowed; l; l = l->next) {
-        if (psa->sa_family == AF_INET) {
-            in_addr_t cli_inaddr;
-            /* The double-cast here avoids warnings from -Wcast-align. */
-            cli_inaddr = ((struct sockaddr_in *) (void *) psa)->sin_addr.s_addr;
-
-            if ((ret = dcc_check_address(cli_inaddr, l->addr, l->mask)) == 0)
-                break;
-#ifdef ENABLE_RFC2553
-        } else if (psa->sa_family == AF_INET6) {
-            const struct sockaddr_in6 *sa6 = (const struct sockaddr_in6 *) psa;
-            const struct in6_addr *a6 = &sa6->sin6_addr;
-            const in_addr_t *a4;
-
-            if (IN6_IS_ADDR_V4MAPPED(a6) || IN6_IS_ADDR_V4COMPAT(a6)) {
-                a4 = (const in_addr_t *) &a6->s6_addr[12];
-                if ((ret = dcc_check_address(*a4, l->addr, l->mask)) == 0)
-                    break;
-            } else {
-                rs_log_notice("ipv6 ACLs not implemented");
-                free(client_ip);
-                return EXIT_ACCESS_DENIED;
-            }
-#endif
-        } else {
-            ret = EXIT_ACCESS_DENIED;
-            rs_log_notice("access denied from unknown address family %d",
-                          psa->sa_family);
+        ret = dcc_check_address(psa, &l->addr, &l->mask);
+        if (ret != EXIT_ACCESS_DENIED)
             break;
-        }
     }
 
     if (ret != 0) {
Index: src/zeroconf-reg.c
===================================================================
--- src/zeroconf-reg.c (revision 676)
+++ src/zeroconf-reg.c (working copy)
@@ -57,6 +57,11 @@
 static void publish_reply(AvahiEntryGroup *g, AvahiEntryGroupState state, void *userdata);
 
 static void register_stuff(struct context *ctx) {
+#ifndef ENABLE_RFC2553
+    static const AvahiProtocol dcc_proto = AVAHI_PROTO_INET;
+#else
+    static const AvahiProtocol dcc_proto = AVAHI_PROTO_UNSPEC;
+#endif
 
     if (!ctx->group) {
 
@@ -79,7 +84,7 @@
         if (avahi_entry_group_add_service(
                     ctx->group,
                     AVAHI_IF_UNSPEC,
-                    AVAHI_PROTO_UNSPEC,
+                    dcc_proto,
                     0,
                     ctx->name,
                     DCC_DNS_SERVICE_TYPE,

---

__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc

[Fergus Henderson-4]

Some parts of this message have been removed. Learn more about Nabble's security policy.

Thanks for the patch!

Some comments:

+        host_str = malloc(value_len + 1);
+        strncpy(host_str, spec, value_len);

Here you are not checking the return value of malloc().

Oh, I see that the old code that this is replacing didn't check the return value of strdup() either.
So I guess this is no worse.

+static void set_allones(dcc_address_t *addr)
+{
+#ifndef ENABLE_RFC2553
+    *addr = *(dcc_address_t *)&allones;

I'm concerned that this code may violate the C standard's rules on aliasing/type-punning.

+static void set_mask_inet6(struct in6_addr *mask, int mask_bits) {
+    uint32_t *iptr = (uint32_t *)mask;
+    int allones_count = mask_bits / 32;
+    int i;
+    
+    for(i = 0; i < allones_count; ++i, ++iptr)
+        *iptr = allones;
+    
+    set_mask_32(iptr, mask_bits % 32);
+    ++iptr;
+    
+    for(i = 4 - allones_count - 1; i > 0; --i, ++iptr)
+        *iptr = 0;
+}

Likewise here.

The C standard says: (ISO/IEC 9899:TC2, section 6.5, paragraph 7):

An object shall have its stored value accessed only by an lvalue expression that has one of
the following types:74)
-- a type compatible with the effective type of the object,
-- a qualified version of a type compatible with the effective type of the object,
-- a type that is the signed or unsigned type corresponding to the effective type of the
    object,
-- a type that is the signed or unsigned type corresponding to a qualified version of the
    effective type of the object,
-- an aggregate or union type that includes one of the aforementioned types among its
    members (including, recursively, a member of a subaggregate or contained union), or
-- a character type.

The Posix standard says:

^[IP6] The <netinet/in.h> header shall define the in6_addr structure that contains at least the following member:

uint8_t s6_addr[16]

So in general I don't think it is standard-conforming to assume that an in6_addr can be treated as an array of uint32_t.

I think that this issue could be avoided by rewriting the code using uint8_t instead of uint32_t and using the s6_addr member of the in6_addr struct instead of casts.

Now in practice the code probably works fine.  So maybe I'm being a bit pedantic here.
But I think it could be made standard-conforming without that much additional effort.
What do you think?

Cheers,
  Fergus.

On Mon, Jul 13, 2009 at 4:52 AM, Bob Ham <rah@...> wrote:

Hi there,

The attached patch implements IPv6 support for access control.  It also
includes a small update to restrict zeroconf advertisements to IPv4 if
RFC2553 support isn't enabled.

I've tested the patch both with RFC2553 support enabled and without.
There seem to be no regressions.

A more extensive zeroconf update will follow to restrict the addresses
it advertises to ones that are actually listened on.  It seems there may
be issues listening on IPv6 addresses as well.

Regards,

Bob Ham

--
Bob Ham <rah@...>

for (;;) { ++pancakes; }

__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc

--
Fergus Henderson <fergus@...>

__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc

[Bob Ham]

On Mon, 2009-07-20 at 22:23 -0400, Fergus Henderson wrote:

> Oh, I see that the old code that this is replacing didn't check the
> return value of strdup() either.

Indeed, both of the issues you raised were a continuation of the modus
operandi.  As you noted, the return value of strdup() was not checked.
Also, the following non-standard C was used in srvnet.c:

-            const in_addr_t *a4;
...
-                a4 = (const in_addr_t *) &a6->s6_addr[12];


Regardless, I've updated the patch to correct these issues.  I've also
made two more updates.  One is a fix to ensure the maximum bit mask size
is set according to the address family and not just whether RFC2553
support is enabled.

The other, slightly more worrying update modifies the non-RFC2553 code
to use inet_aton() instead of inet_pton().  The inet_pton() function is
defined in RFC2553.  This is worrying because it means that people have
been compiling distcc without RFC2553 support enabled but still using a
function defined by that RFC.  The fact that this state has continued
without being noticed previously calls into question the value of
maintaining non-RFC2553 code at all.


--
Bob Ham <rah@...>

[distcc-v6-acl-2.patch]

---

Index: src/access.c
===================================================================
--- src/access.c (revision 676)
+++ src/access.c (working copy)
@@ -37,6 +37,7 @@
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
+#include <netdb.h>
 
 #include "types.h"
 #include "distcc.h"
@@ -47,79 +48,329 @@
 /**
  * @file
  *
- * Simple IP-based access-control system
+ * IP-based access-control system
  */
 
-static const in_addr_t allones = 0xffffffffUL;
+static const uint32_t allones = 0xffffffffUL;
+static const uint8_t allones8 = 0xffU;
 
+/**
+ * Split a "HOST/BITS" mask specification into HOST and BITS.
+ *
+ * @returns 0 on success or -1 if the program ran out of heap space
+ **/
+static int split_mask(const char *spec,
+                       char **host,
+                       const char **bits)
+{
+    int value_len = strcspn(spec, "/");
+    
+    if (host != NULL) {
+        char *host_str;
+        
+        host_str = malloc(value_len + 1);
+        if (host_str == NULL) {
+            rs_log_error("could not allocate memory (%d bytes)", value_len + 1);
+            return -1;
+        }
+        strncpy(host_str, spec, value_len);
+        host_str[value_len] = '\0';
+        
+        *host = host_str;
+    }
+    
+    if (bits != NULL) {
+        if (spec[value_len] && spec[value_len + 1])
+            *bits = spec + value_len + 1;
+        else
+            *bits = NULL;
+    }
+    
+    return 0;
+}
 
 /**
+ * Set a dcc_address_t to a full mask.
+ **/
+static void set_allones(dcc_address_t *addr)
+{
+#ifndef ENABLE_RFC2553
+    addr->s_addr = allones;
+#else
+    memset(&addr->addr, allones8, sizeof(struct in6_addr));
+#endif
+}
+
+
+/**
+ * Set a 32-bit address, @p addr, to a mask of @p mask_size bits.
+ **/
+static inline void set_mask_inet(struct in_addr *addr, int mask_bits) {
+    addr->s_addr = htonl(~(allones >> mask_bits));
+}
+
+
+#ifdef ENABLE_RFC2553
+/**
+ * Set a v6 address, @p addr, to a mask of @p mask_size bits.
+ **/
+static void set_mask_inet6(struct in6_addr *mask, int mask_bits) {
+    uint8_t *iptr = mask->s6_addr;
+    int allones_count = mask_bits / 8;
+    int i;
+    
+    for(i = 0; i < allones_count; ++i, ++iptr)
+        *iptr = allones8;
+    
+    *iptr = ~(allones8 >> (mask_bits % 8));
+    ++iptr;
+    
+    for(i = 16 - allones_count - 1; i > 0; --i, ++iptr)
+        *iptr = 0;
+}
+#endif /* ENABLE_RFC2553 */
+
+
+/**
+ * Set a dcc_address_t to a mask of @p mask_bits bits.  The family field of
+ * the address should be set appropriately if RFC2553 is enabled.
+ **/
+static void set_mask(dcc_address_t *mask, int mask_bits)
+{
+#ifndef ENABLE_RFC2553
+    set_mask_inet(mask, mask_bits);
+#else
+    if (mask->family == AF_INET)
+        set_mask_inet(&mask->addr.inet, mask_bits);
+    else
+        set_mask_inet6(&mask->addr.inet6, mask_bits);
+#endif
+}
+
+/**
  * Interpret a "HOST/BITS" mask specification.  Return @p value and @p mask.
  **/
 int dcc_parse_mask(const char *spec,
-                   in_addr_t *value,
-                   in_addr_t *mask)
+                   dcc_address_t *value,
+                   dcc_address_t *mask)
 {
-    int value_len;
-    struct in_addr ia;
-    int mask_bits;
     char *value_str;
-    int matched;
+    int ret;
     const char *mask_str;
-
-    value_len = strcspn(spec, "/");
-
-    /* get bit before slash */
-    value_str = strdup(spec);
-    value_str[value_len] = '\0';
-    matched = inet_pton(AF_INET, value_str, &ia);
-
+#ifndef ENABLE_RFC2553
+    struct in_addr ia;
+#else
+    struct addrinfo hints;
+    struct addrinfo *res;
+#endif
+    
+    ret = split_mask(spec, &value_str, &mask_str);
+    if (ret == -1)
+        return EXIT_OUT_OF_MEMORY;
+    
     /* extract and parse value part */
-    if (!matched) {
+#ifndef ENABLE_RFC2553
+    ret = inet_aton(value_str, &ia);
+    if (ret == 0) {
         rs_log_error("can't parse internet address \"%s\"", value_str);
         free(value_str);
         return EXIT_BAD_ARGUMENTS;
     }
+    *value = ia;
+    
+#else /* ENABLE_RFC2553 */
+    
+    memset(&hints, 0, sizeof(hints));
+    hints.ai_flags = AI_NUMERICHOST;
+    hints.ai_family = AF_UNSPEC;
+    hints.ai_socktype = SOCK_STREAM;
+    
+    ret = getaddrinfo(value_str, NULL, &hints, &res);
+    if (ret != 0) {
+        rs_log_error("can't parse internet address \"%s\": %s",
+                     value_str, gai_strerror(ret));
+        free(value_str);
+        return EXIT_BAD_ARGUMENTS;
+    }
+    
+    value->family = res->ai_family;
+    if (res->ai_family == AF_INET) {
+        value->addr.inet = ((struct sockaddr_in *)res->ai_addr)->sin_addr;
+    } else if (res->ai_family == AF_INET6) {
+        value->addr.inet6 = ((struct sockaddr_in6 *)res->ai_addr)->sin6_addr;
+    } else {
+        rs_log_error("unsupported address family %d for address \"%s\"",
+                     res->ai_family, value_str);
+        freeaddrinfo(res);
+        free(value_str);
+        return EXIT_BAD_ARGUMENTS;
+    }
+    
+    freeaddrinfo(res);
+#endif /* !ENABLE_RFC2553 */
     free(value_str);
-    *value = ia.s_addr;
+    
+    
+#ifdef ENABLE_RFC2553
+    mask->family = value->family;
+#endif
+    if (mask_str != NULL) {
+        int mask_bits;
+#ifndef ENABLE_RFC2553
+        static const int mask_max_bits = 32;
+#else
+        int mask_max_bits;
+        if (mask->family == AF_INET)
+            mask_max_bits = 32;
+        else
+            mask_max_bits = 128;
+#endif
 
-    mask_str = &spec[value_len + 1];
-    if (spec[value_len] && *mask_str) {
         /* find mask length as a number of bits */
         mask_bits = atoi(mask_str);
-        if (mask_bits < 0 || mask_bits > 32) {
+        if (mask_bits < 0 || mask_bits > mask_max_bits) {
             rs_log_error("invalid mask \"%s\"", mask_str);
             return EXIT_BAD_ARGUMENTS;
         }
 
         /* Make a network-endian mask with the top mask_bits set.  */
-        if (mask_bits == 32)
-            *mask = allones;
-        else
-            *mask = htonl(~(allones >> mask_bits));
+        if (mask_bits == mask_max_bits) {
+            set_allones(mask);
+        } else {
+            set_mask(mask, mask_bits);
+        }
     } else {
-        *mask = allones;
+        set_allones(mask);
     }
     return 0;
 }
 
 
 /**
- * Check whether a client ought to be allowed.
+ * Check whether two 32-bit addresses match.
  *
- * @returns 0 for allowed, or EXIT_ACCESS_DENIED.
+ * @returns 0 if they match, or EXIT_ACCESS_DENIED.
  **/
-int dcc_check_address(in_addr_t client,
-                      in_addr_t value,
-                      in_addr_t mask)
+static int check_address_inet(const struct in_addr *client,
+                              const struct in_addr *value,
+                              const struct in_addr *mask)
 {
-    if ((client & mask) == (value & mask)) {
+    if ((client->s_addr & mask->s_addr) == (value->s_addr & mask->s_addr)) {
         rs_trace("match client %#lx, value %#lx, mask %#lx",
-                 (long) client, (long) value, (long) mask);
+                 (long) client->s_addr,
+                 (long) value->s_addr,
+                 (long) mask->s_addr);
         return 0;
     } else {
         rs_trace("deny client %#lx, value %#lx, mask %#lx",
-                 (long) client, (long) value, (long) mask);
+                 (long) client->s_addr,
+                 (long) value->s_addr,
+                 (long) mask->s_addr);
         return EXIT_ACCESS_DENIED;
     }
 }
+
+#ifdef ENABLE_RFC2553
+/**
+ * Compare the masked components of two v6 addresses
+ *
+ * @returns 1 if they match, or 0.
+ **/
+static int compare_address_inet6(const struct in6_addr *client,
+                                 const struct in6_addr *value,
+                                 const struct in6_addr *mask)
+{
+    int i;
+    
+    for (i = 0; i < 16; ++i)
+        if ((client->s6_addr[i] & mask->s6_addr[i]) != (value->s6_addr[i] & mask->s6_addr[i]))
+            return 0;
+    
+    return 1;
+}
+
+
+/**
+ * Check whether two v6 addresses match.
+ *
+ * @returns 0 if they match, or EXIT_ACCESS_DENIED.
+ **/
+static int check_address_inet6(const struct in6_addr *client,
+                               const struct in6_addr *value,
+                               const struct in6_addr *mask)
+{
+    if (compare_address_inet6(client, value, mask)) {
+        rs_trace("match v6 client");
+        //rs_trace("match client %#lx, value %#lx, mask %#lx",
+        //(long) *client, (long) *value, (long) *mask);
+        return 0;
+    } else {
+        rs_trace("deny v6 client");
+        //rs_trace("deny client %#lx, value %#lx, mask %#lx",
+        //(long) *client, (long) *value, (long) *mask);
+        return EXIT_ACCESS_DENIED;
+    }
+}
+
+/**
+ * Extract the lower 32-bits of a v6 address
+ **/
+static uint32_t inet_from_inet6(const struct in6_addr *addr)
+{
+    uint32_t dest = 0;
+    int i;
+    
+    for (i = 0; i < 4; ++i) {
+        uint32_t dest_byte = addr->s6_addr[12 + i];
+        dest |= (dest_byte << (8 * (3-i)));
+    }
+    
+    return dest;
+}
+#endif /* ENABLE_RFC2553 */
+
+
+/**
+ * Check whether a client ought to be allowed.
+ *
+ * @returns 0 for allowed, or EXIT_ACCESS_DENIED.
+ **/
+int dcc_check_address(const struct sockaddr *client,
+                      const dcc_address_t *value,
+                      const dcc_address_t *mask)
+{
+    if (client->sa_family == AF_INET) {
+        /* The double-cast here avoids warnings from -Wcast-align. */
+        const struct sockaddr_in *sockaddr = (const struct sockaddr_in *) (void *) client;
+        const struct in_addr *cli_addr = &sockaddr->sin_addr;
+
+#ifndef ENABLE_RFC2553
+        return check_address_inet(cli_addr, value, mask);
+#else
+        if (value->family == AF_INET)
+            return check_address_inet(cli_addr, &value->addr.inet, &mask->addr.inet);
+        else
+            return EXIT_ACCESS_DENIED;
+        
+        
+    } else if (client->sa_family == AF_INET6) {
+        const struct sockaddr_in6 *sa6 = (const struct sockaddr_in6 *)client;
+        const struct in6_addr *a6 = &sa6->sin6_addr;
+        
+        if (value->family == AF_INET) {
+            if (IN6_IS_ADDR_V4MAPPED(a6) || IN6_IS_ADDR_V4COMPAT(a6)) {
+                struct in_addr a4;
+                a4.s_addr = inet_from_inet6(a6);
+                return check_address_inet(&a4, &value->addr.inet, &mask->addr.inet);
+            } else
+                return EXIT_ACCESS_DENIED;
+        } else
+            return check_address_inet6(a6, &value->addr.inet6, &mask->addr.inet6);
+        
+#endif
+    } else {
+        rs_log_notice("access denied from unsupported address family %d",
+                      client->sa_family);
+        return EXIT_ACCESS_DENIED;
+    }
+}
Index: src/access.h
===================================================================
--- src/access.h (revision 676)
+++ src/access.h (working copy)
@@ -20,16 +20,31 @@
  * USA.
  */
 
+#include <config.h>
+
 /* access.c */
+
+#ifndef ENABLE_RFC2553
+typedef struct in_addr dcc_address_t;
+#else
+typedef struct dcc_address {
+    sa_family_t family;
+    union {
+        struct in_addr inet;
+        struct in6_addr inet6;
+    } addr;
+} dcc_address_t;
+#endif
+
 int dcc_parse_mask(const char *mask_spec,
-                   in_addr_t *value,
-                   in_addr_t *mask);
+                   dcc_address_t *value,
+                   dcc_address_t *mask);
 
-int dcc_check_address(in_addr_t client,
-                      in_addr_t value,
-                      in_addr_t mask);
+int dcc_check_address(const struct sockaddr *client,
+                      const dcc_address_t *value,
+                      const dcc_address_t *mask);
 
 struct dcc_allow_list {
-    in_addr_t           addr, mask;
+    dcc_address_t addr, mask;
     struct dcc_allow_list *next;
 };
Index: src/srvnet.c
===================================================================
--- src/srvnet.c (revision 676)
+++ src/srvnet.c (working copy)
@@ -252,35 +252,9 @@
     }
 
     for (l = allowed; l; l = l->next) {
-        if (psa->sa_family == AF_INET) {
-            in_addr_t cli_inaddr;
-            /* The double-cast here avoids warnings from -Wcast-align. */
-            cli_inaddr = ((struct sockaddr_in *) (void *) psa)->sin_addr.s_addr;
-
-            if ((ret = dcc_check_address(cli_inaddr, l->addr, l->mask)) == 0)
-                break;
-#ifdef ENABLE_RFC2553
-        } else if (psa->sa_family == AF_INET6) {
-            const struct sockaddr_in6 *sa6 = (const struct sockaddr_in6 *) psa;
-            const struct in6_addr *a6 = &sa6->sin6_addr;
-            const in_addr_t *a4;
-
-            if (IN6_IS_ADDR_V4MAPPED(a6) || IN6_IS_ADDR_V4COMPAT(a6)) {
-                a4 = (const in_addr_t *) &a6->s6_addr[12];
-                if ((ret = dcc_check_address(*a4, l->addr, l->mask)) == 0)
-                    break;
-            } else {
-                rs_log_notice("ipv6 ACLs not implemented");
-                free(client_ip);
-                return EXIT_ACCESS_DENIED;
-            }
-#endif
-        } else {
-            ret = EXIT_ACCESS_DENIED;
-            rs_log_notice("access denied from unknown address family %d",
-                          psa->sa_family);
+        ret = dcc_check_address(psa, &l->addr, &l->mask);
+        if (ret != EXIT_ACCESS_DENIED)
             break;
-        }
     }
 
     if (ret != 0) {
Index: src/zeroconf-reg.c
===================================================================
--- src/zeroconf-reg.c (revision 676)
+++ src/zeroconf-reg.c (working copy)
@@ -57,6 +57,11 @@
 static void publish_reply(AvahiEntryGroup *g, AvahiEntryGroupState state, void *userdata);
 
 static void register_stuff(struct context *ctx) {
+#ifndef ENABLE_RFC2553
+    static const AvahiProtocol dcc_proto = AVAHI_PROTO_INET;
+#else
+    static const AvahiProtocol dcc_proto = AVAHI_PROTO_UNSPEC;
+#endif
 
     if (!ctx->group) {
 
@@ -79,7 +84,7 @@
         if (avahi_entry_group_add_service(
                     ctx->group,
                     AVAHI_IF_UNSPEC,
-                    AVAHI_PROTO_UNSPEC,
+                    dcc_proto,
                     0,
                     ctx->name,
                     DCC_DNS_SERVICE_TYPE,

---

__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc

[Fergus Henderson-5]

Looks good to me.

There are some minor style issues where improvements are possible,  
e.g. using *iptr++ = ... everywhere instead of mixing the increments  
of iptr in the for(); and using the same type with just the ipv6 field  
of the union #ifdef'd, rather than the whole type. But this I'd fine  
as is.

On Jul 21, 2009, at 5:15 AM, Bob Ham <rah@...> wrote:

> On Mon, 2009-07-20 at 22:23 -0400, Fergus Henderson wrote:
>
>> Oh, I see that the old code that this is replacing didn't check the
>> return value of strdup() either.
>
> Indeed, both of the issues you raised were a continuation of the modus
> operandi.  As you noted, the return value of strdup() was not checked.
> Also, the following non-standard C was used in srvnet.c:
>
> -            const in_addr_t *a4;
> ...
> -                a4 = (const in_addr_t *) &a6->s6_addr[12];
>
>
> Regardless, I've updated the patch to correct these issues.  I've also
> made two more updates.  One is a fix to ensure the maximum bit mask  
> size
> is set according to the address family and not just whether RFC2553
> support is enabled.
>
> The other, slightly more worrying update modifies the non-RFC2553 code
> to use inet_aton() instead of inet_pton().  The inet_pton() function  
> is
> defined in RFC2553.  This is worrying because it means that people  
> have
> been compiling distcc without RFC2553 support enabled but still  
> using a
> function defined by that RFC.  The fact that this state has continued
> without being noticed previously calls into question the value of
> maintaining non-RFC2553 code at all.
>
>
> --
> Bob Ham <rah@...>
>
> --~--~---------~--~----~------------~-------~--~----~
> You received this message because you are subscribed to the "distcc-
> patches" list.
> To post to this list, send email to <distcc-patches@...>.
> To unsubscribe from this list, send email to <distcc-patches-unsubscribe@...
> >.
> For archives and more options, see <http://groups.google.com/group/distcc-patches 
> >.
> -~----------~----~----~----~------~----~------~--~---
>
> <distcc-v6-acl-2.patch>

__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc

[Fergus Henderson-5]

I applied this patch.
Then I remember to run "make check", and it turns out that "make check" fails with a compile error in code used for testing.
So I've sent a patch for the compile error.
I also made some stylistic improvements that I've sent as a separate patch.

However, the failure of "make check" also reminds me that it would be nice to have some tests of the IPv6 address parsing and access control,
analogous to the AccessDenied_Case and ParseMask_Case tests in test/testdistcc.py.  If you could make a patch for that, I'd really appreciate it.

Cheers,
  Fergus.

On Wed, Jul 22, 2009 at 10:32 AM, Fergus Henderson <fergus.henderson@...> wrote:

Looks good to me.

There are some minor style issues where improvements are possible, e.g. using *iptr++ = ... everywhere instead of mixing the increments of iptr in the for(); and using the same type with just the ipv6 field of the union #ifdef'd, rather than the whole type. But this I'd fine as is.

On Jul 21, 2009, at 5:15 AM, Bob Ham <rah@...> wrote:

On Mon, 2009-07-20 at 22:23 -0400, Fergus Henderson wrote:

Oh, I see that the old code that this is replacing didn't check the
return value of strdup() either.

Indeed, both of the issues you raised were a continuation of the modus
operandi.  As you noted, the return value of strdup() was not checked.
Also, the following non-standard C was used in srvnet.c:

-            const in_addr_t *a4;
...
-                a4 = (const in_addr_t *) &a6->s6_addr[12];


Regardless, I've updated the patch to correct these issues.  I've also
made two more updates.  One is a fix to ensure the maximum bit mask size
is set according to the address family and not just whether RFC2553
support is enabled.

The other, slightly more worrying update modifies the non-RFC2553 code
to use inet_aton() instead of inet_pton().  The inet_pton() function is
defined in RFC2553.  This is worrying because it means that people have
been compiling distcc without RFC2553 support enabled but still using a
function defined by that RFC.  The fact that this state has continued
without being noticed previously calls into question the value of
maintaining non-RFC2553 code at all.


--
Bob Ham <rah@...>

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the "distcc-patches" list.
To post to this list, send email to <distcc-patches@...>.
To unsubscribe from this list, send email to <distcc-patches-unsubscribe@...>.
For archives and more options, see <http://groups.google.com/group/distcc-patches>.
-~----------~----~----~----~------~----~------~--~---

<distcc-v6-acl-2.patch>

--
Fergus Henderson <fergus.henderson@...>

__
distcc mailing list            http://distcc.samba.org/
To unsubscribe or change options:
https://lists.samba.org/mailman/listinfo/distcc