Re: ncat - UNIX-domain sockets support

[Tomas Hozza <thozza () redhat com>]

Hi David.

Thank you very much for reviewing my patches. I did some refactoring
and added/changed thinks you've requested.

> This code looks quite solid, thanks for it. I have two requests for
> this
> patch:
>  1. Reduce code duplication in ncat_listen.c.

I removed duplicated sections of code and used existing functions
ncat_listen_stream and ncat_listen_dgram.

>  2. Add tests for local sockets in test/ncat-test.pl.

I added three tests for UNIX domain sockets implementation in Ncat.

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

I removed the nsock_connect_unixsock_internal and one other function for
making sockets and moved more common code to the existing functions.
 
> 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.

They are gone! I used existing functions.

> 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.)

As stated earlier, I removed ncat_listen_stream and ncat_listen_dgram.
SOCK_STREAM and SOCK_DGRAM sockets cannot be handled in the same loop,
because STREAM sockets are with connection while the DGRAM sockets are
connectionless.

> > -      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.

I moved the abstraction to new get_hostaddr_string() function.

> > +        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.

Well, to be honest I'm not sure if they won't work with UNIX sockets.
I removed some of them and included only ones I think they won't work.

It would be great if you (David) or other people would have a look on
my new patches and give me some feedback.

Thanks!

Regards,
Tomas Hozza

Attachment: 0001-Implementation-of-UNIX-domain-sockets-for-Nsock.patch
Description:

Attachment: 0002-Implementation-of-UNIX-domain-sockets-for-Ncat.patch
Description:

Attachment: 0003-Tests-for-UNIX-sockets-implementation-in-Ncat.patch
Description:

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