Re: ncat - UNIX-domain sockets support

[Tomas Hozza <thozza () redhat com>]

----- Original Message -----
> On Mon, Nov 26, 2012 at 10:44:53AM -0500, Tomas Hozza wrote:
> > ----- Original Message -----
> > If the file already exists it is a real problem. The bind() call
> > will fail
> > with errno 98 "Address already in use". Therefore when creating
> > source socket
> > "file" (when bind() is called) file with the given/generated path
> > MUST NOT exist.
>
> I have made some changes and sommitted your patch. Thank you for
> spending time on it. My comments are below.

Thank you very much. Unfortunately you removed one extra part of the
patch that shouldn't be removed by my opinion.

When source socket exist, when Ncat is connecting to a server using
DATAGRAM UNIX sockets, Ncat should exit. Therefore nsock_connect() should
return "-1" when bind() returns "-1" in case of UNIX sockets.

> I see now that netcat-openbsd calls unlink just before bind, so the
> path
> doesn't exist:
>
> int
> unix_bind(char *path)
> {
>       ...
>
>         unlink(path);
>
>         if (bind(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) < 0) {
>                 close(s);
>                 return (-1);
>         }
>         return (s);
> }
>
> > Another thing I found out after discussion with my colleagues is,
> > that
> > Ncat's configure script should let user specify the directory for
> > creating
> > temporary source DGRAM Unix sockets generated by Ncat. Therefore I
> > added
> > new argument into Ncat configure script called TMP_UNIX_SOCK_DIR.
> > If it is
> > specified, then configure will define macro in config.h header.
> > Depending on
> > whether it is specified, tempnam() is compiled with or without
> > specific
> > directory set:
> >
> > #ifdef TMP_UNIX_SOCK_DIR
> >                 if ((tmp_name = tempnam(TMP_UNIX_SOCK_DIR,
> >                 "ncat.")) == NULL)
> > #else
> >                 if ((tmp_name = tempnam(NULL, "ncat.")) == NULL)
> > #endif
> >
> > Even if TMP_UNIX_SOCK_DIR is specified, tempnam() prefers TMPDIR
> > environment
> > variable if it is set.
> >
> > My motivation for this is, that regarding to FSH 2.3 standard
> > (http://www.pathname.com/fhs/pub/fhs-2.3.html#VARRUNRUNTIMEVARIABLEDATA)
> > System programs that maintain transient UNIX-domain sockets must
> > place
> > them in "/var/run". Therefore we would like to have a way to
> > specify
> > the directory in compilation time.
>
> This seems like good reasoning. However I have removed this part of
> the
> patch. The name TMP_UNIX_SOCK_DIR hardly seems standard; what do
> other
> packages do in this regard? Also since one of the ideas behind adding
> Unix domain support was compatibility with openbsd-netcat, I guess
> that
> creating files in /tmp is fine. Distributors might like to patch the
> source code rather than rely on an environment variable. You are
> welcome
> to resubmit this part, but I will ask that you show me some other
> package that deals with the same issue.

The variable is not defined as Environment Variable in the system. This
variable is used just in configuration script for defining the path in
config.h header if specified as an argument to the script.

> > I modified the code, so Ncat does not call unlink on socket, that
> > it did not create.
>
> I checked this part of the patch, but I had to remove it. Nsock is
> not
> allowed to look into userdata in this way. Only the calling program
> is
> allowed to read and write userdata.
>
> +#if HAVE_SYS_UN_H
> +    if (ss->ss_family == AF_UNIX) {
> +      if (type == SOCK_DGRAM && nse->userdata)
> +        *(int *)nse->userdata = 1;
> +    } else
> +#endif

I knew this was not really the best solution, but it was the less
complex one. Maybe in the future I'll come up with something better.

> > > I'd also like to be sure to unlink the socket even if there is an
> > > error.
> > > Would you check if it's possible to replace
> > >         exit(1);
> > > with
> > >         nsock_loop_quit(nsp);
> > >         return;
> > > in the ncat_connect callbacks?
> >
> > Thank you for this idea. I used it in every handler function, so
> > now
> > the created socket is deleted if any ERROR occurs.
>
> Thanks for checking this. This change appears to have broken some of
> the
> tests:
>
> FAIL Connect connection refused exit code
>      Exit code was 0, not 1 at ./ncat-test.pl line 594.
> FAIL Connect connection interrupted exit code
>      Exit code was 0, not 1 at ./ncat-test.pl line 622.
>
> I reverted this part of the patch because I'd rather have a new bug
> (Unix domain socket failing to clean up) than a regression.

I understand your reasons. I didn't noticed this.


Tomas Hozza

Attachment: 0001-If-source-DGRAM-UNIX-socket-exist-Ncat-should-exit.patch
Description:

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