Re: Dual Listening on IPV4 & 6 in broker and listen modes

[David Fifield <david () bamsoftware com>]

On Wed, Jun 22, 2011 at 04:58:43PM -0500, Colin L. Rice wrote:
> Hello,
>
> At the request of David I rewrote broker to allow clients using IPV4 and
> IPV6 to communicate with each other using the default broker as well as
> rewriting the whole dual listening system to make a little more sense
> and be more maintainable.
>
> However due to a weird feature of Linux where :: binds to 0.0.0.0 as
> well we added a change where all IPV6 sockets now specify IPV6_V6ONLY so
> on Linux networking stacks you can have a dual listening mode without
> collisions. This means however that ncat -l -6 :: will not accept IPV4
> connections in their IPV6 address format on Linux.

This isn't a feature of Linux only, it's part of the design of IPv6
sockets. See sections 3.7 "Compatibility with IPv4 Nodes" and 5.3
"IPV6_V6ONLY option for AF_INET6 Sockets" of RFC 3493.
http://tools.ietf.org/html/rfc3493
Some operating systems disable the "compatibility" part by default for
technical or security reasons.
http://tools.ietf.org/html/draft-itojun-v6ops-v4mapped-harmful-02

> The patch is attached. Does anyone have any feedback? Would you prefer
> that IPV6_V6ONLY only be set for ncat -l or ncat --broker and it be
> turned off -6 is passed? 

I think that -6 mode should not accept any IPv4 connections, not even
from IPv4-mapped addresses like ::ffff:127.0.0.1. If I run "ncat -6 -l 80"
I expect to also be able to run a separate IPv4 web server. I think
we should turn on IPV6_V6ONLY for all IPv6 sockets.

> Index: ncat_listen.c
> ===================================================================
> --- ncat_listen.c     (revision 24247)
> +++ ncat_listen.c     (working copy)
>      /* setup the main listening socket */
> -    listen_socket = do_listen(SOCK_STREAM, proto, &srcaddr);
> +    listen_socket = do_listen(SOCK_STREAM, proto, &srcaddr[0]);

I think that wherever you refer to srcaddr[0], you need to refer to
srcaddr[1] equally. They should not be separated in the code. The best
way is to loop over num_srcaddrs. listen_socket should be an array of
the same size as srcaddr, not separate variables listen_socket and
listen_socket6.

> @@ -545,7 +535,7 @@
>  
>      while (1) {
>          /* create the UDP listen socket */
> -        sockfd = do_listen(SOCK_DGRAM, proto, &srcaddr);
> +        sockfd = do_listen(SOCK_DGRAM, proto, &srcaddr[0]);

This looks broken to me. The UDP mode only refers to srcaddr[0].

> Index: ncat_main.c
> ===================================================================
> --- ncat_main.c       (revision 24247)
> +++ ncat_main.c       (working copy)
> @@ -533,6 +533,7 @@
>  
>      if (o.verbose)
>          print_banner();
> +
>      if(o.debug)
>          nbase_set_log(loguser, logdebug);
>      else
> @@ -541,7 +542,7 @@
>      /* Will be AF_INET or AF_INET6 when valid */
>      memset(&targetss.storage, 0, sizeof(targetss.storage));
>      targetss.storage.ss_family = AF_UNSPEC;
> -    httpconnect.storage = socksconnect.storage = srcaddr.storage = targetss.storage;
> +    httpconnect.storage = socksconnect.storage = srcaddr[0].storage = srcaddr[1].storage = targetss.storage;

I would prefer that you handle these things in a loop, instead of
hardcoding 0 and 1.

>      if (proxyaddr) {
>        if (!o.proxytype)
> @@ -582,7 +583,7 @@
>          if (o.listen)
>              bye("-l and -s are incompatible.  Specify the address and port to bind to like you would a host to 
> connect to.");
>  
> -        if (!resolve(source, 0, &srcaddr.storage, &srcaddrlen, o.af))
> +        if (!resolve(source, 0, &srcaddr[0].storage, &srcaddrlen[0], o.af))
>              bye("Could not resolve source address %s.", source);
>      }
>  
> @@ -592,18 +593,18 @@
>                 compatibility. */
>              o.portno = srcport;
>          } else {
> -            if (srcaddr.storage.ss_family == AF_UNSPEC)
> -                srcaddr.storage.ss_family = o.af;
> +            if (srcaddr[0].storage.ss_family == AF_UNSPEC)
> +                srcaddr[0].storage.ss_family = o.af;
>              if (o.af == AF_INET) {
> -                srcaddr.in.sin_port = htons((unsigned short) srcport);
> -                if (!srcaddrlen)
> -                    srcaddrlen = sizeof(srcaddr.in);
> +                srcaddr[0].in.sin_port = htons((unsigned short) srcport);
> +                if (!srcaddrlen[0])
> +                    srcaddrlen[0] = sizeof(srcaddr[0].in);
>              }
>  #ifdef HAVE_IPV6
>              else {
> -                srcaddr.in6.sin6_port = htons((unsigned short) srcport);
> -                if (!srcaddrlen)
> -                    srcaddrlen = sizeof(srcaddr.in6);
> +                srcaddr[0].in6.sin6_port = htons((unsigned short) srcport);
> +                if (!srcaddrlen[0])
> +                    srcaddrlen[0] = sizeof(srcaddr[0].in6);
>              }
>  #endif
>          }
> @@ -626,6 +627,7 @@
>              if (!resolve(o.target, 0, &targetss.storage, &targetsslen, o.af))
>                  bye("Could not resolve hostname %s.", o.target);
>              optind++;
> +            o.inetset=1;

I think you can make this code a lot clearer. Instead of overloading the
meaning of the global srcaddr array (used for both option processing and
as data passed to ncat_listen), use a temporary variable for option
processing only. Then after the option loop, it can be very clear:

num_srcaddrs = 0;
if (tmp_addr.storage.ss_family == AF_UNSPEC) {
        if (o.af == AF_INET || o.af == AF_UNSPEC)
                srcaddr[num_srcaddrs++] = 0.0.0.0;
        if (o.af == AF_INET6 || o.af == AF_UNSPEC)
                srcaddr[num_srcaddrs++] = ::;
} else {
        srcaddr[num_srcaddrs++] = tmp_addr;
}

I think that better shows your intention of setting up two listening
addresses when no address and neither -4 nor -6 is given. This means
that you will have to change the default value of o.af in listen mode; I
think that's cleaner than the o.inetset variable.

This also can clear up a lot of confusion when someone is reading the
option processing loop while thinking about connect mode: "Why are they
using an array when you can only connect to one address?"

> Index: ncat_core.c
> ===================================================================
> --- ncat_core.c       (revision 24247)
> +++ ncat_core.c       (working copy)
> @@ -107,8 +107,8 @@
>  #include <time.h>
>  #include <assert.h>
>  
> -union sockaddr_u srcaddr;
> -size_t srcaddrlen;
> +union sockaddr_u srcaddr[2];
> +size_t srcaddrlen[2];

Do we need srcaddrlen? Could we get rid of it?

This declaration needs a comment saying why the array has size 2: we set
up at most 2 listening sockets, one for IPv4 and one for IPv6.

David Fifield
_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://seclists.org/nmap-dev/