Re: ncat - UNIX-domain sockets support

[David Fifield <david () bamsoftware com>]

On Mon, Sep 24, 2012 at 08:31:15AM -0400, Tomas Hozza wrote:
> I finished implementing the UNIX-domain socket support for Ncat and Nsock.
> I'm attaching two patches, one is for Nsock library and the second is for
> Ncat. The listen functionality is implemented the same way it is now in Ncat,
> but I have no problem rewrite it after you introduce new functions and support
> for listening in Nsock library.
>
> I would like to ask everybody to have look on my patches and give me some
> feedback, if it's OK, or what should be changed.

This code looks quite solid, thanks for it. I have two requests for this
patch:
 1. Reduce code duplication in ncat_listen.c.
 2. Add tests for local sockets in test/ncat-test.pl.

nsock_connect_unixsock_internal should be static. Also, it looks like
more common code can be moved into nsock_connect_internal.

ncat_listen_unixsock_stream and ncat_listen_unixsock_dgram look like
mostly copy-pastes from ncat_listen_stream and ncat_listen_dgram. We
can't accept that for reasons of maintenance. These need to be
integrated in the existing loops; this may mean building new supporting
abstractions.

Is there any reason not to handle both SOCK_STREAM and SOCK_DGRAM Unix
domain sockets in the same loop? I don't know if the same reasoning
applies as for separating ncat_listen_stream and ncat_listen_dgram. (I'm
not sure why ncat_listen_stream and ncat_listen_dgram are separate
either.)

> -      nsock_trace(ms, "Callback: %s %s %sfor EID %li [%s:%d]",
> -                  nse_type2str(nse->type), nse_status2str(nse->status), errstr,
> -                  nse->id, inet_ntop_ez(&nsi->peer, nsi->peerlen), nsi_peerport(nsi));
> +#if HAVE_SYS_UN_H
> +      if (nsi->peer.ss_family == AF_UNIX)
> +        nsock_trace(ms, "Callback: %s %s %sfor EID %li [%s]",
> +                    nse_type2str(nse->type), nse_status2str(nse->status), errstr,
> +                    nse->id, get_unixsock_path(&nsi->peer));
> +      else
> +#endif
> +        nsock_trace(ms, "Callback: %s %s %sfor EID %li [%s:%d]",
> +                    nse_type2str(nse->type), nse_status2str(nse->status), errstr,
> +                    nse->id, inet_ntop_ez(&nsi->peer, nsi->peerlen), nsi_peerport(nsi));

Here and in many other places it looks like you need an abstraction
around inet_ntop_ez and get_unixsock_path rather than duplicating the
nsock_trace call.

> +        if (o.chat)
> +            bye("Chat option not supported when using UNIX-domain sockets.");
> +#ifdef HAVE_OPENSSL
> +        if (o.ssl)
> +            bye("SSL option not supported when using UNIX-domain sockets.");
> +#endif
> +        if (o.nodns)
> +            bye("Not using DNS option with UNIX-domain sockets doesn't make sense.");
> +        if (o.broker)
> +            bye("Connection brokering not supported when using UNIX-domain sockets.");

Do all these things intrinsically not work with Unix domain sockets? Or
is it just a side effect of not sharing the same loop. I see no reason
to forbid o.nodns: that option may have effects beyond just looking up a
connect target.

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