Re: Patches to NMAP-6.01

[Bill Parker <wp02855 () gmail com>]

I will start working on this probably sometime in the next day or so.

On Tue, Nov 20, 2012 at 8:53 PM, David Fifield <david () bamsoftware com>wrote:

> On Tue, Nov 13, 2012 at 12:14:32PM -0800, Bill Parker wrote:
> >    In reviewing code in NMAP-6.01, I found various calls to
> > malloc()/calloc()/realloc() which were not being checked
> > for a return value of NULL which would indicate failure.
> >
> > In addition, I also found deprecated calls to bzero() which
> > were replaced by memset() which is ANSI/ISO compliant, a
> > call to read() which isn't checked for failure, and added
> > a warning message in the event the call to munmap() cannot
> > unmap the memory location in question.
> >
> > In directory nmap-6.01/libpcap, file 'pcap-sita.c', several
> > of these calls were found, and code was added to check
> > for return values of NULL.  Additionally, the deprecated
> > calls to bzero() were replaced with memset().  The patch file
> > listing is below:
>
> Thanks for these patches. Some of them need to be sent upstream, which
> you have done:
>
> > pcap-sita.c.patch
> > pcap.c.patch
> > pcap-bpf.c.patch
> > predict.c.patch
>
> Here are some comments on the others.
>
> > intf.c.patch
> > ip.c.patch
> > In directory nmap-6.01/libdnet-stripped/src, file 'intf.c', the
> > last argument in getsockname should be casted to (socklen_t *)
> > per the manual/posix specification.
> > -       if (getsockname(intf->fd, (struct sockaddr *)&sin, &n) < 0)
> > +       if (getsockname(intf->fd, (struct sockaddr *)&sin, (socklen_t
> *)&n)
> > -       if (getsockopt(i->fd, SOL_SOCKET, SO_SNDBUF, &n, &len) < 0)
> > +       if (getsockopt(i->fd, SOL_SOCKET, SO_SNDBUF, &n, (socklen_t
> *)&len)
>
> Does this change actually make any difference? Doesn't the compiler
> coerce the type of the pointer automatically? Is there a tool that is
> complaining about this code?

In the above case, it's kind of a toss-up whether or not the compiler gets
annoyed
by stuff like this, though eventually it would wind up being POSIX
compliant if it
was referred to in the manner above. <shrug>

> > arp-win32.c.patch
> > route-win32.c.patch
>
> These are fine, but please redo them without the calls to fprintf,
> because libdnet doesn't print to stderr in other places. Also please
> mind the prevailing code style, including parentheses around return
> values.
>
> I will look for the equivalent error printing routine in libdnet, and
change the code to reflect that, then re-submit :)


> > intf-win32.c.patch
>
> There might be more required in this patch. Maybe _ifcombo_add should
> have a return value indicating success or failure that can be propagated
> upward. As it is, the patch postpones a segmentation violation that
> otherwise would be immediate, which might make a runtime error harder to
> diagnose.

Not a bad idea, I'll have to review the code stream to see what is needed
for future development.

> > rand.c.patch
>
> This is a good one to fix. It looks to me like the function is
> deliberately ignoring the return value of read, because it doesn't treat
> failure to seed the RNG as an error (which arguably is a severe bug,
> depending on what it is used for). You may be able to make a case that
> the function should return NULL if it can't get sufficient randomness.
> As it is, this function appears not to be used by Nmap, so I'm more
> likely just to delete rand.c than try to make design decisions about it.
>
> If rand.c isn't used by Nmap, then it would probably be a good idea to
comment out any reference to it in the NMAP source tree, rather than
deleting the code, since it's always harder to put something back
in rather than remove comment blocks or preprocessor directives
like #if 1 or #if 0


> > nmap.cc.patch
>
> Showing a message if munmap fails is a good idea. Please change the
> message so it includes the strerror, and match the indentation of the
> surrounding code.
>
> I can do this...

Bill Parker

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