Re: [nmap-svn] r31378 - nmap-exp/d33tah/ncat-env-conninfo/ncat

[d33 tah <d33tah () gmail com>]

2013/7/18 Henri Doreau <henri.doreau () gmail com>:
> 2013/7/17 d33 tah <d33tah () gmail com>:
> > Hi Henri,
> >
> > > > +    *env = (LPTSTR) safe_realloc(*env, *env_size+added_value_len);
> > > > +
> > > > +    Snprintf(*env + *env_size-1, added_value_len, "%s=%s", name, value);
> > > > +
> > > > +    *env_size += added_value_len;
> > > > +    (*env)[*env_size] = '\0';
> > > > +    (*env)[*env_size-1] = '\0';
> > > Why do you zero the last *two* indices? Safety? You might want to
> > > check/use Snprintf return value too.
> >
> > As Windows MSDN docs say, the environment description string contains
> > a NULL-separated array of key=value strings. Its ending is marked by
> > two NULL bytes - probably one for the end of string, and another to
> > make next string zero-bytes long.
> Thanks, I see.
>
> > > > +}
> > > > +
> > > > +static void setup_environment(struct fdinfo *info, LPTSTR* env, int *env_size)
> > > > +{
> > > > +    char *dest_addr=NULL;
> > > > +    char dest_port[10];
> > > > +    union sockaddr_u su;
> > > > +    unsigned short port;
> > > > +    char str[16];
> > > > +    char ip[4 * INET6_ADDRSTRLEN];
> > > Why is ip 4*INET6_ADDRSTRLEN?
> >
> > No idea really. I copied it from here (line 1821):
> > http://git.savannah.gnu.org/cgit/inetutils.git/tree/src/inetd.c
> > I'll do some digging about what kind of buffers Ncat uses to represent
> > IP's - if you know the answer already, I'd be happy to hear it.
> There's a comment in inetd.c, according to which this
> 4*INET6_ADDRSTRLEN is a tradeoff between space and official max
> hostname length (in their code, the buffer is susceptible to handle a
> hostname too). I believe that INET6_ADDRSTRLEN is big enough in your
> case, because you're requesting NI_NUMERICHOST and the getnameinfo
> which is provided by nbase doesn't support IPv6... Why I'm insisting
> on that is because this strange constant makes code hard to understand
> ("why 4 times something already larger than needed?"). I wouldn't want
> that to end up being a line that noone dares touching, labeled with a
> comment like: /* XXX: isn't this too big? */ :)
>
> Finally, and way more important than sizing a buffer, please pay
> attention to licensing issues. Nmap license isn't compatible with
> GPLv3. I think BSD code is Ok (looks like this inetutils file has
> both) but IIRC the BSD header has to be reproduced then. I let fyodor
> or someone confirm but you might want to rewrite this code.
>
> Regards
>
> --
> Henri

(sorry for replying twice, my GMail replied only to Henri by default)


Thanks for the warning, I'll pay more attention from now on. Though I
don't really believe that two variable declarations (out of which one
was altered already) and two calls is enough to void any license. It'd
be like patenting a set of getsockname/getnameinfo arguments.
Basically, only the following code is from inetd.c:

    char str[16];
    char ip[INET6_ADDRSTRLEN];

(...)

    if (getsockname (info->fd, (struct sockaddr *) &su, &alen) < 0) {
        bye("getsockname failed: %s",
socket_strerror(socket_errno()));
    }

    if (getnameinfo ((struct sockaddr *) &su, alen, ip, sizeof (ip),
            str, sizeof (str), NI_NUMERICHOST | NI_NUMERICSERV)==0) {
        _NCAT_SETENV("NCAT_LOCAL_ADDR", ip);
        _NCAT_SETENV("NCAT_LOCAL_PORT", str);
    } else {
        bye("getsockname failed: %s",
socket_strerror(socket_errno()));
    }

And consult its original:

  char ip[4 * INET6_ADDRSTRLEN];
  int ret;

(...)

  if (getsockname (ctrl, (struct sockaddr *) &sa_server, &len) < 0)
    syslog (LOG_WARNING, "getsockname(): %m");
  else
    {
      ret = getnameinfo ((struct sockaddr *) &sa_server, len,
                          ip, sizeof (ip), str, sizeof (str),
                          NI_NUMERICHOST | NI_NUMERICSERV);
      if (ret == 0)
        {
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/