Re: Patches to NMAP-6.01

[David Fifield <david () bamsoftware com>]

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?

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

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

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

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

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