Re: ncat should try connecting to all resolved addresses, not only the first one

[David Fifield <david () bamsoftware com>]

On Wed, Dec 11, 2013 at 11:33:07AM -0500, Jaromir Koncicky wrote:
> That sounds really good that my patch is working. Also thanks for fixing the casts and malloc.
> The variable targetaddrs_allocated is here to indicate that targetaddrs was dynamically allocated and should be freed 
> in the end of the program.
> However, I got rid of this and now I am using up the first static member of targetaddrs and allocating memory only 
> for the second address and so on.
> I tried to make some tests for this, but the only testing cases I came up for this is to use 'localhost' as connect 
> address of client. In not-patched version, new tests with IPv4 should fail because ncat tries only IPv6 which is 
> resolved first. In patched ncat, all new tests should pass. Do you have more ideas for the tests?

This new patch is looking good. I am inclined to accept it.

add_connection is not a good name. It doesn't sound like it is actually
asking Nsock to make a connection, which it is. Can you think of a
better name for it, maybe reflective of the fact that all the functions
it calls are called ncat_connect_*?

There shouldn't be duplicated code in resolve_multi. I suggest that you
instead modify resolve_internal to return all addresses, and change
resolve to ignore all but the first result.

You should allocate the whole targetaddrs list. It's tricky if only the
first element is statically allocated. It means that some blocks of code
will be executed only rarely, which leaves more room for bugs to hide.

Thanks for writing tests; however they don't conceptually work in my
opinion. They rely on the name "localhost" resolving to more than one
address, which is not the case everywhere and anyway is out of the
control of the test program. I think we should leave these tests out
until we can think of better ones.

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