Re: ncat - UNIX-domain sockets support

[David Fifield <david () bamsoftware com>]

On Mon, Nov 26, 2012 at 10:44:53AM -0500, Tomas Hozza wrote:
> ----- Original Message -----
> > > I used tempnam() function which uses TMPDIR and TMP, so "/tmp" is
> > > not
> > > hardcoded. It was done so also in the last bunch of patches.
> > >
> > > > http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/avoid-race.html#TEMPORARY-FILES
> > >
> > > There is example of how to use tempnam function to be more safe,
> > > but unfortunately this can not be used in this case. The problem is
> > > that after the temporary name is generated, we are not
> > > opening/creating
> > > any file/socket. It is done later when calling bind() on the source
> > > socket FD
> > > together with the temporary name (so the socket binds to the
> > > path/name).
> >
> > This is the way I was thinking before too. But netcat-openbsd calls
> > mkstemp, at least if I understand correctly. Apparently it is no
> > problem for the file to exist as a regular file before binding to it?
>
> 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.

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.

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

> Regarding to this I discovered one Bug in Nsock library, that caused
> bad statistic output when client tried to send some data to server,
> but the server was not running any more. I this case it caused output
> like the following (where you can see "18446744073709551615 bytes sent"):

Thanks, I committed this separately.

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